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

[Vladimir Sementsov-Ogievskiy]

06.09.2019 22:57, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335

My fault, I'm sorry :( And great thanks for investigating this!

> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> The corruption happens when we do a write that
>     * writes to two or more unallocated clusters at once
>     * doesn't fully cover the first sector
>     * doesn't fully cover the last sector
> 
> In this case, when allocating the new clusters we COW both areas
> prior to the write and after the write, and we encrypt them.
> 
> The above mentioned commit accidentally made it so we encrypt the
> second COW area using the physical cluster offset of the first area.
> 
> Fix this by:
>   * Remove the offset_in_cluster parameter of do_perform_cow_encrypt,
>     since it is misleading. That offset can be larger than cluster size
>     currently.

Ohh, tricky thing. For sure I missed this logic.

> 
>     Instead just add the start and the end COW area 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
> 
> In the bugreport that was triggered by rebasing a luks image to new,
> zero filled base, which lot of such writes, and causes some files
> with zero areas to contain garbage there instead.
> But as described above it can happen elsewhere as well
> 
> 
> Signed-off-by: Maxim Levitsky <address@hidden>

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block/qcow2-cluster.c | 28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 1989b423da..6df17dd296 100644
> --- a/block/qcow2-cluster.c
> +++ 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((bytes & ~BDRV_SECTOR_MASK) == 0);
> +
> +        assert(QEMU_IS_ALIGNED(guest_offset | host_offset | bytes,
> +               BDRV_SECTOR_SIZE));
>           assert(s->crypto);
> -        if (qcow2_co_encrypt(bs, host_cluster_offset,
> -                             guest_cluster_offset + offset_in_cluster,
> -                             buffer, bytes) < 0) {
> +
> +        if (qcow2_co_encrypt(bs, start_of_cluster(s, host_offset),
> +                             guest_offset, buffer, bytes) < 0) {
>               return false;
>           }
>       }
> @@ -890,11 +890,15 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta 
> *m)
>   
>       /* Encrypt the data if necessary before writing it */
>       if (bs->encrypted) {
> -        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -                                    start->offset, start_buffer,
> +        if (!do_perform_cow_encrypt(bs,
> +                                    m->offset + start->offset,
> +                                    m->alloc_offset + start->offset,
> +                                    start_buffer,
>                                       start->nb_bytes) ||
> -            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
> -                                    end->offset, end_buffer, end->nb_bytes)) 
> {
> +            !do_perform_cow_encrypt(bs,
> +                                    m->offset + end->offset,
> +                                    m->alloc_offset + end->offset,
> +                                    end_buffer, end->nb_bytes)) {
>               ret = -EIO;
>               goto fail;
>           }
> 


-- 
Best regards,
Vladimir