| Message ID | fb53467cf48e95ff3330def1cf1003a5b862b7d9.1509718618.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc qcow2 corruption checks | expand |
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 <berto@igalia.com> > --- > 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 <mreitz@redhat.com> 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 >
On Fri 03 Nov 2017 05:27:59 PM CET, Max Reitz wrote: >> + 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. :-) After the new check offset is now guaranteed to be 0, so what's the point of keeping the assert() ? Berto
On Fri 03 Nov 2017 09:22:39 PM CET, Alberto Garcia wrote: >>> - assert(offset); >> >> I don't think this assert() was meant as a protection against offset >> being 0. :-) > > After the new check offset is now guaranteed to be 0, so what's the > point of keeping the assert() ? I meant "guaranteed _not_ to be 0" :-) Berto
On 2017-11-03 21:32, Alberto Garcia wrote: > On Fri 03 Nov 2017 09:22:39 PM CET, Alberto Garcia wrote: >>>> - assert(offset); >>> >>> I don't think this assert() was meant as a protection against offset >>> being 0. :-) >> >> After the new check offset is now guaranteed to be 0, so what's the >> point of keeping the assert() ? > > I meant "guaranteed _not_ to be 0" :-) That is the point of an assert. An assert should not guard against something that can occur. It should express that something will always be true (in this case that the offset is guaranteed not to be 0). Then, someone who reads the code does not have to read all code paths to check whether that condition is true. If an assert checks a condition that can be true, it's wrong. Then either the code is buggy (like it was before this patch) or the error should be handled gracefully instead of aborting the program. In a perfect world, all assert()s would be checked at compile time. Max
On Mon 06 Nov 2017 01:36:01 PM CET, Max Reitz wrote: >>>>> - assert(offset); >>>> >>>> I don't think this assert() was meant as a protection against offset >>>> being 0. :-) >>> >>> After the new check offset is now guaranteed to be 0, so what's the >>> point of keeping the assert() ? >> >> I meant "guaranteed _not_ to be 0" :-) > > That is the point of an assert. > > An assert should not guard against something that can occur. It should > express that something will always be true (in this case that the > offset is guaranteed not to be 0). Right, and they're especially useful when it's not obvious that the assertion is always true. The reason why I removed the assertion in this case is that the code that checks and guarantees that the offset is not null is immediately before the assert() line. But I'm not going to argue over something like this, I don't mind if you prefer to keep it :-) Berto
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); 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
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 <berto@igalia.com> --- block/qcow2-refcount.c | 8 +++++++- tests/qemu-iotests/060 | 10 ++++++++++ tests/qemu-iotests/060.out | 8 ++++++++ 3 files changed, 25 insertions(+), 1 deletion(-)