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 <mlevi...@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);
signature.asc
Description: OpenPGP digital signature
