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

[Max Reitz]

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 <address@hidden>
> ---
>  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 <address@hidden>

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
>

signature.asc
Description: OpenPGP digital signature