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 <mlevi...@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > 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
