[Qemu-devel] [PATCH for-2.11 0/5] qcow2: Fixes for corrupted images

[Qemu-devel] [PATCH for-2.11 0/5] qcow2: Fixes for corrupted images

[Max Reitz]

This series contains fixes for another batch of qcow2-related crashes
reported on Launchpad by Nageswara (the first batch was
http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00082.html by
Berto).

Patch 4 fixes an out-of-bounds array access in memory which is not
really a security issue for multiple reasons (really, at most you can
read eight bytes from somewhere with an extremely high chance of
crashing qemu and requiring the user to invoke a block_resize shrinking
the qcow2 image (and also reset some bit in the image from 1 to 0, but
only if the overlap checks don't catch you)), but most importantly that
code hasn't been in 2.10, so we're fine.


Max Reitz (5):
  qcow2: check_errors are fatal
  qcow2: Unaligned zero cluster in handle_alloc()
  block: Guard against NULL bs->drv
  qcow2: Add bounds check to get_refblock_offset()
  qcow2: Refuse to get unaligned offsets from cache

 block/qcow2.h              |   6 ---
 block.c                    |  19 ++++++-
 block/io.c                 |  36 +++++++++++++
 block/qapi.c               |   8 ++-
 block/qcow2-cache.c        |  21 ++++++++
 block/qcow2-cluster.c      |  13 ++++-
 block/qcow2-refcount.c     |  26 +++++++++-
 block/qcow2.c              |   5 +-
 block/replication.c        |  15 ++++++
 block/vvfat.c              |   2 +-
 tests/qemu-iotests/060     | 125 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 115 +++++++++++++++++++++++++++++++++++++++++
 12 files changed, 379 insertions(+), 12 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 1/5] qcow2: check_errors are fatal

[Max Reitz]

When trying to repair a dirty image, qcow2_check() may apparently
succeed (no really fatal error occurred that would prevent the check
from continuing), but if check_errors in the result object is non-zero,
we cannot trust the image to be usable.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728639
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c              |  5 ++++-
 tests/qemu-iotests/060     | 20 ++++++++++++++++++++
 tests/qemu-iotests/060.out | 23 +++++++++++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 92e5d548e3..d4fcb0250d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1475,7 +1475,10 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
         BdrvCheckResult result = {0};
 
         ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
-        if (ret < 0) {
+        if (ret < 0 || result.check_errors) {
+            if (ret >= 0) {
+                ret = -EIO;
+            }
             error_setg_errno(errp, -ret, "Could not repair dirty image");
             goto fail;
         }
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index fae08b03bf..56bdf1ee2e 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -301,6 +301,26 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "48"                "\x00\x00\x00\x00\x00\x00\x00\x00"
 $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing dirty corrupt image ==="
+echo
+
+_make_test_img 64M
+
+# Let the refblock appear unaligned
+poke_file "$TEST_IMG" "$rt_offset"        "\x00\x00\x00\x00\xff\xff\x2a\x00"
+# Mark the image dirty, thus forcing an automatic check when opening it
+poke_file "$TEST_IMG" 72 "\x00\x00\x00\x00\x00\x00\x00\x01"
+# Open the image (qemu should refuse to do so)
+$QEMU_IO -c close "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt
+
+echo '--- Repairing ---'
+
+# The actual repair should have happened (because of the dirty bit),
+# but some cleanup may have failed (like freeing the old reftable)
+# because the image was already marked corrupt by that point
+_check_test_img -r all
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 62c22701b8..f013fe73c0 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -284,4 +284,27 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at offset 0; further corruption events will be suppressed
 write failed: Input/output error
+
+=== Testing dirty corrupt image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+ERROR refcount block 0 is not cluster aligned; refcount table entry corrupted
+IMGFMT: Marking image as corrupt: Refblock offset 0xffff2a00 unaligned (reftable index: 0); further corruption events will be suppressed
+Can't get refcount for cluster 0: Input/output error
+Can't get refcount for cluster 1: Input/output error
+Can't get refcount for cluster 2: Input/output error
+Can't get refcount for cluster 3: Input/output error
+Rebuilding refcount structure
+Repairing cluster 1 refcount=1 reference=0
+can't open device TEST_DIR/t.IMGFMT: Could not repair dirty image: Input/output error
+--- Repairing ---
+Leaked cluster 1 refcount=1 reference=0
+Repairing cluster 1 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+    1 leaked clusters
+    0 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
 *** done
-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 2/5] qcow2: Unaligned zero cluster in handle_alloc()

[Max Reitz]

We should check whether the cluster offset we are about to use is
actually valid; that is, whether it is aligned to cluster boundaries.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728643
Buglink: https://bugs.launchpad.net/qemu/+bug/1728657
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c      | 13 ++++++++++++-
 tests/qemu-iotests/060     | 16 ++++++++++++++++
 tests/qemu-iotests/060.out | 10 ++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 2e072ed155..a3fec27bf9 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1308,10 +1308,21 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
         (!*host_offset ||
          start_of_cluster(s, *host_offset) == (entry & L2E_OFFSET_MASK)))
     {
+        int preallocated_nb_clusters;
+
+        if (offset_into_cluster(s, entry & L2E_OFFSET_MASK)) {
+            qcow2_signal_corruption(bs, true, -1, -1, "Preallocated zero "
+                                    "cluster offset %#llx unaligned (guest "
+                                    "offset: %#" PRIx64 ")",
+                                    entry & L2E_OFFSET_MASK, guest_offset);
+            ret = -EIO;
+            goto fail;
+        }
+
         /* Try to reuse preallocated zero clusters; contiguous normal clusters
          * would be fine, too, but count_cow_clusters() above has limited
          * nb_clusters already to a range of COW clusters */
-        int preallocated_nb_clusters =
+        preallocated_nb_clusters =
             count_contiguous_clusters(nb_clusters, s->cluster_size,
                                       &l2_table[l2_index], QCOW_OFLAG_COPIED);
         assert(preallocated_nb_clusters > 0);
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 56bdf1ee2e..49bc89df38 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -321,6 +321,22 @@ echo '--- Repairing ---'
 # because the image was already marked corrupt by that point
 _check_test_img -r all
 
+echo
+echo "=== Writing to an unaligned preallocated zero cluster ==="
+echo
+
+_make_test_img 64M
+
+# Allocate the L2 table
+$QEMU_IO -c "write 0 64k" -c "discard 0 64k" "$TEST_IMG" | _filter_qemu_io
+# Pretend there is a preallocated zero cluster somewhere inside the
+# image header
+poke_file "$TEST_IMG" "$l2_offset" "\x80\x00\x00\x00\x00\x00\x2a\x01"
+# Let's write to it!
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+
+# Can't repair this yet (TODO: We can just deallocate the cluster)
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index f013fe73c0..c583076808 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -307,4 +307,14 @@ The following inconsistencies were found and repaired:
 
 Double checking the fixed image now...
 No errors were found on the image.
+
+=== Writing to an unaligned preallocated zero cluster ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Preallocated zero cluster offset 0x2a00 unaligned (guest offset: 0); further corruption events will be suppressed
+write failed: Input/output error
 *** done
-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 3/5] block: Guard against NULL bs->drv

