Re: [Qemu-devel] [PATCH 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files

[Eric Blake]

On 9/6/19 12:32 PM, Maxim Levitsky wrote:
> This fixes subltle corruption introduced by luks threaded encryption

subtle

> in commit 8ac0f15f335
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> The corruption happens when we do
>    * write to two or more unallocated clusters at once
>    * write doesn't fully cover nether first nor last cluster

s/nether/neither/

or even:

write doesn't fully cover either the first or the last cluster

> 
> In this case, when allocating the new clusters we COW both area

areas

> prior to the write and after the write, and we encrypt them.
> 
> The above mentioned commit accidently made it so, we encrypt the

accidentally

s/made it so, we encrypt/changed the encryption of/

> second COW are using the physical cluster offset of the first area.

s/are using/to use/

> 
> Fix this by:
>  * remove the offset_in_cluster parameter of do_perform_cow_encrypt
>    since it is misleading. That offset can be larger that cluster size.
>    instead just add the start and end COW are offsets to both host and guest 
> offsets
>    that do_perform_cow_encrypt receives.
> 
> *  in do_perform_cow_encrypt, remove the cluster offset from the host_offset
>    And thus pass correctly to the qcow2_co_encrypt, the host cluster offset 
> and full guest offset
> 
> 
> Signed-off-by: Maxim Levitsky <address@hidden>
> ---
>  block/qcow2-cluster.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 

> +++ b/block/qcow2-cluster.c
> @@ -463,20 +463,20 @@ static int coroutine_fn 
> do_perform_cow_read(BlockDriverState *bs,
>  }
>  
>  static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -                                                uint64_t 
> guest_cluster_offset,
> -                                                uint64_t host_cluster_offset,
> -                                                unsigned offset_in_cluster,
> +                                                uint64_t guest_offset,
> +                                                uint64_t host_offset,
>                                                  uint8_t *buffer,
>                                                  unsigned bytes)
>  {
>      if (bytes && bs->encrypted) {
>          BDRVQcow2State *s = bs->opaque;
> -        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> +        assert((guest_offset & ~BDRV_SECTOR_MASK) == 0);
> +        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
>          assert((bytes & ~BDRV_SECTOR_MASK) == 0);

Pre-existing, but we could use QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE) for
slightly more legibility than open-coding the bit operation.

Neat trick about power-of-2 alignment checks:

assert(QEMU_IS_ALIGNED(offset_in_cluster | guest_offset |
                       host_offset | bytes, BDRV_SECTOR_SIZE));

gives the same result in one assertion.  (I've used it elsewhere in the
code base, but I'm not opposed to one assert per variable if you think
batching is too dense.)

I'll let Dan review the actual code change, but offhand it makes sense
to me.

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

signature.asc
Description: OpenPGP digital signature