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

[Max Reitz]

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 <address@hidden>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  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 <address@hidden>

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

signature.asc
Description: OpenPGP digital signature