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

[Max Reitz <mreitz@redhat.com>]

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