Re: [Qemu-block] [PATCH 1/4] qcow2: Prevent allocating refcount blocks at offset 0
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-block] [PATCH 1/4] qcow2: Prevent allocating refcount blocks at offset 0
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
> Signed-off-by: Alberto Garcia
> ---
> 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
> +"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
[Qemu-block] [PATCH 1/4] qcow2: Prevent allocating refcount blocks at offset 0
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
Signed-off-by: Alberto Garcia
---
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