[Max Reitz]

We currently do not guard everywhere against a NULL bs->drv where we
should be doing so.  Most of the places fixed here just do not care
about that case at all.

Some care implicitly, e.g. through a prior function call to
bdrv_getlength() which would always fail for an ejected BDS.  Add an
assert there to make it more obvious.

Other places seem to care, but do so insufficiently: Freeing clusters in
a qcow2 image is an error-free operation, but it may leave the image in
an unusable state anyway.  Giving qcow2_free_clusters() an error code is
not really viable, it is much easier to note that bs->drv may be NULL
even after a successful driver call.  This concerns bdrv_co_flush(), and
the way the check is added to bdrv_co_pdiscard() (in every iteration
instead of only once).

Finally, some places employ at least an assert(bs->drv); somewhere, that
may be reasonable (such as in the reopen code), but in
bdrv_has_zero_init(), it is definitely not.  Returning 0 there in case
of an ejected BDS saves us much headache instead.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728660
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    | 19 ++++++++++++++++++-
 block/io.c                 | 36 ++++++++++++++++++++++++++++++++++++
 block/qapi.c               |  8 +++++++-
 block/replication.c        | 15 +++++++++++++++
 block/vvfat.c              |  2 +-
 tests/qemu-iotests/060     | 22 ++++++++++++++++++++++
 tests/qemu-iotests/060.out | 31 +++++++++++++++++++++++++++++++
 7 files changed, 130 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index cec4e9a8d6..dce20ed5e7 100644
