[v5,2/3] block/qcow2: refactor threaded encryption code
diff mbox series

Message ID 20190913152818.17843-3-mlevitsk@redhat.com
State New
Headers show
Series
  • Fix qcow2+luks corruption introduced by commit 8ac0f15f335
Related show

Commit Message

Maxim Levitsky Sept. 13, 2019, 3:28 p.m. UTC
Change do_perform_cow_encrypt and its callee qcow2_co_encrypt
to just receive full host and guest offsets and in pariticular
remove the offset_in_cluster parameter of do_perform_cow_encrypt,
since it is misleading, because that offset can be larger than
cluster size currently.

Also document the qcow2_co_encrypt arguments to prevent
that bug from happening again

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c | 30 ++++++++++++---------
 block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
 block/qcow2.c         |  5 ++--
 block/qcow2.h         |  8 +++---
 4 files changed, 73 insertions(+), 32 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Sept. 13, 2019, 4:11 p.m. UTC | #1
13.09.2019 18:28, Maxim Levitsky wrote:
> Change do_perform_cow_encrypt and its callee qcow2_co_encrypt
> to just receive full host and guest offsets and in pariticular
> remove the offset_in_cluster parameter of do_perform_cow_encrypt,
> since it is misleading, because that offset can be larger than
> cluster size currently.
> 
> Also document the qcow2_co_encrypt arguments to prevent
> that bug from happening again
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>   block/qcow2-cluster.c | 30 ++++++++++++---------
>   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
>   block/qcow2.c         |  5 ++--
>   block/qcow2.h         |  8 +++---
>   4 files changed, 73 insertions(+), 32 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bfeb0241d7..e87a4637fd 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,21 +463,21 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
>   }
>   
>   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -                                                uint64_t src_cluster_offset,
> -                                                uint64_t 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, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
>           assert(s->crypto);
> -        if (qcow2_co_encrypt(bs,
> -                start_of_cluster(s, cluster_offset + offset_in_cluster),
> -                src_cluster_offset + offset_in_cluster,
> -                buffer, bytes) < 0) {
> +
> +        if (qcow2_co_encrypt(bs, host_offset, guest_offset,
> +            buffer, bytes) < 0) {

strange alignment of second line of the condition.. [1]

>               return false;
>           }
>       }
> @@ -891,11 +891,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)) {

Looking at this now, I think that do_perform_cow_encrypt can be dropped at all,
as it's now just an extra wrapper, sending same parameters to qcow2_co_encrypt.

I'll send a follow-up, if this patch goes as is.

>               ret = -EIO;
>               goto fail;
>           }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..9646243a9b 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,15 @@ static int qcow2_encdec_pool_func(void *opaque)
>   }
>   
>   static int coroutine_fn
> -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
> +                uint64_t guest_offset, void *buf, size_t len,
> +                Qcow2EncDecFunc func)
>   {
>       BDRVQcow2State *s = bs->opaque;
> +
>       Qcow2EncDecData arg = {
>           .block = s->crypto,
> -        .offset = s->crypt_physical_offset ?
> -                      file_cluster_offset + offset_into_cluster(s, offset) :
> -                      offset,
> +        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
>           .buf = buf,
>           .len = len,
>           .func = func,
> @@ -251,18 +251,54 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
>       return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
>   }
>   
> +
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts one or more contiguous aligned sectors
> + *
> + * @host_offset - underlying storage offset of the first sector of the
> + * data to be encrypted
> +

asterisk missed

> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
> + *
> + * @buf - buffer with the data to encrypt, that after encryption
> + *        will be written to the underlying storage device at
> + *        @host_offset
> + *
> + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> + *
> + * Depending on the encryption method, @host_cluster_offset and/or @guest_offset

s/host_cluster_offset/host_offset

With at least alignment[1] and s/host_cluster_offset/host_offset/ fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> + * may be used for generating the initialization vector for
> + * encryption.
> + *
> + * Note that while the whole range must be aligned on sectors, it
> + * does not have to be aligned on clusters and can also cross cluster
> + * boundaries
> + *

hmm, extra empty line IMHO.

> + */
>   int coroutine_fn
> -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len)
> +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len)
>   {
> -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> -                             qcrypto_block_encrypt);
> +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> +                           qcrypto_block_encrypt);
>   }
>   
> +
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts one or more contiguous aligned sectors
> + * Similar to qcow2_co_encrypt
> + *

and this one.

