Patchwork [for-4.0?,2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file

login
register
mail settings
Submitter Kevin Wolf
Date April 15, 2019, 3:54 p.m.
Message ID <20190415155452.5115-3-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/773335/
State New
Headers show

Comments

Kevin Wolf - April 15, 2019, 3:54 p.m.
With an external data file, preallocate_co() must write the final byte
to the external data file, not to the qcow2 image file.

This is harmless for preallocation of newly created images (only the
qcow2 file size is increased to the virtual disk size while it should be
much smaller), but with preallocated resize, it could in theory cause
visible corruption if the metadata of the image is larger than the data
(e.g. lots of bitmaps).

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Eric Blake - April 15, 2019, 4:17 p.m.
On 4/15/19 10:54 AM, Kevin Wolf wrote:
> With an external data file, preallocate_co() must write the final byte
> to the external data file, not to the qcow2 image file.
> 
> This is harmless for preallocation of newly created images (only the
> qcow2 file size is increased to the virtual disk size while it should be
> much smaller), but with preallocated resize, it could in theory cause
> visible corruption if the metadata of the image is larger than the data
> (e.g. lots of bitmaps).

Can we come up with such an image - maybe one with 512-byte cluster
sizing and only 1k in guest-visible length?  Since each bitmap is
cluster-aligned, it seems like you'd only need a couple of bitmaps to
easily reach that point.

We're awfully late for 4.0, but as we already have -rc4 coming due, and
as this is a data-corruption bug in a new feature, I can buy the
argument of getting this one into 4.0, particularly if you can design
the iotest along the lines of my ideas to prove that yes, indeed, we are
accidentally wiping out qcow2 metadata for visible image corruption.

Reviewed-by: Eric Blake <eblake@redhat.com>

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index c8400e9712..dfac74c264 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2772,7 +2772,7 @@  static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
      */
     if (host_offset != 0) {
         uint8_t data = 0;
-        ret = bdrv_pwrite(bs->file, (host_offset + cur_bytes) - 1,
+        ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
                           &data, 1);
         if (ret < 0) {
             return ret;