| Message ID | 20190913172741.5662-3-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix qcow2+luks corruption introduced by commit 8ac0f15f335 | expand |
13.09.2019 20:27, Maxim Levitsky wrote: > Change the 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. > > Remove the do_perform_cow_encrypt by merging it with > qcow2_co_encrypt > > Also document the qcow2_co_encrypt arguments to prevent > that bug from happening again > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-cluster.c | 35 +++++-------------- > block/qcow2-threads.c | 81 ++++++++++++++++++++++++++++++++++++------- > block/qcow2.c | 5 +-- > block/qcow2.h | 8 ++--- > 4 files changed, 83 insertions(+), 46 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index bfeb0241d7..f42b8a404c 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, > return 0; > } > > -static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, > - uint64_t src_cluster_offset, > - uint64_t cluster_offset, > - unsigned offset_in_cluster, > - 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(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) { > - return false; > - } > - } > - return true; > -} > - > static int coroutine_fn do_perform_cow_write(BlockDriverState *bs, > uint64_t cluster_offset, > unsigned offset_in_cluster, > @@ -891,11 +869,14 @@ 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, > - start->nb_bytes) || > - !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset, > - end->offset, end_buffer, end->nb_bytes)) { > + if (!qcow2_co_encrypt(bs, > + m->offset + start->offset, > + m->alloc_offset + start->offset, > + start_buffer, start->nb_bytes) || You got caught) second argument qcow2_co_encrypt is host_offset and third is guest_offset. I noticed this difference when reviewing previous version, and it's actually additional reason to drop the function. Unfortunately, I didn't said. Also, we can use return value of qcow2_co_encrypt instead of dropping it, something like classical ret = qcow2_co_encrypt(..) if (ret < 0) { goto fail; } ret = qcow2_co_encrypt(..) if (ret < 0) { goto fail; } Also, IMHO, too much changes too keep reviewed-by mark. Still, maybe it's my "with it least fixed" is misleading, may be interpreted like removing the function and corresponding refactoring is included.. > + !qcow2_co_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..b31d45fb2b 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,73 @@ 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_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); > + > + BDRVQcow2State *s = bs->opaque; > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); > + assert(s->crypto); Hmm, better to move these asertions to _encdec I think, and don't duplicate them in two functions. > + > + if (!len) { > + return 0; > + } Aha, that's why you don't check this in perform_cow. That's OK, but still, better keep it in _encdec and don't duplicate. > + > + 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); > + BDRVQcow2State *s = bs->opaque; > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); > + assert(s->crypto); > + > + if (!len) { > + return 0; > + } > + > + 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 17:51 +0000, Vladimir Sementsov-Ogievskiy wrote: > 13.09.2019 20:27, Maxim Levitsky wrote: > > Change the 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. > > > > Remove the do_perform_cow_encrypt by merging it with > > qcow2_co_encrypt > > > > Also document the qcow2_co_encrypt arguments to prevent > > that bug from happening again > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > --- > > block/qcow2-cluster.c | 35 +++++-------------- > > block/qcow2-threads.c | 81 ++++++++++++++++++++++++++++++++++++------- > > block/qcow2.c | 5 +-- > > block/qcow2.h | 8 ++--- > > 4 files changed, 83 insertions(+), 46 deletions(-) > > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > > index bfeb0241d7..f42b8a404c 100644 > > --- a/block/qcow2-cluster.c > > +++ b/block/qcow2-cluster.c > > @@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, > > return 0; > > } > > > > -static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, > > - uint64_t src_cluster_offset, > > - uint64_t cluster_offset, > > - unsigned offset_in_cluster, > > - 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(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) { > > - return false; > > - } > > - } > > - return true; > > -} > > - > > static int coroutine_fn do_perform_cow_write(BlockDriverState *bs, > > uint64_t cluster_offset, > > unsigned offset_in_cluster, > > @@ -891,11 +869,14 @@ 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, > > - start->nb_bytes) || > > - !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset, > > - end->offset, end_buffer, end->nb_bytes)) { > > + if (!qcow2_co_encrypt(bs, > > + m->offset + start->offset, > > + m->alloc_offset + start->offset, > > + start_buffer, start->nb_bytes) || > > You got caught) Ha!, I am just too tired. > second argument qcow2_co_encrypt is host_offset and third is guest_offset. > > I noticed this difference when reviewing previous version, and it's actually > additional reason to drop the function. Unfortunately, I didn't said. > > Also, we can use return value of qcow2_co_encrypt instead of dropping it, > > something like classical > > ret = qcow2_co_encrypt(..) > if (ret < 0) { > goto fail; > } > > ret = qcow2_co_encrypt(..) > if (ret < 0) { > goto fail; > } > > Also, IMHO, too much changes too keep reviewed-by mark. Still, maybe it's my > "with it least fixed" is misleading, may be interpreted like removing the > function and corresponding refactoring is included.. Yea. So V7 there will be... Re-factoring is hard (but worth it) :-( > > > > + !qcow2_co_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..b31d45fb2b 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,73 @@ 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_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); > > + > > + BDRVQcow2State *s = bs->opaque; > > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); > > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); > > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); > > + assert(s->crypto); > > Hmm, better to move these asertions to _encdec I think, and don't duplicate > them in two functions. OK. > > > + > > + if (!len) { > > + return 0; > > + } > > Aha, that's why you don't check this in perform_cow. That's OK, > but still, better keep it in _encdec and don't duplicate. OK. > > > + > > + 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); > > + BDRVQcow2State *s = bs->opaque; > > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); > > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); > > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); > > + assert(s->crypto); > > + > > + if (!len) { > > + return 0; > > + } > > + > > + 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 > > Best regards, Maxim Levitsky
and a bit about empty lines 13.09.2019 20:27, Maxim Levitsky wrote: > Change the 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. > > Remove the do_perform_cow_encrypt by merging it with > qcow2_co_encrypt > > Also document the qcow2_co_encrypt arguments to prevent > that bug from happening again > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- [..] > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c > index 3b1e63fe41..b31d45fb2b 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; > + Don't see real benefit in separating one variable initialization from another one here, but I don't mind, it's OK of course. > 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,73 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset, > return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg); > } > > + Hmm, in this file doubled empty lines are used to separate "Compression" and "Cryptography" sections. And all other splits are one-empty-line. > +/* > + * 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_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); > + This empty line is not needed for sure > + BDRVQcow2State *s = bs->opaque; But it's common practice to split variables definition from code by empty line here > + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); > + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); > + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); > + assert(s->crypto); > + > + if (!len) { > + return 0; > + } > + > + return qcow2_co_encdec(bs, host_offset, guest_offset, buf, len, > + qcrypto_block_encrypt); > } > > + IMHO extra one (see above) > +/* > + * qcow2_co_decrypt() > + * > + * Decrypts one or more contiguous aligned sectors > + * Similar to qcow2_co_encrypt > + */ > + Here you replaced one extra empty line by another > 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);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index bfeb0241d7..f42b8a404c 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -462,28 +462,6 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, return 0; } -static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, - uint64_t src_cluster_offset, - uint64_t cluster_offset, - unsigned offset_in_cluster, - 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(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) { - return false; - } - } - return true; -} - static int coroutine_fn do_perform_cow_write(BlockDriverState *bs, uint64_t cluster_offset, unsigned offset_in_cluster, @@ -891,11 +869,14 @@ 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, - start->nb_bytes) || - !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset, - end->offset, end_buffer, end->nb_bytes)) { + if (!qcow2_co_encrypt(bs, + m->offset + start->offset, + m->alloc_offset + start->offset, + start_buffer, start->nb_bytes) || + !qcow2_co_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..b31d45fb2b 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,73 @@ 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_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); + + BDRVQcow2State *s = bs->opaque; + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); + assert(s->crypto); + + if (!len) { + return 0; + } + + 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); + BDRVQcow2State *s = bs->opaque; + assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE)); + assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE)); + assert(QEMU_IS_ALIGNED(len, BDRV_SECTOR_SIZE)); + assert(s->crypto); + + if (!len) { + return 0; + } + + 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