[Qemu-devel] [PATCH v2 0/7] Misc qcow2 corruption checks

[Qemu-devel] [PATCH v2 0/7] Misc qcow2 corruption checks

[Alberto Garcia]

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

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/7] qcow2: Prevent allocating refcount blocks at offset 0

[Alberto Garcia]

Each entry in the qcow2 cache contains an offset field indicating the
location of the data in the qcow2 image. If the offset is 0 then it
means that the entry contains no data and is available to be used when
needed.

Because of that it is not possible to store in the cache the first
cluster of the qcow2 image (offset = 0). This is not a problem because
that cluster always contains the qcow2 header and we're not using this
cache for that.

However, if the qcow2 image is corrupted it can happen that we try to
allocate a new refcount block at offset 0, triggering this assertion
and crashing QEMU:

  qcow2_cache_entry_mark_dirty: Assertion `c->entries[i].offset != 0' failed

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

This problem was originally reported here:

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

Reported-by: R.Nageswara Sastry <nasastry@in.ibm.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c     |  7 +++++++
 tests/qemu-iotests/060     | 11 +++++++++++
 tests/qemu-iotests/060.out |  8 ++++++++
 3 files changed, 26 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index aa3fd6cf17..9059996c4b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -367,6 +367,13 @@ static int alloc_refcount_block(BlockDriverState *bs,
         return new_block;
     }
 
+    /* If we're allocating the block at offset 0 then something is wrong */
+    if (new_block == 0) {
+        qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+                                "allocation of refcount block at offset 0");
+        return -EIO;
+    }
+
 #ifdef DEBUG_ALLOC2
     fprintf(stderr, "qcow2: Allocate refcount block %d for %" PRIx64
         " at %" PRIx64 "\n",
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 8e95c450eb..dead26aeaf 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -242,6 +242,17 @@ poke_file "$TEST_IMG" "$(($l2_offset+8))" "\x80\x00\x00\x00\x00\x06\x2a\x00"
 # Should emit two error messages
 $QEMU_IO -c "discard 0 64k" -c "read 64k 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing empty refcount table with valid L1 and L2 tables ==="
+echo
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$rt_offset"        "\x00\x00\x00\x00\x00\x00\x00\x00"
+# Since the first data cluster is already allocated this triggers an
+# allocation with an explicit offset (using qcow2_alloc_clusters_at())
+# causing a refcount block to be allocated at offset 0
+$QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 5ca3af491f..872719009c 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -181,4 +181,12 @@ qcow2: Marking image as corrupt: Cluster allocation offset 0x62a00 unaligned (L2
 discard 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read failed: Input/output error
+
+=== Testing empty refcount table with valid L1 and L2 tables ===
+
+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)
+qcow2: Marking image as corrupt: Preventing invalid allocation of refcount block at offset 0; further corruption events will be suppressed
+write failed: Input/output error
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/7] qcow2: Prevent allocating L2 tables at offset 0

[Alberto Garcia]

If the refcount data is corrupted then we can end up trying to
allocate a new L2 table at offset 0 in the image, triggering an
assertion in the qcow2 cache that would crash QEMU:

  qcow2_cache_entry_mark_dirty: Assertion `c->entries[i].offset != 0' failed

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

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c      | 8 ++++++++
 tests/qemu-iotests/060     | 7 +++++++
 tests/qemu-iotests/060.out | 6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fb10e26068..2e072ed155 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -278,6 +278,14 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
         goto fail;
     }
 
+    /* If we're allocating the table at offset 0 then something is wrong */
+    if (l2_offset == 0) {
+        qcow2_signal_corruption(bs, true, -1, -1, "Preventing invalid "
+                                "allocation of L2 table at offset 0");
+        ret = -EIO;
+        goto fail;
+    }
+
     ret = qcow2_cache_flush(bs, s->refcount_block_cache);
     if (ret < 0) {
         goto fail;
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index dead26aeaf..40f85cc216 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -253,6 +253,13 @@ poke_file "$TEST_IMG" "$rt_offset"        "\x00\x00\x00\x00\x00\x00\x00\x00"
 # causing a refcount block to be allocated at offset 0
 $QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing empty refcount block ==="
+echo
+_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
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 872719009c..5b8b518486 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -189,4 +189,10 @@ wrote 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: Preventing invalid allocation of refcount block at offset 0; further corruption events will be suppressed
 write failed: Input/output error
+
+=== Testing empty refcount block ===
+
+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
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/7] qcow2: Prevent allocating compressed clusters at offset 0

