qcow2: Fixes for corrupted images

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 <[email protected]>

Buglink https://bugs.launchpad.net/qemu/+bug/1728639

Signed-off-by Max Reitz <[email protected]>

---
 block/qcow2.c              |  5 ++++-
 tests/qemu-iotests/060     | 20 ++++++++++++++++++++
 tests/qemu-iotests/060.out | 23 +++++++++++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

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 <[email protected]>

Buglink https://bugs.launchpad.net/qemu/+bug/1728643

Buglink https://bugs.launchpad.net/qemu/+bug/1728657

Signed-off-by Max Reitz <[email protected]>

---
 block/qcow2-cluster.c      | 13 ++++++++++++-
 tests/qemu-iotests/060     | 16 ++++++++++++++++
 tests/qemu-iotests/060.out | 10 ++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

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 <[email protected]>

Buglink https://bugs.launchpad.net/qemu/+bug/1728643

Buglink https://bugs.launchpad.net/qemu/+bug/1728657

Signed-off-by Max Reitz <[email protected]>

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 <[email protected]>

Buglink https://bugs.launchpad.net/qemu/+bug/1728660

Signed-off-by Max Reitz <[email protected]>

---

+++ b/block/replication.c

Quoted 29 lines · 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 <[email protected]>

Buglink https://bugs.launchpad.net/qemu/+bug/1728660

Signed-off-by Max Reitz <[email protected]>

---

+++ b/block/replication.c

block/replication.c static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp) · line 361

Quoted 5 lines

block/replication.c +5 -0

+    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 <[email protected]>

Reported-by R. Nageswara Sastry <[email protected]>

Buglink https://bugs.launchpad.net/qemu/+bug/1728661

Signed-off-by Max Reitz <[email protected]>

---
 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(-)

block/qcow2-refcount.c +8 -0

+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;

Quoted 8 lines · Max Reitz

block/qcow2-refcount.c +8 -0

+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;

block/qcow2-refcount.c done: · line 3097

Quoted 10 lines

block/qcow2-refcount.c +10 -0

+    }
+    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;

Quoted 21 lines

Quoted 8 lines

block/qcow2-refcount.c +8 -0

+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;

block/qcow2-refcount.c done: · line 3097

Quoted 10 lines

block/qcow2-refcount.c +10 -0

+    }
+    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?

Quoted 5 lines

   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.

Quoted 27 lines · Max Reitz

Quoted 26 lines

Quoted 8 lines

block/qcow2-refcount.c +8 -0

+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;

block/qcow2-refcount.c done: · line 3097

Quoted 10 lines

block/qcow2-refcount.c +10 -0

+    }
+    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;

Quoted 2 lines

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 <[email protected]>

Berto

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 <[email protected]>

---
 block/qcow2-cache.c        | 21 +++++++++++++++++++++
 tests/qemu-iotests/060     | 21 +++++++++++++++++++++
 tests/qemu-iotests/060.out | 29 +++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

+--- Repairing ---

Quoted 19 lines · 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 <[email protected]>

---
 block/qcow2-cache.c        | 21 +++++++++++++++++++++
 tests/qemu-iotests/060     | 21 +++++++++++++++++++++
 tests/qemu-iotests/060.out | 29 +++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

tests/qemu-iotests/060.out virtual size: 64M (67108864 bytes) · line 383

Quoted 5 lines

+--- Repairing ---

tests/qemu-iotests/060.out +4 -0

+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 <[email protected]>

Quoted 4 lines

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.

Quoted 10 lines · Max Reitz

Quoted 4 lines

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?

block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 75

Quoted 11 lines · Max Reitz

block/qcow2-cache.c +11 -0

+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?

Quoted 5 lines

Quoted 5 lines

block/qcow2-cache.c +5 -0

+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) {

Quoted 8 lines

block/qcow2-cache.c static inline int qcow2_cache_get_table_idx(BlockDriverState *bs, · line 75

Quoted 6 lines

block/qcow2-cache.c +6 -0

+        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.

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(-)

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(-)