Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > This is also a preparation for key read/write/erase functions
> > 
> > * use master key len from the header
> > * prefer to use crypto params in the QCryptoBlockLUKS
> >   over passing them as function arguments
> > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> > * Add comments to various crypto parameters in the QCryptoBlockLUKS
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block-luks.c | 213 ++++++++++++++++++++++----------------------
> >  1 file changed, 105 insertions(+), 108 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 409ab50f20..48213abde7 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> 
> [...]
> 
> > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592);
> >  struct QCryptoBlockLUKS {
> >      QCryptoBlockLUKSHeader header;
> >  
> > -    /* Cache parsed versions of what's in header fields,
> > -     * as we can't rely on QCryptoBlock.cipher being
> > -     * non-NULL */
> 
> Hm, why remove this comment?

Because the intended uses of these fields changed.
As Daniel explained to me initially none of the crypto parameters
were stored in the QCryptoBlockLUKS, and thus there were all passed
as function arguments.
Later when qemu-img info was added/implemented, there was need to 'cache' them
in the header so that info command could use them after image was opened.

Now after my changes this is no longer true. now these crypto parameters are set early
on create/load and used everywhere to avoid passing them over and over to each
function.

> 
> > +    /* Main encryption algorithm used for encryption*/
> >      QCryptoCipherAlgorithm cipher_alg;
> > +
> > +    /* Mode of encryption for the selected encryption algorithm */
> >      QCryptoCipherMode cipher_mode;
> > +
> > +    /* Initialization vector generation algorithm */
> >      QCryptoIVGenAlgorithm ivgen_alg;
> > +
> > +    /* Hash algorithm used for IV generation*/
> >      QCryptoHashAlgorithm ivgen_hash_alg;
> > +
> > +    /*
> > +     * Encryption algorithm used for IV generation.
> > +     * Usually the same as main encryption algorithm
> > +     */
> > +    QCryptoCipherAlgorithm ivgen_cipher_alg;
> > +
> > +    /* Hash algorithm used in pbkdf2 function */
> >      QCryptoHashAlgorithm hash_alg;
> >  };
> >  
> > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> >      }
> >  }
> >  
> > +static int masterkeylen(QCryptoBlockLUKS *luks)
> 
> This should be a const pointer.
I haven't had const in mind while writing this but you are right.
Fixed.


> 
> > +{
> > +    return luks->header.key_bytes;
> > +}
> > +
> > +
> >  /*
> >   * Given a key slot, and user password, this will attempt to unlock
> >   * the master encryption key from the key slot.
> > @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> >   */
> >  static int
> >  qcrypto_block_luks_load_key(QCryptoBlock *block,
> > -                            QCryptoBlockLUKSKeySlot *slot,
> > +                            uint slot_idx,
> 
> Did you use uint on purpose or do you mean a plain “unsigned”?
Well there are just 8 slots, but yea I don't mind to make this an unsigned int.


> 
> >                              const char *password,
> > -                            QCryptoCipherAlgorithm cipheralg,
> > -                            QCryptoCipherMode ciphermode,
> > -                            QCryptoHashAlgorithm hash,
> > -                            QCryptoIVGenAlgorithm ivalg,
> > -                            QCryptoCipherAlgorithm ivcipheralg,
> > -                            QCryptoHashAlgorithm ivhash,
> >                              uint8_t *masterkey,
> > -                            size_t masterkeylen,
> >                              QCryptoBlockReadFunc readfunc,
> >                              void *opaque,
> >                              Error **errp)
> >  {
> >      QCryptoBlockLUKS *luks = block->opaque;
> > +    QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
> 
> I think this is a great opportunity to make this a const pointer.
Agree. Done.
> 
> >      uint8_t *splitkey;
> >      size_t splitkeylen;
> >      uint8_t *possiblekey;
> 
> [...]
> 
> > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> >          goto fail;
> >      }
> >  
> > +    cipher_mode = g_strdup(luks->header.cipher_mode);
> > +
> 
> This should be freed under the fail label.
> 
> (And maybe the fact that this no longer modifies
> luks->header.cipher_mode should be mentioned in the commit message, I
> don’t know.)

I swear I documented that in the commit message... yea in the next commit (:-()
Fixed that now.

> 
> >      /*
> >       * The cipher_mode header contains a string that we have
> >       * to further parse, of the format
> 
> [...]
> 
> > @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> >  
> >      ivhash_name = strchr(ivgen_name, ':');
> >      if (!ivhash_name) {
> > -        ivhash = 0;
> > +        luks->ivgen_hash_alg = 0;
> 
> *luks is initialized to 0 anyway, but it doesn’t hurt, of course.
> 
> >      } else {
> >          *ivhash_name = '\0';
> >          ivhash_name++;
> >  
> > -        ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name,
> > -                                                     &local_err);
> > +        luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name,
> > +                                                                   &local_err);
> >          if (local_err) {
> >              ret = -ENOTSUP;
> >              error_propagate(errp, local_err);
> > @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> 
> [...]
> 
> >  
> > -    hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> > +    luks->hash_alg =
> > +            qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> >                                                 &local_err);
> 
> Indentation is off now.
True! But at least as you probably noticed I now am aware of that rule, and
so I have way less such errors now :-)

> 
> >      if (local_err) {
> >          ret = -ENOTSUP;
> 
> [...]
> 
> > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >              luks_opts.has_ivgen_hash_alg = true;
> >          }
> >      }
> > +
> > +    luks = g_new0(QCryptoBlockLUKS, 1);
> > +    block->opaque = luks;
> > +
> > +    luks->cipher_alg = luks_opts.cipher_alg;
> > +    luks->cipher_mode = luks_opts.cipher_mode;
> > +    luks->ivgen_alg = luks_opts.ivgen_alg;
> > +    luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg;
> > +    luks->hash_alg = luks_opts.hash_alg;
> > +
> > +
> 
> Why did you pull this up?  Now @luks is leaked in both of the next error
> paths.

This is because the purpose of these fields changed. As Daniel explained to me
they were initially added after the fact to serve as a cache of into to present in qemu-img info callback.
But now I use these everywhere in the code so I won't them to be correct as soon as possible to minimize
the risk of calling some function that uses these and would read garbage.

Leak is fixed now, thanks!


> 
> >      /* Note we're allowing ivgen_hash_alg to be set even for
> >       * non-essiv iv generators that don't need a hash. It will
> >       * be silently ignored, for compatibility with dm-crypt */
> 
> [...]
> 
> > @@ -1003,6 +1004,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >          ivcipheralg = luks_opts.cipher_alg;
> >      }
> >  
> > +    luks->ivgen_cipher_alg = ivcipheralg;
> > +
> 
> What’s the point in having a dedicated ivcipheralg variable then?


Leftover, since I added the luks->ivgen_cipher_alg.

The luks->ivgen_cipher_alg is special in the sense that it is not
part of the parsed on-disk header, but rather calculated,
as a workaround for the dm-crypto 'bug'

The long explanation of this is in comment of qcrypto_block_luks_essiv_cipher

I'll remove that local variable, because why not.

> 
> Max
> 
> >      strcpy(luks->header.cipher_name, cipher_alg);
> >      strcpy(luks->header.cipher_mode, cipher_mode_spec);
> >      strcpy(luks->header.hash_spec, hash_alg);
> 
> 


Best regards,
Thanks for the review,
	Maxim Levitsky