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

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

[Alberto Garcia]

Misc qcow2 corruption checks

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

Alberto Garcia (4):
  qcow2: Prevent allocating refcount blocks at offset 0
  qcow2: Prevent allocating L2 tables at offset 0
  qcow2: Don't open images with header.refcount_table_clusters == 0
  qcow2: Add iotest for an empty refcount table

 block/qcow2-cluster.c      |  7 +++++++
 block/qcow2-refcount.c     |  7 +++++++
 block/qcow2.c              |  6 ++++++
 tests/qemu-iotests/060     | 32 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/060.out | 25 +++++++++++++++++++++++++
 5 files changed, 77 insertions(+)

-- 
2.11.0

[Qemu-devel] [PATCH 1/4] 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>
---
 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 2/4] 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>
---
 block/qcow2-cluster.c      | 7 +++++++
 tests/qemu-iotests/060     | 7 +++++++
 tests/qemu-iotests/060.out | 6 ++++++
 3 files changed, 20 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fb10e26068..540af4d19d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -278,6 +278,13 @@ 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");
+        return -EIO;
+    }
+
     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 3/4] 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>
---
 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 40f85cc216..8fcfce1260 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -260,6 +260,13 @@ _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 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 5b8b518486..6db399d674 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -195,4 +195,9 @@ 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 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 4/4] qcow2: Add iotest for an empty refcount table

[Alberto Garcia]

This patch adds a simple iotests 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>
---
 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 8fcfce1260..9344278ac4 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 6db399d674..cc8a155643 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

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

[Max Reitz]

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

On 2017-11-01 16:42, Alberto Garcia wrote:
> Misc qcow2 corruption checks
> 
> 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

There are two more cases which might need a check that the return value
of an allocation function isn't 0:

The first is qcow2_alloc_bytes() which has an assert(offset) after
potentially setting offset = new_cluster (with new_cluster being the
return value of alloc_clusters_noref()).

The second is qcow2_crypto_hdr_init_func() which is simply missing a
pre-write overlap check.

The rest (besides L2 table and refblock allocation) should be guarded by
the pre-write overlap check.

Do you want to fix these or do we need another volunteer? :-)

Max


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

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

[Max Reitz]

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

On 2017-11-01 16:42, Alberto Garcia wrote:
> 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>
> ---
>  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 "

Technically, the offset is 0 and the size is s->cluster_size, but I
won't insist.

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

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



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

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

[Max Reitz]

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

On 2017-11-01 16:42, Alberto Garcia wrote:
> 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>
> ---
>  block/qcow2-cluster.c      | 7 +++++++
>  tests/qemu-iotests/060     | 7 +++++++
>  tests/qemu-iotests/060.out | 6 ++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index fb10e26068..540af4d19d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -278,6 +278,13 @@ 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");
> +        return -EIO;

The other error paths in this function use a goto fail.  It doesn't make
a functional difference, but not doing it here looks a bit weird.

(Also, there's same thing about offset=0 and size=s->cluster_size as in
patch 1.)

Functionally correct, though, so:

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

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



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

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

[Max Reitz]

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

On 2017-11-01 16:42, Alberto Garcia wrote:
> 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>
> ---
>  block/qcow2.c              | 6 ++++++
>  tests/qemu-iotests/060     | 7 +++++++
>  tests/qemu-iotests/060.out | 5 +++++
>  3 files changed, 18 insertions(+)

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


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

Re: [Qemu-devel] [PATCH 4/4] qcow2: Add iotest for an empty refcount table

[Max Reitz]

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

On 2017-11-01 16:42, Alberto Garcia wrote:
> This patch adds a simple iotests 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>
> ---
>  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 1/4] qcow2: Prevent allocating refcount blocks at offset 0

[Alberto Garcia]

On Thu 02 Nov 2017 06:28:18 PM CET, Max Reitz wrote:
>> 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 "
>
> Technically, the offset is 0 and the size is s->cluster_size, but I
> won't insist.

I actually checked the rest of qcow2_signal_corruption() cases in that
file and almost all use -1, -1.

I understand that the problem is not in [0, s->cluster_size] but in the
refcount table/block (the refcount data is wrong). The documentation of
BLOCK_IMAGE_CORRUPTED says:

# @offset: if the corruption resulted from an image access, this is the
#          host's access offset into the image

# @size: if the corruption resulted from an image access, this is the
#        access size

But that's not quite the case here, or is it?

Berto

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

[Alberto Garcia]

On Thu 02 Nov 2017 06:31:18 PM CET, Max Reitz wrote:
>> +    /* 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");
>> +        return -EIO;
>
> The other error paths in this function use a goto fail.  It doesn't
> make a functional difference, but not doing it here looks a bit weird.

Oh, you're right. I'll fix it.

Berto

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

[Alberto Garcia]

On Thu 02 Nov 2017 06:24:40 PM CET, Max Reitz wrote:
> There are two more cases which might need a check that the return
> value of an allocation function isn't 0:
>
> The first is qcow2_alloc_bytes() which has an assert(offset) after
> potentially setting offset = new_cluster (with new_cluster being the
> return value of alloc_clusters_noref()).

Ok. I don't know how to reproduce it, though, but a check won't hurt.

> The second is qcow2_crypto_hdr_init_func() which is simply missing a
> pre-write overlap check.

But that is called when you create a new image, i.e., this is not QEMU
handling a corrupted image incorrectly, but QEMU itself trying to create
a corrupted image.

I'd rather use assert(qcow2_pre_write_overlap_check(...) == 0);

Berto