--- a/block.c
+++ b/block.c
@@ -720,6 +720,10 @@ static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
 {
     BlockDriver *drv = bs->drv;
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     /* Do not attempt drv->bdrv_getlength() on scsi-generic devices */
     if (bdrv_is_sg(bs))
         return 0;
@@ -3431,6 +3435,10 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     BlockDriver *drv = bs->drv;
     int ret;
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     /* Backing file format doesn't make sense without a backing file */
     if (backing_fmt && !backing_file) {
         return -EINVAL;
@@ -3916,7 +3924,9 @@ int bdrv_has_zero_init_1(BlockDriverState *bs)
 
 int bdrv_has_zero_init(BlockDriverState *bs)
 {
-    assert(bs->drv);
+    if (!bs->drv) {
+        return 0;
+    }
 
     /* If BS is a copy on write image, it is initialized to
        the contents of the base image, which may not be zeroes.  */
@@ -4245,6 +4255,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
     BdrvChild *child, *parent;
     int ret;
 
+    if (!bs->drv) {
+        return -ENOMEDIUM;
+    }
+
     if (!setting_flag && bs->drv->bdrv_inactivate) {
         ret = bs->drv->bdrv_inactivate(bs);
         if (ret < 0) {
@@ -4780,6 +4794,9 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs,
 int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
                        BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
 {
+    if (!bs->drv) {
+        return -ENOMEDIUM;
+    }
     if (!bs->drv->bdrv_amend_options) {
         return -ENOTSUP;
     }
diff --git a/block/io.c b/block/io.c
index 3d5ef2cabe..4fdf93a014 100644
--- a/block/io.c
+++ b/block/io.c
@@ -853,6 +853,10 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
 
     assert(!(flags & ~BDRV_REQ_MASK));
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     if (drv->bdrv_co_preadv) {
         return drv->bdrv_co_preadv(bs, offset, bytes, qiov, flags);
     }
@@ -894,6 +898,10 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
 
     assert(!(flags & ~BDRV_REQ_MASK));
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     if (drv->bdrv_co_pwritev) {
         ret = drv->bdrv_co_pwritev(bs, offset, bytes, qiov,
                                    flags & bs->supported_write_flags);
@@ -945,6 +953,10 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 {
     BlockDriver *drv = bs->drv;
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     if (!drv->bdrv_co_pwritev_compressed) {
         return -ENOTSUP;
     }
@@ -975,6 +987,10 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
                                     BDRV_REQUEST_MAX_BYTES);
     unsigned int progress = 0;
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     /* FIXME We cannot require callers to have write permissions when all they
      * are doing is a read request. If we did things right, write permissions
      * would be obtained anyway, but internally by the copy-on-read code. As
@@ -1291,6 +1307,10 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
                         bs->bl.request_alignment);
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     assert(alignment % bs->bl.request_alignment == 0);
     head = offset % alignment;
     tail = (offset + bytes) % alignment;
@@ -1397,6 +1417,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     uint64_t bytes_remaining = bytes;
     int max_transfer;
 
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
     if (bdrv_has_readonly_bitmaps(bs)) {
         return -EPERM;
     }
@@ -1863,6 +1887,8 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
         bytes = n;
     }
 
+    /* Must be non-NULL or bdrv_getlength() would have failed */
+    assert(bs->drv);
     if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = bytes;
         ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
@@ -2373,6 +2399,12 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
+    if (!bs->drv) {
+        /* bs->drv->bdrv_co_flush() might have ejected the BDS
+         * (even in case of apparent success) */
+        ret = -ENOMEDIUM;
+        goto out;
+    }
     if (bs->drv->bdrv_co_flush_to_disk) {
         ret = bs->drv->bdrv_co_flush_to_disk(bs);
     } else if (bs->drv->bdrv_aio_flush) {
@@ -2542,6 +2574,10 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
             num = max_pdiscard;
         }
 
+        if (!bs->drv) {
+            ret = -ENOMEDIUM;
+            goto out;
+        }
         if (bs->drv->bdrv_co_pdiscard) {
             ret = bs->drv->bdrv_co_pdiscard(bs, offset, num);
         } else {
diff --git a/block/qapi.c b/block/qapi.c
index 7fa2437923..fc10f0a565 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -39,8 +39,14 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 {
     ImageInfo **p_image_info;
     BlockDriverState *bs0;
-    BlockDeviceInfo *info = g_malloc0(sizeof(*info));
+    BlockDeviceInfo *info;
 
+    if (!bs->drv) {
+        error_setg(errp, "Block device %s is ejected", bs->node_name);
+        return NULL;
+    }
+
+    info = g_malloc0(sizeof(*info));
     info->file                   = g_strdup(bs->filename);
     info->ro                     = bs->read_only;
     info->drv                    = g_strdup(bs->drv->format_name);
diff --git a/block/replication.c b/block/replication.c
index 1c95d673ff..e41e293d2b 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -342,12 +342,24 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }
 
+    if (!s->active_disk->bs->drv) {
+        error_setg(errp, "Active disk %s is ejected",
+                   s->active_disk->bs->node_name);
+        return;
+    }
+
     ret = s->active_disk->bs->drv->bdrv_make_empty(s->active_disk->bs);
     if (ret < 0) {
         error_setg(errp, "Cannot make active disk empty");
         return;
     }
 
+    if (!s->hidden_disk->bs->drv) {
+        error_setg(errp, "Hidden disk %s is ejected",
+                   s->hidden_disk->bs->node_name);
+        return;
+    }
+
     ret = s->hidden_disk->bs->drv->bdrv_make_empty(s->hidden_disk->bs);
     if (ret < 0) {
         error_setg(errp, "Cannot make hidden disk empty");
@@ -511,6 +523,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
             return;
         }
 
+        /* Must be true, or the bdrv_getlength() calls would have failed */
+        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
+
         if (!s->active_disk->bs->drv->bdrv_make_empty ||
             !s->hidden_disk->bs->drv->bdrv_make_empty) {
             error_setg(errp,
diff --git a/block/vvfat.c b/block/vvfat.c
index 0841cc42fc..a690595f2c 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2947,7 +2947,7 @@ static int do_commit(BDRVVVFATState* s)
         return ret;
     }
 
-    if (s->qcow->bs->drv->bdrv_make_empty) {
+    if (s->qcow->bs->drv && s->qcow->bs->drv->bdrv_make_empty) {
         s->qcow->bs->drv->bdrv_make_empty(s->qcow->bs);
     }
 
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 49bc89df38..44141f6243 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -337,6 +337,28 @@ $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 
 # Can't repair this yet (TODO: We can just deallocate the cluster)
 
+echo
+echo '=== Discarding with an unaligned refblock ==='
+echo
+
+_make_test_img 64M
+
+$QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io
+# Make our refblock unaligned
+poke_file "$TEST_IMG" "$(($rt_offset))" "\x00\x00\x00\x00\x00\x00\x2a\x00"
+# Now try to discard something that will be submitted as two requests
+# (main part + tail)
+$QEMU_IO -c "discard 0 65537" "$TEST_IMG"
+
+echo '--- Repairing ---'
+# Fails the first repair because the corruption prevents the check
+# function from double-checking
+# (Using -q for the first invocation, because otherwise the
+#  double-check error message appears above the summary for some
+#  reason -- so let's just hide the summary)
+_check_test_img -q -r all
+_check_test_img -r all
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index c583076808..07dfdcac99 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -317,4 +317,35 @@ discard 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preallocated zero cluster offset 0x2a00 unaligned (guest offset: 0); further corruption events will be suppressed
 write failed: Input/output error
+
+=== Discarding with an unaligned refblock ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Refblock offset 0x2a00 unaligned (reftable index: 0); further corruption events will be suppressed
+qcow2_free_clusters failed: Input/output error
+discard failed: No medium found
+--- Repairing ---
+ERROR refcount block 0 is not cluster aligned; refcount table entry corrupted
+qcow2: Marking image as corrupt: Refblock offset 0x2a00 unaligned (reftable index: 0); further corruption events will be suppressed
+Can't get refcount for cluster 0: Input/output error
+Can't get refcount for cluster 1: Input/output error
+Can't get refcount for cluster 2: Input/output error
+Can't get refcount for cluster 3: Input/output error
+Can't get refcount for cluster 4: Input/output error
+Can't get refcount for cluster 5: Input/output error
+Can't get refcount for cluster 6: Input/output error
+Rebuilding refcount structure
+Repairing cluster 1 refcount=1 reference=0
+qemu-img: Check failed: No medium found
+Leaked cluster 1 refcount=1 reference=0
+Repairing cluster 1 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+    1 leaked clusters
+    0 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
 *** done
-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Max Reitz]

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1728661
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.h              |  6 ------
 block/qcow2-refcount.c     | 26 +++++++++++++++++++++++++-
 tests/qemu-iotests/060     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 22 ++++++++++++++++++++++
 4 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 782a206ecb..6f0ff15dd0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -527,12 +527,6 @@ uint32_t offset_to_reftable_index(BDRVQcow2State *s, uint64_t offset)
     return offset >> (s->refcount_block_bits + s->cluster_bits);
 }
 
-static inline uint64_t get_refblock_offset(BDRVQcow2State *s, uint64_t offset)
-{
-    uint32_t index = offset_to_reftable_index(s, offset);
-    return s->refcount_table[index] & REFT_OFFSET_MASK;
-}
-
 /* qcow2.c functions */
 int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov,
                   int64_t sector_num, int nb_sectors);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 60b8eef3e8..3de1ab51ba 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -3077,16 +3077,40 @@ done:
     return ret;
 }
 
+static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
+{
+    BDRVQcow2State *s = bs->opaque;
+    uint32_t index = offset_to_reftable_index(s, offset);
+    int64_t covering_refblock_offset = 0;
+
+    if (index < s->refcount_table_size) {
+        covering_refblock_offset = s->refcount_table[index] & REFT_OFFSET_MASK;
+    }
+    if (!covering_refblock_offset) {
+        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " is "
+                                "not covered by the refcount structures",
+                                offset);
+        return -EIO;
+    }
+
+    return covering_refblock_offset;
+}
+
 static int qcow2_discard_refcount_block(BlockDriverState *bs,
                                         uint64_t discard_block_offs)
 {
     BDRVQcow2State *s = bs->opaque;
-    uint64_t refblock_offs = get_refblock_offset(s, discard_block_offs);
+    int64_t refblock_offs;
     uint64_t cluster_index = discard_block_offs >> s->cluster_bits;
     uint32_t block_index = cluster_index & (s->refcount_block_size - 1);
     void *refblock;
     int ret;
 
+    refblock_offs = get_refblock_offset(bs, discard_block_offs);
+    if (refblock_offs < 0) {
+        return refblock_offs;
+    }
+
     assert(discard_block_offs != 0);
 
     ret = qcow2_cache_get(bs, s->refcount_block_cache, refblock_offs,
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 44141f6243..c230696b3a 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -359,6 +359,52 @@ echo '--- Repairing ---'
 _check_test_img -q -r all
 _check_test_img -r all
 
+echo
+echo "=== Discarding an out-of-bounds refblock ==="
+echo
+
+_make_test_img 64M
+
+# Pretend there's a refblock really up high
+poke_file "$TEST_IMG" "$(($rt_offset+8))" "\x00\xff\xff\xff\x00\x00\x00\x00"
+# Let's try to shrink the qcow2 image so that the block driver tries
+# to discard that refblock (and see what happens!)
+$QEMU_IMG resize --shrink "$TEST_IMG" 32M
+
+echo '--- Checking and retrying ---'
+# Image should not be resized
+_img_info | grep 'virtual size'
+# But it should pass this check, because the "partial" resize has
+# already overwritten refblocks past the end
+_check_test_img -r all
+# So let's try again
+$QEMU_IMG resize --shrink "$TEST_IMG" 32M
+_img_info | grep 'virtual size'
+
+echo
+echo "=== Discarding a non-covered in-bounds refblock ==="
+echo
+
+IMGOPTS='refcount_bits=1' _make_test_img 64M
+
+# Pretend there's a refblock somewhere where there is no refblock to
+# cover it (but the covering refblock has a valid index in the
+# reftable)
+# Every refblock covers 65536 * 8 * 65536 = 32 GB, so we have to point
+# to 0x10_0000_0000 (64G) to point to the third refblock
+poke_file "$TEST_IMG" "$(($rt_offset+8))" "\x00\x00\x00\x10\x00\x00\x00\x00"
+$QEMU_IMG resize --shrink "$TEST_IMG" 32M
+
+echo '--- Checking and retrying ---'
+# Image should not be resized
+_img_info | grep 'virtual size'
+# But it should pass this check, because the "partial" resize has
+# already overwritten refblocks past the end
+_check_test_img -r all
+# So let's try again
+$QEMU_IMG resize --shrink "$TEST_IMG" 32M
+_img_info | grep 'virtual size'
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 07dfdcac99..358e54cdc9 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -348,4 +348,26 @@ The following inconsistencies were found and repaired:
 
 Double checking the fixed image now...
 No errors were found on the image.
+
+=== Discarding an out-of-bounds refblock ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Refblock at 0xffffff00000000 is not covered by the refcount structures; further corruption events will be suppressed
+qemu-img: Failed to discard unused refblocks: Input/output error
+--- Checking and retrying ---
+virtual size: 64M (67108864 bytes)
+No errors were found on the image.
+Image resized.
+virtual size: 32M (33554432 bytes)
+
+=== Discarding a non-covered in-bounds refblock ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Refblock at 0x1000000000 is not covered by the refcount structures; further corruption events will be suppressed
+qemu-img: Failed to discard unused refblocks: Input/output error
+--- Checking and retrying ---
+virtual size: 64M (67108864 bytes)
+No errors were found on the image.
+Image resized.
+virtual size: 32M (33554432 bytes)
 *** done
-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Max Reitz]

Instead of using an assertion, it is better to emit a corruption event
here.  Checking all offsets for correct alignment can be tedious and it
is easily possible to forget to do so.  qcow2_cache_do_get() is a
function every L2 and refblock access has to go through, so this is a
good central point to add such a check.

And for good measure, let us also add an assertion that the offset is
non-zero.  Making this a corruption event is not feasible, because a
zero offset usually means something special (such as the cluster is
unused), so all callers should be checking this anyway.  If they do not,
it is their fault, hence the assertion here.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cache.c        | 21 +++++++++++++++++++++
 tests/qemu-iotests/060     | 21 +++++++++++++++++++++
 tests/qemu-iotests/060.out | 29 +++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 75746a7f43..a5baaba0ff 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -62,6 +62,18 @@ static inline int qcow2_cache_get_table_idx(BlockDriverState *bs,
     return idx;
 }
 
+static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, Qcow2Cache *c)
+{
+    if (c == s->refcount_block_cache) {
+        return "refcount block";
+    } else if (c == s->l2_table_cache) {
+        return "L2 table";
+    } else {
+        /* Do not abort, because this is not critical */
+        return "unknown";
+    }
+}
+
 static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
                                       int i, int num_tables)
 {
@@ -314,9 +326,18 @@ static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c,
     uint64_t min_lru_counter = UINT64_MAX;
     int min_lru_index = -1;
 
+    assert(offset != 0);
+
     trace_qcow2_cache_get(qemu_coroutine_self(), c == s->l2_table_cache,
                           offset, read_from_disk);
 
+    if (offset_into_cluster(s, offset)) {
+        qcow2_signal_corruption(bs, true, -1, -1, "Cannot get entry from %s "
+                                "cache: Offset %#" PRIx64 " is unaligned",
+                                qcow2_cache_get_name(s, c), offset);
+        return -EIO;
+    }
+
     /* Check if the table is already cached */
     i = lookup_index = (offset / s->cluster_size * 4) % c->size;
     do {
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index c230696b3a..1eca09417b 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -405,6 +405,27 @@ _check_test_img -r all
 $QEMU_IMG resize --shrink "$TEST_IMG" 32M
 _img_info | grep 'virtual size'
 
+echo
+echo "=== Discarding a refblock covered by an unaligned refblock ==="
+echo
+
+IMGOPTS='refcount_bits=1' _make_test_img 64M
+
+# Same as above
+poke_file "$TEST_IMG" "$(($rt_offset+8))" "\x00\x00\x00\x10\x00\x00\x00\x00"
+# But now we actually "create" an unaligned third refblock
+poke_file "$TEST_IMG" "$(($rt_offset+16))" "\x00\x00\x00\x00\x00\x00\x02\x00"
+$QEMU_IMG resize --shrink "$TEST_IMG" 32M
+
+echo '--- Repairing ---'
+# Fails the first repair because the corruption prevents the check
+# function from double-checking
+# (Using -q for the first invocation, because otherwise the
+#  double-check error message appears above the summary for some
+#  reason -- so let's just hide the summary)
+_check_test_img -q -r all
+_check_test_img -r all
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 358e54cdc9..56f5eb15d8 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -370,4 +370,33 @@ virtual size: 64M (67108864 bytes)
 No errors were found on the image.
 Image resized.
 virtual size: 32M (33554432 bytes)
+
+=== Discarding a refblock covered by an unaligned refblock ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Cannot get entry from refcount block cache: Offset 0x200 is unaligned; further corruption events will be suppressed
+qemu-img: Failed to discard unused refblocks: Input/output error
+--- Repairing ---
+Repairing refcount block 1 is outside image
+ERROR refcount block 2 is not cluster aligned; refcount table entry corrupted
+qcow2: Marking image as corrupt: Refblock offset 0x200 unaligned (reftable index: 0x2); further corruption events will be suppressed
+Can't get refcount for cluster 1048576: Input/output error
+Rebuilding refcount structure
+Repairing cluster 1 refcount=1 reference=0
+Repairing cluster 2 refcount=1 reference=0
+Repairing cluster 1048576 refcount=1 reference=0
+qemu-img: Check failed: No medium found
+Leaked cluster 1 refcount=1 reference=0
+Leaked cluster 2 refcount=1 reference=0
+Leaked cluster 1048576 refcount=1 reference=0
+Repairing cluster 1 refcount=1 reference=0
+Repairing cluster 2 refcount=1 reference=0
+Repairing cluster 1048576 refcount=1 reference=0
+The following inconsistencies were found and repaired:
+
+    3 leaked clusters
+    0 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
 *** done
-- 
2.13.6

Re: [Qemu-devel] [PATCH for-2.11 0/5] qcow2: Fixes for corrupted images

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

On 2017-11-10 21:31, Max Reitz wrote:
> This series contains fixes for another batch of qcow2-related crashes
> reported on Launchpad by Nageswara (the first batch was
> http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00082.html by
> Berto).
> 
> Patch 4 fixes an out-of-bounds array access in memory which is not
> really a security issue for multiple reasons (really, at most you can
> read eight bytes from somewhere with an extremely high chance of
> crashing qemu and requiring the user to invoke a block_resize shrinking
> the qcow2 image (and also reset some bit in the image from 1 to 0, but
> only if the overlap checks don't catch you)), but most importantly that
> code hasn't been in 2.10, so we're fine.
> 
> 
> Max Reitz (5):
>   qcow2: check_errors are fatal
>   qcow2: Unaligned zero cluster in handle_alloc()
>   block: Guard against NULL bs->drv
>   qcow2: Add bounds check to get_refblock_offset()
>   qcow2: Refuse to get unaligned offsets from cache
> 
>  block/qcow2.h              |   6 ---
>  block.c                    |  19 ++++++-
>  block/io.c                 |  36 +++++++++++++
>  block/qapi.c               |   8 ++-
>  block/qcow2-cache.c        |  21 ++++++++
>  block/qcow2-cluster.c      |  13 ++++-
>  block/qcow2-refcount.c     |  26 +++++++++-
>  block/qcow2.c              |   5 +-
>  block/replication.c        |  15 ++++++
>  block/vvfat.c              |   2 +-
>  tests/qemu-iotests/060     | 125 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/060.out | 115 +++++++++++++++++++++++++++++++++++++++++
>  12 files changed, 379 insertions(+), 12 deletions(-)

I see that Patchew complains, so let's try:

Based-on: <cover.1510143008.git.berto@igalia.com>

And let's see whether it can handle the recursive dependency...

(Letting Patchew base something on git branches would be nice O:-))


Also note my follow-up patch "qcow2: Repair unaligned preallocated zero
clusters" which fixes the TODO added in patch 2.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 1/5] qcow2: check_errors are fatal

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On 11/10/2017 02:31 PM, Max Reitz wrote:
> When trying to repair a dirty image, qcow2_check() may apparently
> succeed (no really fatal error occurred that would prevent the check
> from continuing), but if check_errors in the result object is non-zero,
> we cannot trust the image to be usable.
> 
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728639
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c              |  5 ++++-
>  tests/qemu-iotests/060     | 20 ++++++++++++++++++++
>  tests/qemu-iotests/060.out | 23 +++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 2/5] qcow2: Unaligned zero cluster in handle_alloc()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On 11/10/2017 02:31 PM, Max Reitz wrote:
> We should check whether the cluster offset we are about to use is
> actually valid; that is, whether it is aligned to cluster boundaries.
> 
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728643
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728657
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c      | 13 ++++++++++++-
>  tests/qemu-iotests/060     | 16 ++++++++++++++++
>  tests/qemu-iotests/060.out | 10 ++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 3/5] block: Guard against NULL bs->drv

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On 11/10/2017 02:31 PM, Max Reitz wrote:
> We currently do not guard everywhere against a NULL bs->drv where we
> should be doing so.  Most of the places fixed here just do not care
> about that case at all.
> 
> Some care implicitly, e.g. through a prior function call to
> bdrv_getlength() which would always fail for an ejected BDS.  Add an
> assert there to make it more obvious.
> 
> Other places seem to care, but do so insufficiently: Freeing clusters in
> a qcow2 image is an error-free operation, but it may leave the image in
> an unusable state anyway.  Giving qcow2_free_clusters() an error code is
> not really viable, it is much easier to note that bs->drv may be NULL
> even after a successful driver call.  This concerns bdrv_co_flush(), and
> the way the check is added to bdrv_co_pdiscard() (in every iteration
> instead of only once).
> 
> Finally, some places employ at least an assert(bs->drv); somewhere, that
> may be reasonable (such as in the reopen code), but in
> bdrv_has_zero_init(), it is definitely not.  Returning 0 there in case
> of an ejected BDS saves us much headache instead.
> 
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728660
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> +++ b/block/replication.c

>  
> +    if (!s->hidden_disk->bs->drv) {
> +        error_setg(errp, "Hidden disk %s is ejected",
> +                   s->hidden_disk->bs->node_name);
> +        return;
> +    }

How would the hidden disk ever be ejected?  Could this be an assert instead?

But what you have is equally safe, so
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

On 11/10/2017 02:31 PM, Max Reitz wrote:
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728661
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.h              |  6 ------
>  block/qcow2-refcount.c     | 26 +++++++++++++++++++++++++-
>  tests/qemu-iotests/060     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/060.out | 22 ++++++++++++++++++++++
>  4 files changed, 93 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

On 11/10/2017 02:31 PM, Max Reitz wrote:
> Instead of using an assertion, it is better to emit a corruption event
> here.  Checking all offsets for correct alignment can be tedious and it
> is easily possible to forget to do so.  qcow2_cache_do_get() is a
> function every L2 and refblock access has to go through, so this is a
> good central point to add such a check.
> 
> And for good measure, let us also add an assertion that the offset is
> non-zero.  Making this a corruption event is not feasible, because a
> zero offset usually means something special (such as the cluster is
> unused), so all callers should be checking this anyway.  If they do not,
> it is their fault, hence the assertion here.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cache.c        | 21 +++++++++++++++++++++
>  tests/qemu-iotests/060     | 21 +++++++++++++++++++++
>  tests/qemu-iotests/060.out | 29 +++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+)
> 

> +--- Repairing ---
> +Repairing refcount block 1 is outside image
> +ERROR refcount block 2 is not cluster aligned; refcount table entry corrupted
> +qcow2: Marking image as corrupt: Refblock offset 0x200 unaligned (reftable index: 0x2); further corruption events will be suppressed
> +Can't get refcount for cluster 1048576: Input/output error

Trying to understand this: we have a double corruption, because we
encountered a refblock that points outside of the image, but fixing the
refblock in turn encounters a second refblock that points within the
image but to an unaligned area.

Of course, you should never encounter these bad refblocks in normal
usage, but when it comes to dealing with untrusted images, being robust
is always worth it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

On 2017-11-10 22:54, Eric Blake wrote:
> On 11/10/2017 02:31 PM, Max Reitz wrote:
>> Instead of using an assertion, it is better to emit a corruption event
>> here.  Checking all offsets for correct alignment can be tedious and it
>> is easily possible to forget to do so.  qcow2_cache_do_get() is a
>> function every L2 and refblock access has to go through, so this is a
>> good central point to add such a check.
>>
>> And for good measure, let us also add an assertion that the offset is
>> non-zero.  Making this a corruption event is not feasible, because a
>> zero offset usually means something special (such as the cluster is
>> unused), so all callers should be checking this anyway.  If they do not,
>> it is their fault, hence the assertion here.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2-cache.c        | 21 +++++++++++++++++++++
>>  tests/qemu-iotests/060     | 21 +++++++++++++++++++++
>>  tests/qemu-iotests/060.out | 29 +++++++++++++++++++++++++++++
>>  3 files changed, 71 insertions(+)
>>
> 
>> +--- Repairing ---
>> +Repairing refcount block 1 is outside image
>> +ERROR refcount block 2 is not cluster aligned; refcount table entry corrupted
>> +qcow2: Marking image as corrupt: Refblock offset 0x200 unaligned (reftable index: 0x2); further corruption events will be suppressed
>> +Can't get refcount for cluster 1048576: Input/output error
> 
> Trying to understand this: we have a double corruption, because we
> encountered a refblock that points outside of the image, but fixing the
> refblock in turn encounters a second refblock that points within the
> image but to an unaligned area.

No, it's the very same.  As far as I've seen it, the repair function
tries to fix the "refblock is outside image" error by resizing the image
so the refblock is inside the image.  However, the subsequent
bdrv_truncate() detects the alignment corruption, too, and thus marks
the image corrupt.

The check function itself never marks the image corrupt because it's
doing its best to fix it. :-)

(And the only point in marking an image corrupt is to force the user to
repair it.)

And that's also the reason why we need to invoke the repair twice: On
the first run the check function notices that the image is so broken we
need to create new refcount structures, so it does that.  But it cannot
free the old structures (or something) because bs->drv == NULL by now.
And then it cannot be run a second time because !bs->drv.

So you need to manually invoke it a second time, and then it goes over
the newly created refcount structures which are then fixed up.

> Of course, you should never encounter these bad refblocks in normal
> usage, but when it comes to dealing with untrusted images, being robust
> is always worth it.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On 11/10/2017 04:00 PM, Max Reitz wrote:
>> Trying to understand this: we have a double corruption, because we
>> encountered a refblock that points outside of the image, but fixing the
>> refblock in turn encounters a second refblock that points within the
>> image but to an unaligned area.
> 
> No, it's the very same.  As far as I've seen it, the repair function
> tries to fix the "refblock is outside image" error by resizing the image
> so the refblock is inside the image.  However, the subsequent
> bdrv_truncate() detects the alignment corruption, too, and thus marks
> the image corrupt.

Is resizing the image to be larger always a wise thing compared to just
rebuilding the refcount?  If I stick a large enough out-of-image value
in the table, can I cause a denial-of-service by making qemu try to
allocate petabytes of storage just to bring it into range?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On 2017-11-10 23:15, Eric Blake wrote:
> On 11/10/2017 04:00 PM, Max Reitz wrote:
>>> Trying to understand this: we have a double corruption, because we
>>> encountered a refblock that points outside of the image, but fixing the
>>> refblock in turn encounters a second refblock that points within the
>>> image but to an unaligned area.
>>
>> No, it's the very same.  As far as I've seen it, the repair function
>> tries to fix the "refblock is outside image" error by resizing the image
>> so the refblock is inside the image.  However, the subsequent
>> bdrv_truncate() detects the alignment corruption, too, and thus marks
>> the image corrupt.
> 
> Is resizing the image to be larger always a wise thing compared to just
> rebuilding the refcount?  If I stick a large enough out-of-image value
> in the table, can I cause a denial-of-service by making qemu try to
> allocate petabytes of storage just to bring it into range?

But it's just a qcow2 resize (with no preallocation), so nothing will be
allocated.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 2/5] qcow2: Unaligned zero cluster in handle_alloc()

[Alberto Garcia]

On Fri 10 Nov 2017 09:31:08 PM CET, Max Reitz wrote:
> We should check whether the cluster offset we are about to use is
> actually valid; that is, whether it is aligned to cluster boundaries.
>
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728643
> Buglink: https://bugs.launchpad.net/qemu/+bug/1728657
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Alberto Garcia]

On Fri 10 Nov 2017 09:31:10 PM CET, Max Reitz wrote:
> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    uint32_t index = offset_to_reftable_index(s, offset);
> +    int64_t covering_refblock_offset = 0;
> +
> +    if (index < s->refcount_table_size) {
> +        covering_refblock_offset = s->refcount_table[index] & REFT_OFFSET_MASK;
> +    }
> +    if (!covering_refblock_offset) {
> +        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " is "
> +                                "not covered by the refcount structures",
> +                                offset);
> +        return -EIO;
> +    }
> +
> +    return covering_refblock_offset;
> +}

