Re: [Qemu-stable] [PATCH v5 1/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Vladimir Sementsov-Ogievskiy]

13.09.2019 18:28, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335
> 
> 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.
> 
> The problem is that offset_in_cluster in do_perform_cow_encrypt
> can be larger that the cluster size, thus cluster_offset
> will no longer point to the start of the cluster at which encrypted
> area starts.
> 
> Next patch in this series will refactor the code to avoid all these
> assumptions.
> 
> 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 | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index dcacd3c450..bfeb0241d7 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -474,9 +474,10 @@ static bool coroutine_fn 
> do_perform_cow_encrypt(BlockDriverState *bs,
>           assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>           assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>           assert(s->crypto);
> -        if (qcow2_co_encrypt(bs, cluster_offset,
> -                             src_cluster_offset + offset_in_cluster,
> -                             buffer, bytes) < 0) {
> +        if (qcow2_co_encrypt(bs,
> +                start_of_cluster(s, cluster_offset + offset_in_cluster),
> +                src_cluster_offset + offset_in_cluster,
> +                buffer, bytes) < 0) {
>               return false;
>           }
>       }
> 


-- 
Best regards,
Vladimir