> + */
> +
>   int coroutine_fn
> -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len)
> +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len)
>   {
> -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> -                             qcrypto_block_decrypt);
> +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> +                           qcrypto_block_decrypt);
>   }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 57734f20cf..ac768092bb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
>   
>                   assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
>                   assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> -                if (qcow2_co_decrypt(bs, cluster_offset, offset,
> +                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
> +                                     offset,
>                                        cluster_data, cur_bytes) < 0) {
>                       ret = -EIO;
>                       goto fail;
> @@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
>               qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
>                                 cluster_data, cur_bytes);
>   
> -            if (qcow2_co_encrypt(bs, cluster_offset, offset,
> +            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
>                                    cluster_data, cur_bytes) < 0) {
>                   ret = -EIO;
>                   goto out_unlocked;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 998bcdaef1..a488d761ff 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -758,10 +758,10 @@ ssize_t coroutine_fn
>   qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
>                       const void *src, size_t src_size);
>   int coroutine_fn
> -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len);
> +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len);
>   int coroutine_fn
> -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len);
> +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> +                 uint64_t guest_offset, void *buf, size_t len);
>   
>   #endif
>
Maxim Levitsky Sept. 13, 2019, 4:27 p.m. UTC | #2
On Fri, 2019-09-13 at 16:11 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 13.09.2019 18:28, Maxim Levitsky wrote:
> > Change do_perform_cow_encrypt and its callee qcow2_co_encrypt
> > to just receive full host and guest offsets and in pariticular
> > remove the offset_in_cluster parameter of do_perform_cow_encrypt,
> > since it is misleading, because that offset can be larger than
> > cluster size currently.
> > 
> > Also document the qcow2_co_encrypt arguments to prevent
> > that bug from happening again
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   block/qcow2-cluster.c | 30 ++++++++++++---------
> >   block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
> >   block/qcow2.c         |  5 ++--
> >   block/qcow2.h         |  8 +++---
> >   4 files changed, 73 insertions(+), 32 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index bfeb0241d7..e87a4637fd 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -463,21 +463,21 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> >   }
> >   
> >   static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > -                                                uint64_t src_cluster_offset,
> > -                                                uint64_t 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, BDRV_SECTOR_SIZE));
> > +        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
> > +        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> >           assert(s->crypto);
> > -        if (qcow2_co_encrypt(bs,
> > -                start_of_cluster(s, cluster_offset + offset_in_cluster),
> > -                src_cluster_offset + offset_in_cluster,
> > -                buffer, bytes) < 0) {
> > +
> > +        if (qcow2_co_encrypt(bs, host_offset, guest_offset,
> > +            buffer, bytes) < 0) {
> 
> strange alignment of second line of the condition.. [1]
To be honest I am too tired currently, I think I should align to the (
of function argument if possible.
I removed the do_perform_cow_encrypt as you suggested so no issue.

> 
> >               return false;
> >           }
> >       }
> > @@ -891,11 +891,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)) {
> 
> Looking at this now, I think that do_perform_cow_encrypt can be dropped at all,
> as it's now just an extra wrapper, sending same parameters to qcow2_co_encrypt.
> 
> I'll send a follow-up, if this patch goes as is.
I kind of agree but I didn't want to make too many changes. I do think 
now also that it is worth it.


> 
> >               ret = -EIO;
> >               goto fail;
> >           }
> > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > index 3b1e63fe41..9646243a9b 100644
> > --- a/block/qcow2-threads.c
> > +++ b/block/qcow2-threads.c
> > @@ -234,15 +234,15 @@ static int qcow2_encdec_pool_func(void *opaque)
> >   }
> >   
> >   static int coroutine_fn
> > -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
> > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
> > +                uint64_t guest_offset, void *buf, size_t len,
> > +                Qcow2EncDecFunc func)
> >   {
> >       BDRVQcow2State *s = bs->opaque;
> > +
> >       Qcow2EncDecData arg = {
> >           .block = s->crypto,
> > -        .offset = s->crypt_physical_offset ?
> > -                      file_cluster_offset + offset_into_cluster(s, offset) :
> > -                      offset,
> > +        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
> >           .buf = buf,
> >           .len = len,
> >           .func = func,
> > @@ -251,18 +251,54 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> >       return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> >   }
> >   
> > +
> > +/*
> > + * qcow2_co_encrypt()
> > + *
> > + * Encrypts one or more contiguous aligned sectors
> > + *
> > + * @host_offset - underlying storage offset of the first sector of the
> > + * data to be encrypted
> > +
> 
> asterisk missed
Too tired :-(
> 
> > + * @guest_offset - guest (virtual) offset of the first sector of the
> > + * data to be encrypted
> > + *
> > + * @buf - buffer with the data to encrypt, that after encryption
> > + *        will be written to the underlying storage device at
> > + *        @host_offset
> > + *
> > + * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
> > + *
> > + * Depending on the encryption method, @host_cluster_offset and/or @guest_offset
> 
> s/host_cluster_offset/host_offset
Opps, that is leftover, I remember that I already fixed that, but I guess
I didn't.
> 
> With at least alignment[1] and s/host_cluster_offset/host_offset/ fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> > + * may be used for generating the initialization vector for
> > + * encryption.
> > + *
> > + * Note that while the whole range must be aligned on sectors, it
> > + * does not have to be aligned on clusters and can also cross cluster
> > + * boundaries
> > + *
> 
> hmm, extra empty line IMHO.

> 
> > + */
> >   int coroutine_fn
> > -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len)
> > +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >   {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > -                             qcrypto_block_encrypt);
> > +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> > +                           qcrypto_block_encrypt);
> >   }
> >   
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts one or more contiguous aligned sectors
> > + * Similar to qcow2_co_encrypt
> > + *
> 
> and this one.
Doesn't really matter, but no problem to fix.