Isn't it simpler to do something like this instead?

   if (index >= s->refcount_table_size) {
       qcow2_signal_corruption(...);
       return -EIO;
   }
   return s->refcount_table[index] & REFT_OFFSET_MASK;

Berto

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Alberto Garcia]

On Fri 10 Nov 2017 09:31:11 PM CET, Max Reitz wrote:
> +static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, Qcow2Cache *c)
> +{
> +    if (c == s->refcount_block_cache) {
> +        return "refcount block";
> +    } else if (c == s->l2_table_cache) {
> +        return "L2 table";
> +    } else {
> +        /* Do not abort, because this is not critical */
> +        return "unknown";
> +    }
> +}

Why is an unknown cache not critical?

Berto

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On 2017-11-14 16:06, Alberto Garcia wrote:
> On Fri 10 Nov 2017 09:31:11 PM CET, Max Reitz wrote:
>> +static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, Qcow2Cache *c)
>> +{
>> +    if (c == s->refcount_block_cache) {
>> +        return "refcount block";
>> +    } else if (c == s->l2_table_cache) {
>> +        return "L2 table";
>> +    } else {
>> +        /* Do not abort, because this is not critical */
>> +        return "unknown";
>> +    }
>> +}
> 
> Why is an unknown cache not critical?

Because this is debugging information.  I know others disagree with my
opinion that I'd rather not abort qemu just because someone forgot to
add a 'return "foo";' here when adding a new cache, but that's my
opinion so I wanted to at least be told by someone that we should abort
here before doing it.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

On 2017-11-14 16:02, Alberto Garcia wrote:
> On Fri 10 Nov 2017 09:31:10 PM CET, Max Reitz wrote:
>> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    uint32_t index = offset_to_reftable_index(s, offset);
>> +    int64_t covering_refblock_offset = 0;
>> +
>> +    if (index < s->refcount_table_size) {
>> +        covering_refblock_offset = s->refcount_table[index] & REFT_OFFSET_MASK;
>> +    }
>> +    if (!covering_refblock_offset) {
>> +        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " is "
>> +                                "not covered by the refcount structures",
>> +                                offset);
>> +        return -EIO;
>> +    }
>> +
>> +    return covering_refblock_offset;
>> +}
> 
> Isn't it simpler to do something like this instead?
> 
>    if (index >= s->refcount_table_size) {
>        qcow2_signal_corruption(...);
>        return -EIO;
>    }
>    return s->refcount_table[index] & REFT_OFFSET_MASK;

But that doesn't cover the case were s->refcount_table[index] &
REFT_OFFSET_MASK is 0.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 5/5] qcow2: Refuse to get unaligned offsets from cache

[Alberto Garcia]

On Tue 14 Nov 2017 04:09:16 PM CET, Max Reitz wrote:
>>> +static inline const char *qcow2_cache_get_name(BDRVQcow2State *s, Qcow2Cache *c)
>>> +{
>>> +    if (c == s->refcount_block_cache) {
>>> +        return "refcount block";
>>> +    } else if (c == s->l2_table_cache) {
>>> +        return "L2 table";
>>> +    } else {
>>> +        /* Do not abort, because this is not critical */
>>> +        return "unknown";
>>> +    }
>>> +}
>> 
>> Why is an unknown cache not critical?
>
> Because this is debugging information.

Ah, right, this is only used for qcow2_signal_corruption().

> I know others disagree with my opinion that I'd rather not abort qemu
> just because someone forgot to add a 'return "foo";' here when adding
> a new cache, but that's my opinion so I wanted to at least be told by
> someone that we should abort here before doing it.

I just checked the code and at least qcow2_cache_entry_flush() assumes
that there can be other caches, so this problem is not new.

Perhaps we should rethink this in the future and add a stronger
assertion somewhere else instead of having dead code, but for now this
is OK I guess.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH for-2.11 3/5] block: Guard against NULL bs->drv

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

On 2017-11-10 22:46, Eric Blake wrote:
> On 11/10/2017 02:31 PM, Max Reitz wrote:
>> We currently do not guard everywhere against a NULL bs->drv where we
>> should be doing so.  Most of the places fixed here just do not care
>> about that case at all.
>>
>> Some care implicitly, e.g. through a prior function call to
>> bdrv_getlength() which would always fail for an ejected BDS.  Add an
>> assert there to make it more obvious.
>>
>> Other places seem to care, but do so insufficiently: Freeing clusters in
>> a qcow2 image is an error-free operation, but it may leave the image in
>> an unusable state anyway.  Giving qcow2_free_clusters() an error code is
>> not really viable, it is much easier to note that bs->drv may be NULL
>> even after a successful driver call.  This concerns bdrv_co_flush(), and
>> the way the check is added to bdrv_co_pdiscard() (in every iteration
>> instead of only once).
>>
>> Finally, some places employ at least an assert(bs->drv); somewhere, that
>> may be reasonable (such as in the reopen code), but in
>> bdrv_has_zero_init(), it is definitely not.  Returning 0 there in case
>> of an ejected BDS saves us much headache instead.
>>
>> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1728660
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
>> +++ b/block/replication.c
> 
>>  
>> +    if (!s->hidden_disk->bs->drv) {
>> +        error_setg(errp, "Hidden disk %s is ejected",
>> +                   s->hidden_disk->bs->node_name);
>> +        return;
>> +    }
> 
> How would the hidden disk ever be ejected?  Could this be an assert instead?

