QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-devel@nongnu.org
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Fam Zheng" <fam@euphon.net>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	qemu-block@nongnu.org, "Markus Armbruster" <armbru@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring
Date: Thu, 22 Aug 2019 02:59:03 +0300
Message-ID: <98bce17338318a95b94e053731864ce1caaeab4c.camel@redhat.com> (raw)
In-Reply-To: <4d242249-f823-7465-4b31-64fce484679f@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




  reply index

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 20:22 [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring Maxim Levitsky
2019-08-20 16:38   ` Max Reitz
2019-08-22  0:05     ` Maxim Levitsky
2019-08-22 14:34       ` Max Reitz
2019-08-22 15:04         ` Maxim Levitsky
2019-08-21 15:39   ` Daniel P. Berrangé
2019-08-22  0:08     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 02/13] qcrypto-luks: " Maxim Levitsky
2019-08-15 21:40   ` [Qemu-devel] [Qemu-block] " John Snow
2019-08-19 14:21     ` Maxim Levitsky
2019-08-22 10:29     ` Daniel P. Berrangé
2019-08-22 11:04       ` Maxim Levitsky
2019-08-22 11:10         ` Daniel P. Berrangé
2019-08-22 11:13           ` Maxim Levitsky
2019-08-20 17:36   ` [Qemu-devel] " Max Reitz
2019-08-21 23:59     ` Maxim Levitsky [this message]
2019-08-22 14:32       ` Max Reitz
2019-08-25 10:46         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions Maxim Levitsky
2019-08-20 18:01   ` Max Reitz
2019-08-21 22:43     ` Maxim Levitsky
2019-08-22 10:32       ` Daniel P. Berrangé
2019-08-22 10:57         ` Maxim Levitsky
2019-08-22 10:34       ` Daniel P. Berrangé
2019-08-25 14:11         ` Maxim Levitsky
2019-08-22 10:38   ` Daniel P. Berrangé
2019-08-25 14:09     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations Maxim Levitsky
2019-08-22 10:47   ` Daniel P. Berrangé
2019-08-25 14:30     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always Maxim Levitsky
2019-08-20 18:12   ` Max Reitz
2019-08-21 22:40     ` Maxim Levitsky
2019-08-22 10:49     ` Daniel P. Berrangé
2019-08-22 10:56       ` Maxim Levitsky
2019-08-25 15:31         ` Maxim Levitsky
2019-08-25 17:15           ` Maxim Levitsky
2019-08-27  8:55           ` Daniel P. Berrangé
2019-08-21 23:01   ` [Qemu-devel] [Qemu-block] " Nir Soffer
2019-08-21 23:11     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous header checking Maxim Levitsky
2019-08-22 11:04   ` Daniel P. Berrangé
2019-08-25 15:40     ` Maxim Levitsky
2019-08-25 16:08       ` Maxim Levitsky
2019-08-26 13:31         ` Eric Blake
2019-08-26 13:39           ` Maxim Levitsky
2019-08-27  8:56         ` Daniel P. Berrangé
2019-08-14 20:22 ` [Qemu-devel] [PATCH 07/13] block: add manage-encryption command (qmp and blockdev) Maxim Levitsky
2019-08-20 18:27   ` Max Reitz
2019-08-21 22:32     ` Maxim Levitsky
2019-08-22 11:14     ` Daniel P. Berrangé
2019-08-21 11:47   ` Markus Armbruster
2019-08-21 22:24     ` Maxim Levitsky
2019-08-22 14:07       ` Markus Armbruster
2019-08-25 16:42         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 08/13] qcrypto: add the plumbing for encryption management Maxim Levitsky
2019-08-22 11:16   ` Daniel P. Berrangé
2019-08-22 11:47     ` Maxim Levitsky
2019-08-22 11:49       ` Daniel P. Berrangé
2019-08-14 20:22 ` [Qemu-devel] [PATCH 09/13] qcrypto-luks: implement the encryption key management Maxim Levitsky
2019-08-22 11:27   ` Daniel P. Berrangé
2019-08-25 17:01     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 10/13] block/crypto: " Maxim Levitsky
2019-08-22 11:29   ` Daniel P. Berrangé
2019-08-22 11:36     ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 11/13] block/qcow2: implement the encryption key managment Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 12/13] qemu-img: implement key management Maxim Levitsky
2019-08-20 18:29   ` Max Reitz
2019-08-21 22:33     ` Maxim Levitsky
2019-08-22 11:32     ` Daniel P. Berrangé
2019-08-22 14:42       ` Max Reitz
2019-08-25 17:04         ` Maxim Levitsky
2019-08-14 20:22 ` [Qemu-devel] [PATCH 13/13] iotests : add tests for encryption " Maxim Levitsky
2019-08-14 21:08 ` [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 " Eric Blake
2019-08-15  8:49   ` Maxim Levitsky
2019-08-15  9:10   ` Kevin Wolf
2019-08-15 14:18     ` Markus Armbruster
2019-08-15 14:44       ` Maxim Levitsky
2019-08-15 15:00         ` Eric Blake
2019-08-19 12:35           ` Maxim Levitsky
2019-08-21 11:31             ` Markus Armbruster
2019-08-21 13:22               ` Maxim Levitsky
2019-08-20 17:59 ` Max Reitz
2019-08-21 22:00   ` Maxim Levitsky
2019-08-22 11:35 ` Daniel P. Berrangé
2019-08-25 17:10   ` Maxim Levitsky

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98bce17338318a95b94e053731864ce1caaeab4c.camel@redhat.com \
    --to=mlevitsk@redhat.com \
    --cc=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox