On Tue, 2019-09-10 at 14:17 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 10.09.2019 15:31, Maxim Levitsky wrote:
> > On Sat, 2019-09-07 at 19:08 +0000, Vladimir Sementsov-Ogievskiy wrote:
> > > 06.09.2019 22:57, Maxim Levitsky wrote:
> > > > This commit tries to clarify few function arguments,
> > > > and add comments describing the encrypt/decrypt interface
> > > >
> > > > Signed-off-by: Maxim Levitsky <mlevi...@redhat.com>
> > > > ---
> > > > block/qcow2-cluster.c | 10 +++----
> > > > block/qcow2-threads.c | 61
> > > > ++++++++++++++++++++++++++++++++++---------
> > > > 2 files changed, 53 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > > index f09cc992af..1989b423da 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;
> > > > }
> > > > @@ -496,7 +496,7 @@ static int coroutine_fn
> > > > do_perform_cow_write(BlockDriverState *bs,
> > > > }
> > > >
> > > > ret = qcow2_pre_write_overlap_check(bs, 0,
> > > > - cluster_offset + offset_in_cluster, qiov->size, true);
> > > > + cluster_offset + offset_in_cluster, qiov->size, true);
> > >
> > >
> > > Hmm, unrelated hunk.
> >
> > I was asked to do this to fix coding style, so that wrapped line,
> > is 4 characters shifted to the right.
>
> AFAIS, Eric asked only about qcow2_co_encdec calls and definition.. It's OK
> to fix style in code
> you touch in you patch anyway, but no reason to fix style somewhere else, it
> dirties patch for
> no reason, making it more difficult (a bit, but still) to review and more
> difficult to backport..
All right, then I'll drop that change. I kind of agree with this, but I also
didn't mind doing these fixes
either.
>
> >
> > >
> > > > if (ret < 0) {
> > > > return ret;
> > > > }
> > > > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > > > index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk offset of the first cluster in which
> > > > + * the encrypted data will be written
> > >
> > >
> > > It's not quite right, it's not on disk, but on .file child of qcow2 node,
> > > which
> > > may be any other format or protocol node.. So, I called it
> > > file_cluster_offset.
> > > But I'm OK with new naming anyway. And it may be better for encryption
> > > related
> > > logic..
> >
> > Yes, the .file is the underlying storage for both qcow2 metadata and the
> > data,
> > and it is unlikely be another qcow2 file. Usually it will be a raw file,
> > accessed with some protocol.
> > I will change the wording to not include the 'disk' word though.
> >
> >
> > To be really honest, the best naming here would be one that follows the
> > virtual memory concepts.
> > A virtual block/cluster address and a physical block/cluster address.
> > However we talked with Kevin recently and I also studied quite a lot of
> > qcow2 code,
> > and the usual convention is guest cluster offset and host cluster offset,
> > and often guest offsets are just called offsets, which is very confusing
> > IMHO.
>
> It don't confuse me, as it's an interface of block-layer. Interface user just
> writes at some offset.
> And the fact that internally, we consider interface parameter "offset" as
> "guest offset" and map it
> to some physical offset - it's an implementation details. In other words,
> guest don't know that it
> writes at "guest offset", it just writes at "offset" and don't care.
>
> >
> >
> > >
> > > > + * Used as an initialization vector for encryption
> > >
> > > Hmm, is it default now?
> >
> >
> > Most of block crypto implementations have IV which derive
> > some way or another from the sector address.
> >
> > From what I see, the block address is either used as is,
> > or encrypted itself with same encryption key,
> > and the result is used as IV. Even the legacy qcow
> > encryption uses this, although it uses the virtual block
> > address, and apparently this is one of its security flaws.
> >
> > If you don't use any IV, you end up with major security
> > hole - sectors of the same content will be encrypted
> > to the same cipertext.
> >
> > I added this comment to clarify the usage of offset,
> > since other that this aspect of IV generation,
> > the crypto routines only need the data to be encrypted and
> > the encryption key which is stored in the crypto context.
> >
> >
> > >
> > > > + *
> > > > + * @guest_offset - guest (virtual) offset of the first sector of the
> > > > + * data to be encrypted
> > >
> > > Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not
> > > first sector of
> > > the data to be encrypted, but first sector in which guest writes data (to
> > > be encrypted
> > > in meantime).
> >
> > No no no!
> >
> > guest_offset is literally the guest's disk address of the first sector that
> > is in the buffer.
> > The qcow2_co_encrypt is called from 2 places:
> >
> > 1. qcow2_co_pwritev_part - here indeed the actually guest written data
> > is encrypted, and the 'offset' is passed which is the offset on which
> > pwritev was called
>
> I mean, that in this case your comment for me sounds like "data to be
> encrypted is on disk, at this offset".
> but it's not actually on disk, data is in buffer. And what is on disk we
> don't know, we are going to
> write...
>
> But I don't really care. Hope it's actually obvious for averyone, where is
> data on write path.
Ah, now I understand. I don't mind adding a word or two explicitly mentioning
that the *data* to be
encrypted is in the given buffer.
>
> >
> >
> > 2. do_perform_cow_encrypt - here the just read data from before or after
> > actually written guest
> > data is encrypted, and the guest_offset represents the address of that data.
> > I changed the do_perform_cow_encrypt, so that it receives from the caller
> > the host and guest offset
> > of the data to be encrypted. It then aligns the host offset on start of the
> > cluster, and passes
> > the guest offset as is, so that it does the same as qcow2_co_pwritev_part.
> >
> >
> > So what is wrong here?
> >
> > >
> > >
> > > > + * Used as an initialization vector for older, qcow2 native encryption
> > > > + *
> > > > + * @buf - buffer with the data to encrypt
> > > > + * @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.
> > >
> > > And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a
> > > group
> > > of the sectors crossing a cluster boundary, we will finish up with
> > > similar bug: we'll
> > > use first cluster offset as a vector for all the sectors. We still never
> > > do it.. So,
> > > I think it worth assertion and corresponding comment.
> > >
> > > Or is it correct?
> >
> > Crypto code receives the data to be encrypted and decrypted,
> > and offset of first sector (512 bytes aligned) of that data (for IV
> > calculation).
> > If the data spans multiple sectors (this happens already a lot)
>
> Ah, yes, that's OK, sorry.
>
> > then it able to handle this since the code works.
> > The relevant code is in do_qcrypto_block_cipher_encdec, and
> > is actually generic for all the crypto modes.
> >
> > So there should not be anything special about crossing the cluster
> > boundary, other that noting this here, just in case.
> >
> > No only that code can cross a cluster boundary, but it can even be
> > called for more that one cluster at once. It looks like qcow2 code limits
> > all
> > the IO in case crypto is used to QCOW_MAX_CRYPT_CLUSTERS, which is 32
> > currently. I don't know why to be honest.
> >
> > Remember that crypto code has no notion of clusters. It works purely
> > on sector (512 bytes) level, and each sector will have its own IV
> > calculated,
> > based on its sector address.
> >
> > >
> > > > + *
> > > > + *
> > > > + */
> > > > 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
> > > > + * Same function as qcow2_co_encrypt
> > >
> > > Hmm, not exactly same :)
> >
> > I'll fix that.
> >
> >
> > >
> > > > + *
> > > > + */
> > > > +
> > > > 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