Maybe? :-)

Isn't the hidden disk usually a qcow2 file?  As such I guess there can
be corruptions in it that make the qcow2 driver eject it (even though
qemu isn't writing to it).

Max

> But what you have is equally safe, so
> Reviewed-by: Eric Blake <eblake@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Alberto Garcia]

On Tue 14 Nov 2017 04:27:56 PM CET, Max Reitz wrote:
>>> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    uint32_t index = offset_to_reftable_index(s, offset);
>>> +    int64_t covering_refblock_offset = 0;
>>> +
>>> +    if (index < s->refcount_table_size) {
>>> +        covering_refblock_offset = s->refcount_table[index] & REFT_OFFSET_MASK;
>>> +    }
>>> +    if (!covering_refblock_offset) {
>>> +        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " is "
>>> +                                "not covered by the refcount structures",
>>> +                                offset);
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    return covering_refblock_offset;
>>> +}
>> 
>> Isn't it simpler to do something like this instead?
>> 
>>    if (index >= s->refcount_table_size) {
>>        qcow2_signal_corruption(...);
>>        return -EIO;
>>    }
>>    return s->refcount_table[index] & REFT_OFFSET_MASK;
>
> But that doesn't cover the case were s->refcount_table[index] &
> REFT_OFFSET_MASK is 0.

