diff mbox series

[v2,1/4] qcow2: Require that the virtual size is a multiple of the sector size

Message ID 6a1cfcbb533b487bac96e1d2282ebf210954d27a.1578596897.git.berto@igalia.com
State New
Headers show
Series [v2,1/4] qcow2: Require that the virtual size is a multiple of the sector size | expand

Commit Message

Alberto Garcia Jan. 9, 2020, 7:12 p.m. UTC
The qcow2 header specifies the virtual size of the image in bytes, but
BlockDriverState stores it as a number of 512-byte sectors.

If the user tries to create an image with a size that is not a
multiple of the sector size then this is fixed on creation by
silently rounding the image size up (see commit c2eb918e32).
qcow2_co_truncate() is more strict and returns an error instead.

However when an image is opened the virtual size is rounded down,
which means that trying to access the last few advertised bytes will
result in an error. As seen above QEMU cannot create such images and
there's no good use case that would require us to try to handle them
so let's just treat them as unsupported.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c              | 7 +++++++
 docs/interop/qcow2.txt     | 3 ++-
 tests/qemu-iotests/080     | 8 ++++++++
 tests/qemu-iotests/080.out | 5 +++++
 4 files changed, 22 insertions(+), 1 deletion(-)

Comments

Max Reitz Jan. 14, 2020, 1:01 p.m. UTC | #1
On 09.01.20 20:12, Alberto Garcia wrote:
> The qcow2 header specifies the virtual size of the image in bytes, but
> BlockDriverState stores it as a number of 512-byte sectors.
> 
> If the user tries to create an image with a size that is not a
> multiple of the sector size then this is fixed on creation by
> silently rounding the image size up (see commit c2eb918e32).
> qcow2_co_truncate() is more strict and returns an error instead.
> 
> However when an image is opened the virtual size is rounded down,
> which means that trying to access the last few advertised bytes will
> result in an error. As seen above QEMU cannot create such images and
> there's no good use case that would require us to try to handle them
> so let's just treat them as unsupported.

But isn’t that just a bug in qemu?

Max
Alberto Garcia Jan. 14, 2020, 1:20 p.m. UTC | #2
On Tue 14 Jan 2020 02:01:03 PM CET, Max Reitz wrote:
>> However when an image is opened the virtual size is rounded down,
>> which means that trying to access the last few advertised bytes will
>> result in an error. As seen above QEMU cannot create such images and
>> there's no good use case that would require us to try to handle them
>> so let's just treat them as unsupported.
>
> But isn’t that just a bug in qemu?

Yes, but does it make sense to try to support images with unaligned
sizes if no one is going to create them ever and QEMU cannot even
generate them?

Berto
Max Reitz Jan. 14, 2020, 1:47 p.m. UTC | #3
On 14.01.20 14:20, Alberto Garcia wrote:
> On Tue 14 Jan 2020 02:01:03 PM CET, Max Reitz wrote:
>>> However when an image is opened the virtual size is rounded down,
>>> which means that trying to access the last few advertised bytes will
>>> result in an error. As seen above QEMU cannot create such images and
>>> there's no good use case that would require us to try to handle them
>>> so let's just treat them as unsupported.
>>
>> But isn’t that just a bug in qemu?
> 
> Yes, but does it make sense to try to support images with unaligned
> sizes if no one is going to create them ever and QEMU cannot even
> generate them?

If nobody uses such images ever, isn’t the current code fine as-is?

Max
Alberto Garcia Jan. 14, 2020, 1:58 p.m. UTC | #4
On Tue 14 Jan 2020 02:47:53 PM CET, Max Reitz wrote:

>> Yes, but does it make sense to try to support images with unaligned
>> sizes if no one is going to create them ever and QEMU cannot even
>> generate them?
>
> If nobody uses such images ever, isn’t the current code fine as-is?

I'll rephrase:

1. It is possible to have a qcow2 image with a size of 1 GB + 13 bytes
   (the size field in the qcow2 header allows it).

2. qemu-img cannot currently produce such images.

3. QEMU can open them but it gets the size wrong.

4. Fixing this in QEMU involves non-trivial changes (at least at the
   moment).

5. I don't think there's any use case for such images so instead of
   fixing QEMU I propose that we simply refuse to open them.

