QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Maxim Levitsky <mlevitsk@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: Tue, 20 Aug 2019 19:36:47 +0200
Message-ID: <4d242249-f823-7465-4b31-64fce484679f@redhat.com> (raw)
In-Reply-To: <20190814202219.1870-3-mlevitsk@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 6431 bytes --]

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?

> +    /* 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.

> +{
> +    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”?

>                              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.

>      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.)

>      /*
>       * 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.

>      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.

>      /* 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?

Max

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  parent 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   ` Max Reitz [this message]
2019-08-21 23:59     ` [Qemu-devel] " Maxim Levitsky
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=4d242249-f823-7465-4b31-64fce484679f@redhat.com \
    --to=mreitz@redhat.com \
    --cc=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mlevitsk@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