> 
> > + */
> > +
> >   int coroutine_fn
> > -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len)
> > +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >   {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > -                             qcrypto_block_decrypt);
> > +    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
> > +                           qcrypto_block_decrypt);
> >   }
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 57734f20cf..ac768092bb 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2069,7 +2069,8 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
> >   
> >                   assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> >                   assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
> > -                if (qcow2_co_decrypt(bs, cluster_offset, offset,
> > +                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
> > +                                     offset,
> >                                        cluster_data, cur_bytes) < 0) {
> >                       ret = -EIO;
> >                       goto fail;
> > @@ -2288,7 +2289,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
> >               qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
> >                                 cluster_data, cur_bytes);
> >   
> > -            if (qcow2_co_encrypt(bs, cluster_offset, offset,
> > +            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
> >                                    cluster_data, cur_bytes) < 0) {
> >                   ret = -EIO;
> >                   goto out_unlocked;
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index 998bcdaef1..a488d761ff 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -758,10 +758,10 @@ ssize_t coroutine_fn
> >   qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> >                       const void *src, size_t src_size);
> >   int coroutine_fn
> > -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len);
> > +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len);
> >   int coroutine_fn
> > -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len);
> > +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len);
> >   
> >   #endif
> 


Thanks for the review,
	Best regards,
		Maxim Levitsky

Patch
diff mbox series

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bfeb0241d7..e87a4637fd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -463,21 +463,21 @@  static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
 }
 
 static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-                                                uint64_t src_cluster_offset,
-                                                uint64_t 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, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
         assert(s->crypto);
-        if (qcow2_co_encrypt(bs,
-                start_of_cluster(s, cluster_offset + offset_in_cluster),
-                src_cluster_offset + offset_in_cluster,
-                buffer, bytes) < 0) {
+
+        if (qcow2_co_encrypt(bs, host_offset, guest_offset,
+            buffer, bytes) < 0) {
             return false;
         }
     }
@@ -891,11 +891,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;
         }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..9646243a9b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -234,15 +234,15 @@  static int qcow2_encdec_pool_func(void *opaque)
 }
 
 static int coroutine_fn
-qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
-                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
+qcow2_co_encdec(BlockDriverState *bs, uint64_t host_offset,
+                uint64_t guest_offset, void *buf, size_t len,
+                Qcow2EncDecFunc func)
 {
     BDRVQcow2State *s = bs->opaque;
+
     Qcow2EncDecData arg = {
         .block = s->crypto,
-        .offset = s->crypt_physical_offset ?
-                      file_cluster_offset + offset_into_cluster(s, offset) :
-                      offset,
+        .offset = s->crypt_physical_offset ? host_offset : guest_offset,
         .buf = buf,
         .len = len,
         .func = func,
@@ -251,18 +251,54 @@  qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
     return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
 }
 
+
+/*
+ * qcow2_co_encrypt()
+ *
+ * Encrypts one or more contiguous aligned sectors
+ *
+ * @host_offset - underlying storage offset of the first sector of the
+ * data to be encrypted
+
+ * @guest_offset - guest (virtual) offset of the first sector of the
+ * data to be encrypted
+ *
+ * @buf - buffer with the data to encrypt, that after encryption
+ *        will be written to the underlying storage device at
+ *        @host_offset
+ *
+ * @len - length of the buffer (must be a BDRV_SECTOR_SIZE multiple)
+ *
+ * Depending on the encryption method, @host_cluster_offset and/or @guest_offset
+ * may be used for generating the initialization vector for
+ * encryption.
+ *
+ * Note that while the whole range must be aligned on sectors, it
+ * does not have to be aligned on clusters and can also cross cluster
+ * boundaries
+ *
+ */
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_encrypt);
+    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+                           qcrypto_block_encrypt);
 }
 
+
+/*
+ * qcow2_co_decrypt()
+ *
+ * Decrypts one or more contiguous aligned sectors
+ * Similar to qcow2_co_encrypt
+ *
+ */
+
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_decrypt);
+    return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len,
+                           qcrypto_block_decrypt);
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf..ac768092bb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2069,7 +2069,8 @@  static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
 
                 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
                 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
-                if (qcow2_co_decrypt(bs, cluster_offset, offset,
+                if (qcow2_co_decrypt(bs, cluster_offset + offset_in_cluster,
+                                     offset,
                                      cluster_data, cur_bytes) < 0) {
                     ret = -EIO;
                     goto fail;
@@ -2288,7 +2289,7 @@  static coroutine_fn int qcow2_co_pwritev_part(
             qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
                               cluster_data, cur_bytes);
 
-            if (qcow2_co_encrypt(bs, cluster_offset, offset,
+            if (qcow2_co_encrypt(bs, cluster_offset + offset_in_cluster, offset,
                                  cluster_data, cur_bytes) < 0) {
                 ret = -EIO;
                 goto out_unlocked;
diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..a488d761ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -758,10 +758,10 @@  ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
                     const void *src, size_t src_size);
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len);
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len);
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len);
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
+                 uint64_t guest_offset, void *buf, size_t len);
 
 #endif