Berto
Max Reitz Jan. 14, 2020, 2:03 p.m. UTC | #5
On 14.01.20 14:58, Alberto Garcia wrote:
> On Tue 14 Jan 2020 02:47:53 PM CET, Max Reitz wrote:
> 
>>> Yes, but does it make sense to try to support images with unaligned
>>> sizes if no one is going to create them ever and QEMU cannot even
>>> generate them?
>>
>> If nobody uses such images ever, isn’t the current code fine as-is?
> 
> I'll rephrase:
> 
> 1. It is possible to have a qcow2 image with a size of 1 GB + 13 bytes
>    (the size field in the qcow2 header allows it).
> 
> 2. qemu-img cannot currently produce such images.
> 
> 3. QEMU can open them but it gets the size wrong.

Yes, but in a safe way.  The user simply doesn’t get access to those 13
bytes.

With your proposed change, it would just reject the image altogether.
As a user, I’d prefer not being able to access 13 bytes to not being
able to access 1 GB + 13 bytes.

Furthermore, the user couldn’t even fix the image then because they
couldn’t resize it with qemu-img resize.

> 4. Fixing this in QEMU involves non-trivial changes (at least at the
>    moment).
> 
> 5. I don't think there's any use case for such images so instead of
>    fixing QEMU I propose that we simply refuse to open them.

If there’s no use case, I don’t see how the current behavior is problematic.

The arguments I could immediately agree to are (1) simplicity, and (2)
the image is probably corrupted.

As for (1), this patch adds more lines than it removes, so from a pure
quantitative standpoint, it doesn’t make the code simpler.

As for (2), I’m always of the opinion we shouldn’t refuse to read
corrupted images just because they are corrupted.  We should open them
and let the user read as much as is reasonably possible.

Max
Alberto Garcia Jan. 16, 2020, 2:24 p.m. UTC | #6
On Tue 14 Jan 2020 03:03:31 PM CET, Max Reitz wrote:
>> 3. QEMU can open them but it gets the size wrong.
>
> Yes, but in a safe way.  The user simply doesn’t get access to those
> 13 bytes.

Ok, I think I'll withdraw the patch then :-)

Berto
diff mbox series

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 7fbaac8457..87ca2832f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1326,6 +1326,13 @@  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
+    if (!QEMU_IS_ALIGNED(header.size, BDRV_SECTOR_SIZE)) {
+        error_setg(errp, "Virtual size is not a multiple of %u",
+                   (unsigned) BDRV_SECTOR_SIZE);
+        ret = -EINVAL;
+        goto fail;
+    }
+
     if (header.header_length > sizeof(header)) {
         s->unknown_header_fields_size = header.header_length - sizeof(header);
         s->unknown_header_fields = g_malloc(s->unknown_header_fields_size);
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..891f5662d8 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -40,7 +40,8 @@  The first cluster of a qcow2 image contains the file header:
                     with larger cluster sizes.
 
          24 - 31:   size
-                    Virtual disk size in bytes.
+                    Virtual disk size in bytes. qemu can only handle
+                    sizes that are a multiple of 512 bytes.
 
                     Note: qemu has an implementation limit of 32 MB as
                     the maximum L1 table size.  With a 2 MB cluster
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 4bcb5021e8..6f136d616f 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -48,6 +48,7 @@  header_size=104
 
 offset_backing_file_offset=8
 offset_backing_file_size=16
+offset_virtual_size=24
 offset_l1_size=36
 offset_l1_table_offset=40
 offset_refcount_table_offset=48
@@ -197,6 +198,13 @@  poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
 { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
 _check_test_img
 
+echo
+echo "== Image size not a multiple of the sector size =="
+_make_test_img 64k
+echo "Modifying virtual size to 65535 bytes"
+poke_file "$TEST_IMG" "$offset_virtual_size" "\x00\x00\x00\x00\x00\x00\xff\xff"
+{ $QEMU_IMG info $TEST_IMG; } 2>&1 | _filter_testdir | _filter_imgfmt
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 45ab01db8e..aadc817339 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -104,4 +104,9 @@  Data may be corrupted, or further writes to the image may corrupt it.
 
 3 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
+
+== Image size not a multiple of the sector size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+Modifying virtual size to 65535 bytes
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Virtual size is not a multiple of 512
 *** done