| Message ID | 20190913152818.17843-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix qcow2+luks corruption introduced by commit 8ac0f15f335 | expand |
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 >
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
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
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(-)