Ah, you're right. That would be detected by the qcow2_cache_get() call
though, but it's fine to do the check here as well.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

On 2017-11-14 16:38, Alberto Garcia wrote:
> On Tue 14 Nov 2017 04:27:56 PM CET, Max Reitz wrote:
>>>> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
>>>> +{
>>>> +    BDRVQcow2State *s = bs->opaque;
>>>> +    uint32_t index = offset_to_reftable_index(s, offset);
>>>> +    int64_t covering_refblock_offset = 0;
>>>> +
>>>> +    if (index < s->refcount_table_size) {
>>>> +        covering_refblock_offset = s->refcount_table[index] & REFT_OFFSET_MASK;
>>>> +    }
>>>> +    if (!covering_refblock_offset) {
>>>> +        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 " is "
>>>> +                                "not covered by the refcount structures",
>>>> +                                offset);
>>>> +        return -EIO;
>>>> +    }
>>>> +
>>>> +    return covering_refblock_offset;
>>>> +}
>>>
>>> Isn't it simpler to do something like this instead?
>>>
>>>    if (index >= s->refcount_table_size) {
>>>        qcow2_signal_corruption(...);
>>>        return -EIO;
>>>    }
>>>    return s->refcount_table[index] & REFT_OFFSET_MASK;
>>
>> But that doesn't cover the case were s->refcount_table[index] &
>> REFT_OFFSET_MASK is 0.
> 
> Ah, you're right. That would be detected by the qcow2_cache_get() call
> though, but it's fine to do the check here as well.

After patch 5, yes, but it would lead to a failed assertion.

Before this series, there is no protection in place; the cache will
happily serve you any empty entry (because offset 0 is used for empty
entries) and pretend it's the correct block.

Only when you try to dirty it is when you run into problems (because
then you'll get an assertion failure).

Max

> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 0/5] qcow2: Fixes for corrupted images

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

On 2017-11-10 21:31, Max Reitz wrote:
> This series contains fixes for another batch of qcow2-related crashes
> reported on Launchpad by Nageswara (the first batch was
> http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00082.html by
> Berto).
> 
> Patch 4 fixes an out-of-bounds array access in memory which is not
> really a security issue for multiple reasons (really, at most you can
> read eight bytes from somewhere with an extremely high chance of
> crashing qemu and requiring the user to invoke a block_resize shrinking
> the qcow2 image (and also reset some bit in the image from 1 to 0, but
> only if the overlap checks don't catch you)), but most importantly that
> code hasn't been in 2.10, so we're fine.
> 
> 
> Max Reitz (5):
>   qcow2: check_errors are fatal
>   qcow2: Unaligned zero cluster in handle_alloc()
>   block: Guard against NULL bs->drv
>   qcow2: Add bounds check to get_refblock_offset()
>   qcow2: Refuse to get unaligned offsets from cache
> 
>  block/qcow2.h              |   6 ---
>  block.c                    |  19 ++++++-
>  block/io.c                 |  36 +++++++++++++
>  block/qapi.c               |   8 ++-
>  block/qcow2-cache.c        |  21 ++++++++
>  block/qcow2-cluster.c      |  13 ++++-
>  block/qcow2-refcount.c     |  26 +++++++++-
>  block/qcow2.c              |   5 +-
>  block/replication.c        |  15 ++++++
>  block/vvfat.c              |   2 +-
>  tests/qemu-iotests/060     | 125 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/060.out | 115 +++++++++++++++++++++++++++++++++++++++++
>  12 files changed, 379 insertions(+), 12 deletions(-)

Applied to my block tree:

https://github.com/XanClic/qemu/commits/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]