| Message ID | 20190912223754.875-2-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix qcow2+luks corruption introduced by commit 8ac0f15f335 | expand |
On 13.09.19 00:37, Maxim Levitsky wrote: > This commit tries to clarify few function arguments, > and add comments describing the encrypt/decrypt interface > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > block/qcow2-cluster.c | 8 +++--- > block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++--------- > 2 files changed, 54 insertions(+), 17 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index f09cc992af..b95e64c237 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -463,8 +463,8 @@ 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, > + uint64_t guest_cluster_offset, > + uint64_t host_cluster_offset, > unsigned offset_in_cluster, > uint8_t *buffer, > unsigned bytes) > @@ -474,8 +474,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, > assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0); > assert((bytes & ~BDRV_SECTOR_MASK) == 0); > assert(s->crypto); > - if (qcow2_co_encrypt(bs, cluster_offset, > - src_cluster_offset + offset_in_cluster, > + if (qcow2_co_encrypt(bs, host_cluster_offset, > + guest_cluster_offset + offset_in_cluster, > buffer, bytes) < 0) { > return false; > } > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c > index 3b1e63fe41..6da1838e95 100644 > --- a/block/qcow2-threads.c > +++ b/block/qcow2-threads.c > @@ -234,15 +234,19 @@ 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_cluster_offset, > + uint64_t guest_offset, void *buf, size_t len, > + Qcow2EncDecFunc func) > { > BDRVQcow2State *s = bs->opaque; > + > + uint64_t offset = s->crypt_physical_offset ? > + host_cluster_offset + offset_into_cluster(s, guest_offset) : > + guest_offset; > + > Qcow2EncDecData arg = { > .block = s->crypto, > - .offset = s->crypt_physical_offset ? > - file_cluster_offset + offset_into_cluster(s, offset) : > - offset, > + .offset = offset, > .buf = buf, > .len = len, > .func = func, > @@ -251,18 +255,51 @@ 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_cluster_offset - underlying storage offset of the first cluster > + * in which the encrypted data will be written > + * Used as an initialization vector for encryption s/as an/for generating the/ (AFAIU) > + * > + * @guest_offset - guest (virtual) offset of the first sector of the > + * data to be encrypted > + * Used as an initialization vector for older, qcow2 native encryption I wouldn’t be so specific here. I think it’d be better to just have a common sentence like “Depending on the encryption method, either of these offsets may be used for generating the initialization vector for encryption.” Well, technically, host_cluster_offset will not be used itself. Before we can use it, of course we have to add the in-cluster offset to it (which qcow2_co_encdec() does). This makes me wonder whether it wouldn’t make more sense to pass a host_offset instead of a host_cluster_offset (i.e. make the callers add the in-cluster offset to the host offset already)? > + * > + * @buf - buffer with the data to encrypt, that after encryption > + * will be written to the underlying storage device at > + * @host_cluster_offset I think just “buffer with the data to encrypt” is sufficient. (The rest is just the same as above.) > + * > + * @len - length of the buffer (in sector size multiplies) “In sector size multiples” to me means that it is a sector count (in that “one sector size multiple” is equal to 512 byes). Maybe “must be a BDRV_SECTOR_SIZE multiple” instead? > + * > + * Note that the group of the sectors, don't have to be aligned > + * on cluster boundary and can also cross a cluster boundary. Maybe “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”? (“The group of sectors” sounds a bit weird to me. I don’t quite know, why. I think that for some reason it makes me think of a non-continuous set of sectors. Also, the caller doesn’t pass sector indices, but byte offsets, that just happen to have to be aligned to sectors. (I suppose because that’s the simplest way to make it a multiple of the encryption block 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_cluster_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_cluster_offset, guest_offset, buf, len, > + qcrypto_block_encrypt); > } > > + > +/* > + * qcow2_co_decrypt() > + * > + * Decrypts one or more contiguous aligned sectors > + * Similar to qcow2_co_encrypt Hm. This would make me wonder in what way it is only similar to qcow2_co_encrypt(). Sure, it decrypts instead of encrypting, but maybe there’s more...? So maybe “Its interface is the same as qcow2_co_encrypt()'s”? Max > + * > + */ > + > 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_cluster_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_cluster_offset, guest_offset, buf, len, > + qcrypto_block_decrypt); > } >
On Fri, 2019-09-13 at 12:37 +0200, Max Reitz wrote: > On 13.09.19 00:37, Maxim Levitsky wrote: > > This commit tries to clarify few function arguments, > > and add comments describing the encrypt/decrypt interface > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > block/qcow2-cluster.c | 8 +++--- > > block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++--------- > > 2 files changed, 54 insertions(+), 17 deletions(-) > > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > > index f09cc992af..b95e64c237 100644 > > --- a/block/qcow2-cluster.c > > +++ b/block/qcow2-cluster.c > > @@ -463,8 +463,8 @@ 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, > > + uint64_t guest_cluster_offset, > > + uint64_t host_cluster_offset, > > unsigned offset_in_cluster, > > uint8_t *buffer, > > unsigned bytes) > > @@ -474,8 +474,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, > > assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0); > > assert((bytes & ~BDRV_SECTOR_MASK) == 0); > > assert(s->crypto); > > - if (qcow2_co_encrypt(bs, cluster_offset, > > - src_cluster_offset + offset_in_cluster, > > + if (qcow2_co_encrypt(bs, host_cluster_offset, > > + guest_cluster_offset + offset_in_cluster, > > buffer, bytes) < 0) { > > return false; > > } > > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c > > index 3b1e63fe41..6da1838e95 100644 > > --- a/block/qcow2-threads.c > > +++ b/block/qcow2-threads.c > > @@ -234,15 +234,19 @@ 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_cluster_offset, > > + uint64_t guest_offset, void *buf, size_t len, > > + Qcow2EncDecFunc func) > > { > > BDRVQcow2State *s = bs->opaque; > > + > > + uint64_t offset = s->crypt_physical_offset ? > > + host_cluster_offset + offset_into_cluster(s, guest_offset) : > > + guest_offset; > > + > > Qcow2EncDecData arg = { > > .block = s->crypto, > > - .offset = s->crypt_physical_offset ? > > - file_cluster_offset + offset_into_cluster(s, offset) : > > - offset, > > + .offset = offset, > > .buf = buf, > > .len = len, > > .func = func, > > @@ -251,18 +255,51 @@ 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_cluster_offset - underlying storage offset of the first cluster > > + * in which the encrypted data will be written > > + * Used as an initialization vector for encryption > > s/as an/for generating the/ > > (AFAIU) Yes, this is better. > > > + * > > + * @guest_offset - guest (virtual) offset of the first sector of the > > + * data to be encrypted > > + * Used as an initialization vector for older, qcow2 native encryption > > I wouldn’t be so specific here. I think it’d be better to just have a > common sentence like “Depending on the encryption method, either of > these offsets may be used for generating the initialization vector for > encryption.” Nothing against this either. > > Well, technically, host_cluster_offset will not be used itself. Before > we can use it, of course we have to add the in-cluster offset to it > (which qcow2_co_encdec() does). > > This makes me wonder whether it wouldn’t make more sense to pass a > host_offset instead of a host_cluster_offset (i.e. make the callers add > the in-cluster offset to the host offset already)? This is what I wanted to do in first place, and it would be the cleanest solution, but that would need to update the 3rd caller of this function, the qcow2_co_pwritev_part, which does pass the cluster offset and guest full offset. You know what, I'll just do it. A bit more changes, but much cleaner code that eliminates the possibility of this bug of happening again. > > > + * > > + * @buf - buffer with the data to encrypt, that after encryption > > + * will be written to the underlying storage device at > > + * @host_cluster_offset > > I think just “buffer with the data to encrypt” is sufficient. (The rest > is just the same as above.) I wrote it to clarify this since Vladimir told me that its not clear enough. Note though that I made a mistake here since the data will be written not at host_cluster_offset but in host_cluster_offset + offset_into_cluster(s, guest_offset > + * @len - length of the buffer (in sector size multiplies) > > “In sector size multiples” to me means that it is a sector count (in > that “one sector size multiple” is equal to 512 byes). > > Maybe “must be a BDRV_SECTOR_SIZE multiple” instead? All right. > > > + * > > + * Note that the group of the sectors, don't have to be aligned > > + * on cluster boundary and can also cross a cluster boundary. > > Maybe “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”? > > (“The group of sectors” sounds a bit weird to me. I don’t quite know, > why. I think that for some reason it makes me think of a non-continuous > set of sectors. Also, the caller doesn’t pass sector indices, but byte > offsets, that just happen to have to be aligned to sectors. (I suppose > because that’s the simplest way to make it a multiple of the encryption > block size.)) All right, this sounds better > > > + * > > + */ > > 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_cluster_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_cluster_offset, guest_offset, buf, len, > > + qcrypto_block_encrypt); > > } > > > > + > > +/* > > + * qcow2_co_decrypt() > > + * > > + * Decrypts one or more contiguous aligned sectors > > + * Similar to qcow2_co_encrypt > > Hm. This would make me wonder in what way it is only similar to > qcow2_co_encrypt(). Sure, it decrypts instead of encrypting, but maybe > there’s more...? I don't think so, since we have symmetrical encryption here. > > So maybe “Its interface is the same as qcow2_co_encrypt()'s”? That would be lawyer territory, since interface is not technically the same, since it decrypts rather that encrypts the data in the buffer... I think that this wording should be good enough. > > Max > > > + * > > + */ > > + > > 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_cluster_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_cluster_offset, guest_offset, buf, len, > > + qcrypto_block_decrypt); > > } > > > > Best regards, Maxim Levitsky
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index f09cc992af..b95e64c237 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -463,8 +463,8 @@ 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, + uint64_t guest_cluster_offset, + uint64_t host_cluster_offset, unsigned offset_in_cluster, uint8_t *buffer, unsigned bytes) @@ -474,8 +474,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs, assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0); assert((bytes & ~BDRV_SECTOR_MASK) == 0); assert(s->crypto); - if (qcow2_co_encrypt(bs, cluster_offset, - src_cluster_offset + offset_in_cluster, + if (qcow2_co_encrypt(bs, host_cluster_offset, + guest_cluster_offset + offset_in_cluster, buffer, bytes) < 0) { return false; } diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c index 3b1e63fe41..6da1838e95 100644 --- a/block/qcow2-threads.c +++ b/block/qcow2-threads.c @@ -234,15 +234,19 @@ 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_cluster_offset, + uint64_t guest_offset, void *buf, size_t len, + Qcow2EncDecFunc func) { BDRVQcow2State *s = bs->opaque; + + uint64_t offset = s->crypt_physical_offset ? + host_cluster_offset + offset_into_cluster(s, guest_offset) : + guest_offset; + Qcow2EncDecData arg = { .block = s->crypto, - .offset = s->crypt_physical_offset ? - file_cluster_offset + offset_into_cluster(s, offset) : - offset, + .offset = offset, .buf = buf, .len = len, .func = func, @@ -251,18 +255,51 @@ 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_cluster_offset - underlying storage offset of the first cluster + * in which the encrypted data will be written + * Used as an initialization vector for encryption + * + * @guest_offset - guest (virtual) offset of the first sector of the + * data to be encrypted + * Used as an initialization vector for older, qcow2 native encryption + * + * @buf - buffer with the data to encrypt, that after encryption + * will be written to the underlying storage device at + * @host_cluster_offset + * + * @len - length of the buffer (in sector size multiplies) + * + * Note that the group of the sectors, don't have to be aligned + * on cluster boundary and can also cross a cluster boundary. + * + */ 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_cluster_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_cluster_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_cluster_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_cluster_offset, guest_offset, buf, len, + qcrypto_block_decrypt); }
This commit tries to clarify few function arguments, and add comments describing the encrypt/decrypt interface Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- block/qcow2-cluster.c | 8 +++--- block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 54 insertions(+), 17 deletions(-)