Misc qcow2 corruption checks

The crypto header is initialized only when QEMU is creating a new image, so there's no chance of this happening on a corrupted image.

If QEMU is really trying to allocate the header overlapping other existing metadata sections then this is a serious bug in QEMU itself so let's add an assertion.

Signed-off-by Alberto Garcia <[email protected]>

---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

If the refcount data is corrupted then we can end up trying to allocate a new compressed cluster at offset 0 in the image, triggering an assertion in qcow2_alloc_bytes() that would crash QEMU:

qcow2_alloc_bytes: Assertion `offset' failed.

This patch adds an explicit check for this scenario and a new test case.

Signed-off-by Alberto Garcia <[email protected]>

---
 block/qcow2-refcount.c     |  8 +++++++-
 tests/qemu-iotests/060     | 10 ++++++++++
 tests/qemu-iotests/060.out |  8 ++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

block/qcow2-refcount.c +7 -0

index 9059996c4b..7eaac11429 100644
@@ -1082,6 +1082,13 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
                 return new_cluster;
             }
 
+            if (new_cluster == 0) {
+                qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+                                        "allocation of compressed cluster "
+                                        "at offset 0");
+                return -EIO;
+            }
+
             if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) {
                 offset = new_cluster;
                 free_in_cluster = s->cluster_size;

block/qcow2-refcount.c +0 -0

         ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER);
         if (ret < 0) {
             offset = 0;

tests/qemu-iotests/060 +10 -0

index 40f85cc216..c3bce27b33 100755
@@ -260,6 +260,16 @@ _make_test_img 64M
 poke_file "$TEST_IMG" "$rb_offset"        "\x00\x00\x00\x00\x00\x00\x00\x00"
 $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing empty refcount block with compressed write ==="
+echo
+_make_test_img 64M
+$QEMU_IO -c "write 64k 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$rb_offset"        "\x00\x00\x00\x00\x00\x00\x00\x00"
+# The previous write already allocated an L2 table, so now this new
+# write will try to allocate a compressed data cluster at offset 0.
+$QEMU_IO -c "write -c 0k 64k" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full

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

index 5b8b518486..cf8790ff57 100644
@@ -195,4 +195,12 @@ write failed: Input/output error
 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 empty refcount block with compressed write ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Preventing invalid allocation of compressed cluster at offset 0; further corruption events will be suppressed
+write failed: Input/output error
 *** done

This patch adds a simple iotest in which we try to write to an image with the refcount table offset set to 0.

This scenario was already handled by the existing consistency checks, but we add an explicit test case for completeness.

Signed-off-by Alberto Garcia <[email protected]>

---
 tests/qemu-iotests/060     | 7 +++++++
 tests/qemu-iotests/060.out | 6 ++++++
 2 files changed, 13 insertions(+)

This series contains a few checks that prevent QEMU from crashing under some scenarios with corrupted qcow2 images.

The first patch solves the crash reported here:

https://bugs.launchpad.net/qemu/+bug/1728615

And the others solve similar crashes that I detected in the process of fixing this one.

Regards,

Berto

v2:
- Use goto fail in the l2_allocate() check [Max]
- Add check and test case for allocation of compressed clusters [Max]
- Add test case for header.refcount_table_offset == 0
- Add overlap checks to qcow2_crypto_hdr_init_func() [Max]

v1: https://lists.gnu.org/archive/html/qemu-block/2017-11/msg00010.html
- Initial version

Output of backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[----] [--] 'qcow2: Prevent allocating refcount blocks at offset 0' 002/7:[0003] [FC] 'qcow2: Prevent allocating L2 tables at offset 0' 003/7:[down] 'qcow2: Prevent allocating compressed clusters at offset 0' 004/7:[----] [-C] 'qcow2: Don't open images with header.refcount_table_clusters == 0' 005/7:[down] 'qcow2: Add iotest for an image with header.refcount_table_offset == 0' 006/7:[----] [--] 'qcow2: Add iotest for an empty refcount table' 007/7:[down] 'qcow2: Assert that the crypto header does not overlap other metadata'

Alberto Garcia (7):
qcow2: Prevent allocating refcount blocks at offset 0
qcow2: Prevent allocating L2 tables at offset 0
qcow2: Prevent allocating compressed clusters at offset 0
qcow2: Don't open images with header.refcount_table_clusters == 0
qcow2: Add iotest for an image with header.refcount_table_offset == 0
qcow2: Add iotest for an empty refcount table
qcow2: Assert that the crypto header does not overlap other metadata

 block/qcow2-cluster.c      |  8 ++++++++
 block/qcow2-refcount.c     | 15 +++++++++++++-
 block/qcow2.c              |  7 +++++++
 tests/qemu-iotests/060     | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 39 ++++++++++++++++++++++++++++++++++++
 5 files changed, 117 insertions(+), 1 deletion(-)

Quoted 14 lines

block/qcow2-refcount.c +7 -0

+            if (new_cluster == 0) {
+                qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+                                        "allocation of compressed cluster "
+                                        "at offset 0");
+                return -EIO;
+            }
+
             if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) {
                 offset = new_cluster;
                 free_in_cluster = s->cluster_size;

@@ -1090,7 +1097,6 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
             }
         }
 

block/qcow2-refcount.c int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) · line 1093

Quoted 1 line

block/qcow2-refcount.c +0 -1

-        assert(offset);

I don't think this assert() was meant as a protection against offset being 0. :-)

Quoted 4 lines

Quoted 1 line

- assert(offset);

I don't think this assert() was meant as a protection against offset being 0. :-)

After the new check offset is now guaranteed to be 0, so what's the point of keeping the assert() ?

Quoted 7 lines · Alberto Garcia

Quoted 4 lines

Quoted 1 line

- assert(offset);

I don't think this assert() was meant as a protection against offset being 0. :-)

After the new check offset is now guaranteed to be 0, so what's the point of keeping the assert() ?

I meant "guaranteed _not_ to be 0" :-)

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

- assert(offset);

I don't think this assert() was meant as a protection against offset being 0. :-)

After the new check offset is now guaranteed to be 0, so what's the point of keeping the assert() ?

Quoted 1 line

I meant "guaranteed _not_ to be 0" :-)

That is the point of an assert.

An assert should not guard against something that can occur. It should express that something will always be true (in this case that the offset is guaranteed not to be 0).

qcow2_do_open() is checking that header.refcount_table_clusters is not too large, but it doesn't check that it's greater than zero. Apart from the fact that an image like that is obviously corrupted, trying to use it crashes QEMU since we end up with a null s->refcount_table after qcow2_refcount_init().

These images can however be repaired, so allow opening them if the BDRV_O_CHECK flag is set.

Signed-off-by Alberto Garcia <[email protected]>

Reviewed-by Max Reitz <[email protected]>

tests/qemu-iotests/060 poke_file "$TEST_IMG" "$rb_offset" "\x00\x00\x00\x00\x00\x00\x00\x00" · line 278

Quoted 6 lines

tests/qemu-iotests/060 +6 -0

+echo
+echo "=== Testing zero refcount table size ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "56"                "\x00\x00\x00\x00"
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt

In the commit message, you claim that the image can be repaired. Would it be worth actually testing the repair here?