[PATCH 1/9] crypto,fs: Separate out hkdf_extract() and hkdf_expand()
Hannes Reinecke
hare at suse.de
Mon Jul 22 23:24:09 PDT 2024
On 7/23/24 03:36, Eric Biggers wrote:
> On Mon, Jul 22, 2024 at 04:21:14PM +0200, Hannes Reinecke wrote:
>> diff --git a/crypto/Makefile b/crypto/Makefile
>> index edbbaa3ffef5..b77fc360f0ff 100644
>> --- a/crypto/Makefile
>> +++ b/crypto/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o
>>
>> crypto_hash-y += ahash.o
>> crypto_hash-y += shash.o
>> +crypto_hash-y += hkdf.o
>> obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
>
> This should go under a kconfig option CONFIG_CRYPTO_HKDF that is selected by the
> users that need it. That way the code will be built only when needed.
>
Okay.
> Including a self-test would also be desirable.
>
First time for me, but ok.
>> diff --git a/crypto/hkdf.c b/crypto/hkdf.c
>> new file mode 100644
>> index 000000000000..22e343851c0b
>> --- /dev/null
>> +++ b/crypto/hkdf.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
>> + * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010):
>> + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
>> + *
>> + * This is used to derive keys from the fscrypt master keys.
>
> This is no longer in fs/crypto/, so the part about fscrypt should be removed.
>
Will be removing this line.
>> +/*
>> + * HKDF consists of two steps:
>> + *
>> + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from
>> + * the input keying material and optional salt.
>
> It doesn't make sense to refer to HKDF_HASHLEN here since it is specific to
> fs/crypto/.
>
Ok.
>> +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
>> +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
>> + unsigned int ikmlen, u8 *prk)
>
> Needs kerneldoc now that this is a library interface.
>
Indeed, will be updating the comment.
>> +{
>> + unsigned int prklen = crypto_shash_digestsize(hmac_tfm);
>> + u8 *default_salt;
>> + int err;
>> +
>> + default_salt = kzalloc(prklen, GFP_KERNEL);
>> + if (!default_salt)
>> + return -ENOMEM;
>
> Now that this is a library interface, it should take the salt as a parameter,
> and the users who want the default salt should explicitly specify that. If we
> only provide support for unsalted use, that might inadventently discourage the
> use of a salt in future code. As the function is named hkdf_extract(), people
> might also overlook that it's unsalted and doesn't actually match the RFC's
> definition of HKDF-Extract.
>
> The use of kzalloc here is also inefficient, as the maximum length of a digest
> is known (HKDF_HASHLEN in fs/crypto/ case, HASH_MAX_DIGESTSIZE in general).
>
I was trying to keep the changes to the actual code to a minimum, so I
didn't modify the calling sequence of the original code.
But sure, passing in the 'salt' as a parameter is sensible.
>> + err = crypto_shash_setkey(hmac_tfm, default_salt, prklen);
>> + if (!err)
>> + err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
>> +
>> + kfree(default_salt);
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(hkdf_extract);
>> +
>> +/*
>> + * HKDF-Expand (RFC 5869 section 2.3).
>> + * This expands the pseudorandom key, which was already keyed into @hmac_tfm,
>> + * into @okmlen bytes of output keying material parameterized by the
>> + * application-specific @info of length @infolen bytes.
>> + * This is thread-safe and may be called by multiple threads in parallel.
>> + */
>> +int hkdf_expand(struct crypto_shash *hmac_tfm,
>> + const u8 *info, unsigned int infolen,
>> + u8 *okm, unsigned int okmlen)
>
> Needs kerneldoc now that this is a library interface.
>
Ok.
>> diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
>> index 5a384dad2c72..9c2f9aca9412 100644
>> --- a/fs/crypto/hkdf.c
>> +++ b/fs/crypto/hkdf.c
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
>> * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010):
>> * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
>> *
>> * This is used to derive keys from the fscrypt master keys.
>> *
>> * Copyright 2019 Google LLC
>> */
>
> The above file comment should be adjusted now that this file doesn't contain the
> actual HKDF implementation.
>
Ok.
>> @@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
>> u8 *okm, unsigned int okmlen)
>> {
>> SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
>> - u8 prefix[9];
>> - unsigned int i;
>> + u8 *prefix;
>> int err;
>> - const u8 *prev = NULL;
>> - u8 counter = 1;
>> - u8 tmp[HKDF_HASHLEN];
>>
>> if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN))
>> return -EINVAL;
>>
>> + prefix = kzalloc(okmlen + 9, GFP_KERNEL);
>> + if (!prefix)
>> + return -ENOMEM;
>> desc->tfm = hkdf->hmac_tfm;
>>
>> memcpy(prefix, "fscrypt\0", 8);
>> prefix[8] = context;
>> + memcpy(prefix + 9, info, infolen);
>
> This makes the variable called 'prefix' no longer be the prefix, but rather the
> full info string. A better name for it would be 'full_info'.
>
> Also, it's being allocated with the wrong length. It should be 9 + infolen.
>
Will fix it up.
>> + err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8,
>> + okm, okmlen);
>> + kfree(prefix);
>
> kfree_sensitive()
>
>> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
>> new file mode 100644
>> index 000000000000..bf06c080d7ed
>> --- /dev/null
>> +++ b/include/crypto/hkdf.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
>> + *
>> + * Extracted from fs/crypto/hkdf.c, which has
>> + * Copyright 2019 Google LLC
>> + */
>
> If this is keeping the copyright of fs/crypto/hkdf.c, the license needs to stay
> the same (GPL-2.0, not GPL-2.0-or-later).
>
Will be modifying it.
Thanks a lot for your review!
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare at suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
More information about the Linux-nvme
mailing list