[Alberto Garcia]

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 <berto@igalia.com>
---
 block/qcow2-refcount.c     |  8 +++++++-
 tests/qemu-iotests/060     | 10 ++++++++++
 tests/qemu-iotests/060.out |  8 ++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9059996c4b..7eaac11429 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -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;
@@ -1090,7 +1097,6 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
             }
         }
 
-        assert(offset);
         ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER);
         if (ret < 0) {
             offset = 0;
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 40f85cc216..c3bce27b33 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -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
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 5b8b518486..cf8790ff57 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -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
-- 
2.11.0

[Qemu-devel] [PATCH v2 4/7] qcow2: Don't open images with header.refcount_table_clusters == 0

[Alberto Garcia]

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 <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c              | 6 ++++++
 tests/qemu-iotests/060     | 7 +++++++
 tests/qemu-iotests/060.out | 5 +++++
 3 files changed, 18 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 92cb9f9bfa..defc1fe49f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1280,6 +1280,12 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    if (header.refcount_table_clusters == 0 && !(flags & BDRV_O_CHECK)) {
+        error_setg(errp, "Image does not contain a reference count table");
+        ret = -EINVAL;
+        goto fail;
+    }
+
     ret = validate_table_offset(bs, s->refcount_table_offset,
                                 s->refcount_table_size, sizeof(uint64_t));
     if (ret < 0) {
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index c3bce27b33..656af50883 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -270,6 +270,13 @@ poke_file "$TEST_IMG" "$rb_offset"        "\x00\x00\x00\x00\x00\x00\x00\x00"
 # write will try to allocate a compressed data cluster at offset 0.
 $QEMU_IO -c "write -c 0k 64k" "$TEST_IMG" | _filter_qemu_io
 
+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
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index cf8790ff57..58456e8487 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -203,4 +203,9 @@ 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
+
+=== Testing zero refcount table size ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+can't open device TEST_DIR/t.IMGFMT: Image does not contain a reference count table
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v2 5/7] qcow2: Add iotest for an image with header.refcount_table_offset == 0

[Alberto Garcia]

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 <berto@igalia.com>
---
 tests/qemu-iotests/060     | 7 +++++++
 tests/qemu-iotests/060.out | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 656af50883..dc5a517673 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -277,6 +277,13 @@ _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
 
+echo
+echo "=== Testing incorrect refcount table offset ==="
+echo
+_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
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 58456e8487..98f314c16d 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -208,4 +208,10 @@ write failed: Input/output error
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 can't open device TEST_DIR/t.IMGFMT: Image does not contain a reference count table
+
+=== Testing incorrect refcount table offset ===
+
+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
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v2 6/7] qcow2: Add iotest for an empty refcount table

[Alberto Garcia]

This patch adds a simple iotest in which we try to write to an image
with an empty refcount table (i.e. with all entries 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 <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/060     | 7 +++++++
 tests/qemu-iotests/060.out | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index dc5a517673..66a8fa4aea 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -243,6 +243,13 @@ poke_file "$TEST_IMG" "$(($l2_offset+8))" "\x80\x00\x00\x00\x00\x06\x2a\x00"
 $QEMU_IO -c "discard 0 64k" -c "read 64k 64k" "$TEST_IMG" | _filter_qemu_io
 
 echo
+echo "=== Testing empty refcount table ==="
+echo
+_make_test_img 64M
+poke_file "$TEST_IMG" "$rt_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 table with valid L1 and L2 tables ==="
 echo
 _make_test_img 64M
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 98f314c16d..cfd78f87a9 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -182,6 +182,12 @@ discard 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read failed: Input/output error
 
+=== Testing empty refcount table ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with refcount table); further corruption events will be suppressed
+write failed: Input/output error
+
 === Testing empty refcount table with valid L1 and L2 tables ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.11.0

[Qemu-devel] [PATCH v2 7/7] qcow2: Assert that the crypto header does not overlap other metadata

[Alberto Garcia]

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 <berto@igalia.com>
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index defc1fe49f..b3d66a0e88 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -126,6 +126,7 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen,
     /* Zero fill remaining space in cluster so it has predictable
      * content in case of future spec changes */
     clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;
+    assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen) == 0);
     ret = bdrv_pwrite_zeroes(bs->file,
                              ret + headerlen,
                              clusterlen - headerlen, 0);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 7/7] qcow2: Assert that the crypto header does not overlap other metadata

