| Message ID | 20190906173201.7926-2-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix qcow2+luks corruption introduced by commit 8ac0f15f335 | expand |
On 9/6/19 12:31 PM, 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 | 53 ++++++++++++++++++++++++++++++++++--------- > 2 files changed, 46 insertions(+), 15 deletions(-) > > +++ 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) Pre-existing bug in alignment... > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset, > + uint64_t guest_offset, void *buf, size_t len, > + Qcow2EncDecFunc func) ...so this would be a great time to fix it. > { > 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, I'm ambivalent on whether the new 'offset' variable gains us anything. But it doesn't hurt. > .buf = buf, > .len = len, > .func = func, > @@ -251,18 +255,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset, > return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg); > } > > + > +/* > + * qcow2_co_encrypt() > + * > + * Encrypts a sector size aligned contiguous area > + * > + * @host_cluster_offset - on disk offset of the cluster in which > + * the buffer resides > + * > + * @guest_offset - guest (virtual) offset of the buffer > + * @buf - buffer with the data to encrypt > + * @len - length of the buffer > + * > + * Note that the area is not cluster aligned and might cross a cluster > + * boundary Umm, how is it possible for a sector to cross a cluster boundary? All clusters are sector-aligned, and encryption only works on aligned sectors. Oh, I see - if @len is a multiple larger than sector size, then we have multiple sectors, and then indeed we may cross clusters. But then the docs about being 'a sector size aligned contiguous area' is not quite right. > + * > + * > + */ > 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, > + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len, > qcrypto_block_encrypt); Another alignment worth fixing up while in the area. > } > > + > +/* > + * qcow2_co_decrypt() > + * > + * Decrypts a sector size aligned contiguous area > + * Same function as 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, > + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len, > qcrypto_block_decrypt); and again. > } >
On Fri, 2019-09-06 at 13:00 -0500, Eric Blake wrote: > On 9/6/19 12:31 PM, 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 | 53 ++++++++++++++++++++++++++++++++++--------- > > 2 files changed, 46 insertions(+), 15 deletions(-) > > > > +++ 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) > > Pre-existing bug in alignment... No problem will fix. > > > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset, > > + uint64_t guest_offset, void *buf, size_t len, > > + Qcow2EncDecFunc func) > > ...so this would be a great time to fix it. > > > { > > 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, > > I'm ambivalent on whether the new 'offset' variable gains us anything. > But it doesn't hurt. I added it, so that I won't need to wrap the lines even more that they are wrapped already since I increased the length of the parameter names a bit. > > > > .buf = buf, > > .len = len, > > .func = func, > > @@ -251,18 +255,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset, > > return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg); > > } > > > > + > > +/* > > + * qcow2_co_encrypt() > > + * > > + * Encrypts a sector size aligned contiguous area > > + * > > + * @host_cluster_offset - on disk offset of the cluster in which > > + * the buffer resides > > + * > > + * @guest_offset - guest (virtual) offset of the buffer > > + * @buf - buffer with the data to encrypt > > + * @len - length of the buffer > > + * > > + * Note that the area is not cluster aligned and might cross a cluster > > + * boundary > > Umm, how is it possible for a sector to cross a cluster boundary? All > clusters are sector-aligned, and encryption only works on aligned > sectors. Oh, I see - if @len is a multiple larger than sector size, > then we have multiple sectors, and then indeed we may cross clusters. > But then the docs about being 'a sector size aligned contiguous area' is > not quite right. Why? the written area is always both aligned on _sector_ boundary and multiple of the sector size. At least that what I see from the existing asserts. > > > + * > > + * > > + */ > > 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, > > + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len, > > qcrypto_block_encrypt); > > > Another alignment worth fixing up while in the area. > > > } > > > > + > > +/* > > + * qcow2_co_decrypt() > > + * > > + * Decrypts a sector size aligned contiguous area > > + * Same function as 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, > > + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len, > > qcrypto_block_decrypt); > > and again. > > > } > > > > Best regards, Thanks for the review, Maxim Levitsky
On 9/6/19 1:55 PM, Maxim Levitsky wrote: >>> +/* >>> + * qcow2_co_encrypt() >>> + * >>> + * Encrypts a sector size aligned contiguous area >>> + * >>> + * @host_cluster_offset - on disk offset of the cluster in which >>> + * the buffer resides >>> + * >>> + * @guest_offset - guest (virtual) offset of the buffer >>> + * @buf - buffer with the data to encrypt >>> + * @len - length of the buffer >>> + * >>> + * Note that the area is not cluster aligned and might cross a cluster >>> + * boundary >> >> Umm, how is it possible for a sector to cross a cluster boundary? All >> clusters are sector-aligned, and encryption only works on aligned >> sectors. Oh, I see - if @len is a multiple larger than sector size, >> then we have multiple sectors, and then indeed we may cross clusters. >> But then the docs about being 'a sector size aligned contiguous area' is >> not quite right. > > Why? the written area is always both aligned on _sector_ boundary > and multiple of the sector size. At least that what I see from > the existing asserts. I'm thinking it should read something like: Encrypts one or more contiguous aligned sectors to make it obvious that because there can be multiple sectors, there may indeed be a cluster boundary mid-length. My complaint was that the original text made it sound like there was exactly one sector (at which point @len is redundant, unless sectors are variably-sized)
On Fri, 2019-09-06 at 14:00 -0500, Eric Blake wrote: > On 9/6/19 1:55 PM, Maxim Levitsky wrote: > > > > > +/* > > > > + * qcow2_co_encrypt() > > > > + * > > > > + * Encrypts a sector size aligned contiguous area > > > > + * > > > > + * @host_cluster_offset - on disk offset of the cluster in which > > > > + * the buffer resides > > > > + * > > > > + * @guest_offset - guest (virtual) offset of the buffer > > > > + * @buf - buffer with the data to encrypt > > > > + * @len - length of the buffer > > > > + * > > > > + * Note that the area is not cluster aligned and might cross a cluster > > > > + * boundary > > > > > > Umm, how is it possible for a sector to cross a cluster boundary? All > > > clusters are sector-aligned, and encryption only works on aligned > > > sectors. Oh, I see - if @len is a multiple larger than sector size, > > > then we have multiple sectors, and then indeed we may cross clusters. > > > But then the docs about being 'a sector size aligned contiguous area' is > > > not quite right. > > > > Why? the written area is always both aligned on _sector_ boundary > > and multiple of the sector size. At least that what I see from > > the existing asserts. > > I'm thinking it should read something like: > > Encrypts one or more contiguous aligned sectors > > to make it obvious that because there can be multiple sectors, there may > indeed be a cluster boundary mid-length. My complaint was that the > original text made it sound like there was exactly one sector (at which > point @len is redundant, unless sectors are variably-sized) > I agree with you completely, I will change that. 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..8bc339690f 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,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset, return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg); } + +/* + * qcow2_co_encrypt() + * + * Encrypts a sector size aligned contiguous area + * + * @host_cluster_offset - on disk offset of the cluster in which + * the buffer resides + * + * @guest_offset - guest (virtual) offset of the buffer + * @buf - buffer with the data to encrypt + * @len - length of the buffer + * + * Note that the area is not cluster aligned and might 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, + return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len, qcrypto_block_encrypt); } + +/* + * qcow2_co_decrypt() + * + * Decrypts a sector size aligned contiguous area + * Same function as 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, + 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 | 53 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 46 insertions(+), 15 deletions(-)