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
Reviewed-by: Vladimir Sementsov-Ogievskiy
> ---
> 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