[Daniel P. Berrange]

On Fri, Nov 03, 2017 at 04:18:56PM +0200, Alberto Garcia wrote:
> 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 <berto@igalia.com>
> ---
>  block/qcow2.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 3/7] qcow2: Prevent allocating compressed clusters at offset 0

[Max Reitz]

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

On 2017-11-03 15:18, Alberto Garcia wrote:
> 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 <berto@igalia.com>
> ---
>  block/qcow2-refcount.c     |  8 +++++++-
>  tests/qemu-iotests/060     | 10 ++++++++++
>  tests/qemu-iotests/060.out |  8 ++++++++
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 9059996c4b..7eaac11429 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -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;
> @@ -1090,7 +1097,6 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
>              }
>          }
>  
> -        assert(offset);

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

Reviewed-by: Max Reitz <mreitz@redhat.com>

if the assert() stays.

>          ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER);
>          if (ret < 0) {
>              offset = 0;
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 40f85cc216..c3bce27b33 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -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
> diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
> index 5b8b518486..cf8790ff57 100644
> --- a/tests/qemu-iotests/060.out
> +++ b/tests/qemu-iotests/060.out
> @@ -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
> 



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

Re: [Qemu-devel] [PATCH v2 5/7] qcow2: Add iotest for an image with header.refcount_table_offset == 0

[Max Reitz]

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

On 2017-11-03 15:18, Alberto Garcia wrote:
> 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 <berto@igalia.com>
> ---
>  tests/qemu-iotests/060     | 7 +++++++
>  tests/qemu-iotests/060.out | 6 ++++++
>  2 files changed, 13 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 0/7] Misc qcow2 corruption checks

[Max Reitz]

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

On 2017-11-03 15:18, Alberto Garcia wrote:
> 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(-)

Thanks, applied to my block branch:

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

Max


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

Re: [Qemu-devel] [PATCH v2 3/7] qcow2: Prevent allocating compressed clusters at offset 0

[Alberto Garcia]

On Fri 03 Nov 2017 05:27:59 PM CET, Max Reitz wrote:
>> +            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)
>>              }
>>          }
>>  
>> -        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() ?

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/7] qcow2: Prevent allocating compressed clusters at offset 0

[Alberto Garcia]

On Fri 03 Nov 2017 09:22:39 PM CET, Alberto Garcia wrote:
>>> -        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" :-)

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/7] qcow2: Prevent allocating compressed clusters at offset 0

[Max Reitz]

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

On 2017-11-03 21:32, Alberto Garcia wrote:
> On Fri 03 Nov 2017 09:22:39 PM CET, Alberto Garcia wrote:
>>>> -        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" :-)

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).  Then, someone who reads the code does not
have to read all code paths to check whether that condition is true.

If an assert checks a condition that can be true, it's wrong.  Then
either the code is buggy (like it was before this patch) or the error
should be handled gracefully instead of aborting the program.

In a perfect world, all assert()s would be checked at compile time.

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/7] qcow2: Prevent allocating compressed clusters at offset 0

[Alberto Garcia]

On Mon 06 Nov 2017 01:36:01 PM CET, Max Reitz wrote:
>>>>> -        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" :-)
>
> 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).

Right, and they're especially useful when it's not obvious that the
assertion is always true.

The reason why I removed the assertion in this case is that the code
that checks and guarantees that the offset is not null is immediately
before the assert() line.

But I'm not going to argue over something like this, I don't mind if you
prefer to keep it :-)

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v2 4/7] qcow2: Don't open images with header.refcount_table_clusters == 0

[Kevin Wolf]

Am 03.11.2017 um 15:18 hat Alberto Garcia geschrieben:
> 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 <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -270,6 +270,13 @@ poke_file "$TEST_IMG" "$rb_offset"        "\x00\x00\x00\x00\x00\x00\x00\x00"
>  # write will try to allocate a compressed data cluster at offset 0.
>  $QEMU_IO -c "write -c 0k 64k" "$TEST_IMG" | _filter_qemu_io
>  
> +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?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 4/7] qcow2: Don't open images with header.refcount_table_clusters == 0

[Alberto Garcia]

On Tue 07 Nov 2017 05:43:49 PM CET, Kevin Wolf wrote:
>> +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?

Good idea. Max already merged this series in his branch, so I'll just
send a follow-up patch with this.

Berto