discard and v2 qcow2 images

discard and v2 qcow2 images

[Alberto Garcia]

Hi,

when full_discard is false in discard_in_l2_slice() then the selected
cluster should be deallocated and it should read back as zeroes. This
is done by clearing the cluster offset field and setting OFLAG_ZERO in
the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible
previous data from the backing file.

This can be trivially reproduced like this:

   qemu-img create -f qcow2 backing.img 64k
   qemu-io -c 'write -P 0xff 0 64k' backing.img
   qemu-img create -f qcow2 -o compat=0.10 -b backing.img top.img
   qemu-io -c 'write -P 0x01 0 64k' top.img

After this, top.img is filled with 0x01. Now we issue a discard
command:

   qemu-io -c 'discard 0 64k' top.img

top.img should now read as zeroes, but instead you get the data from
the backing file (0xff). If top.img was created with compat=1.1
instead (the default) then it would read as zeroes after the discard.

This seems like a bug to me, and I would simply forbid using discard
in this case (see below). The other user of full_discard = false is
qcow2_snapshot_create() but I think that one is safe and should be
allowed?

--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3763,6 +3763,10 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
     int ret;
     BDRVQcow2State *s = bs->opaque;
 
+    if (s->qcow_version < 3) {
+        return -ENOTSUP;
+    }
+
     if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
         assert(bytes < s->cluster_size);
         /* Ignore partial clusters, except for the special case of the

Berto

Re: discard and v2 qcow2 images

[Eric Blake]

On 3/20/20 1:58 PM, Alberto Garcia wrote:
> Hi,
> 
> when full_discard is false in discard_in_l2_slice() then the selected
> cluster should be deallocated and it should read back as zeroes. This
> is done by clearing the cluster offset field and setting OFLAG_ZERO in
> the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible
> previous data from the backing file.

Discard is advisory, and has no requirements that discarded data read 
back as zero.  However, if write zeroes uses discard under the hood, 
then THAT usage must guarantee reading back as zero.

> 
> This can be trivially reproduced like this:
> 
>     qemu-img create -f qcow2 backing.img 64k
>     qemu-io -c 'write -P 0xff 0 64k' backing.img
>     qemu-img create -f qcow2 -o compat=0.10 -b backing.img top.img
>     qemu-io -c 'write -P 0x01 0 64k' top.img
> 
> After this, top.img is filled with 0x01. Now we issue a discard
> command:
> 
>     qemu-io -c 'discard 0 64k' top.img
> 
> top.img should now read as zeroes, but instead you get the data from
> the backing file (0xff). If top.img was created with compat=1.1
> instead (the default) then it would read as zeroes after the discard.

I'd argue that this is undesirable behavior, but not a bug.

> 
> This seems like a bug to me, and I would simply forbid using discard
> in this case (see below). The other user of full_discard = false is
> qcow2_snapshot_create() but I think that one is safe and should be
> allowed?
> 
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3763,6 +3763,10 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
>       int ret;
>       BDRVQcow2State *s = bs->opaque;
>   
> +    if (s->qcow_version < 3) {
> +        return -ENOTSUP;
> +    }
> +

This changes it so you no longer see stale data, but doesn't change the 
fact that you don't read zeroes (just that your stale data is now from 
the current layer instead of the backing layer, since we did nothing at 
all).

I'm not opposed to the patch, per se, but am not convinced that this is 
a problem to worry about.

>       if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
>           assert(bytes < s->cluster_size);
>           /* Ignore partial clusters, except for the special case of the
> 
> Berto
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: discard and v2 qcow2 images

[Alberto Garcia]

On Fri 20 Mar 2020 08:35:44 PM CET, Eric Blake <eblake@redhat.com> wrote:
>> This flag is however only supported when qcow_version >= 3. In older
>> images the cluster is simply deallocated, exposing any possible
>> previous data from the backing file.
>
> Discard is advisory, and has no requirements that discarded data read
> back as zero.  However, if write zeroes uses discard under the hood,
> then THAT usage must guarantee reading back as zero.

write_zeroes doesn't seem to use discard in any case, so no problem
there.

>> @@ -3763,6 +3763,10 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
>>       int ret;
>>       BDRVQcow2State *s = bs->opaque;
>>   
>> +    if (s->qcow_version < 3) {
>> +        return -ENOTSUP;
>> +    }
>> +
>
> This changes it so you no longer see stale data, but doesn't change
> the fact that you don't read zeroes (just that your stale data is now
> from the current layer instead of the backing layer, since we did
> nothing at all).

discard can already fail if the request is not aligned, in this case you
get -ENOTSUP and stay with the same data as before.

What's different in this case is that you can actually get stale data,
that doesn't seem like a desirable outcome.

Berto

Re: discard and v2 qcow2 images

[Daniel P. Berrangé]

On Fri, Mar 20, 2020 at 02:35:44PM -0500, Eric Blake wrote:
> On 3/20/20 1:58 PM, Alberto Garcia wrote:
> > Hi,
> > 
> > when full_discard is false in discard_in_l2_slice() then the selected
> > cluster should be deallocated and it should read back as zeroes. This
> > is done by clearing the cluster offset field and setting OFLAG_ZERO in
> > the L2 entry.
> > 
> > This flag is however only supported when qcow_version >= 3. In older
> > images the cluster is simply deallocated, exposing any possible
> > previous data from the backing file.
> 
> Discard is advisory, and has no requirements that discarded data read back
> as zero.  However, if write zeroes uses discard under the hood, then THAT
> usage must guarantee reading back as zero.
> 
> > 
> > This can be trivially reproduced like this:
> > 
> >     qemu-img create -f qcow2 backing.img 64k
> >     qemu-io -c 'write -P 0xff 0 64k' backing.img
> >     qemu-img create -f qcow2 -o compat=0.10 -b backing.img top.img
> >     qemu-io -c 'write -P 0x01 0 64k' top.img
> > 
> > After this, top.img is filled with 0x01. Now we issue a discard
> > command:
> > 
> >     qemu-io -c 'discard 0 64k' top.img
> > 
> > top.img should now read as zeroes, but instead you get the data from
> > the backing file (0xff). If top.img was created with compat=1.1
> > instead (the default) then it would read as zeroes after the discard.
> 
> I'd argue that this is undesirable behavior, but not a bug.

I think the ability to read old data from the backing file could
potentially be considered a security flaw, depending on what the
original data was in the backing file.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|