[PATCH 1/9] crypto,fs: Separate out hkdf_extract() and hkdf_expand()

[Hannes Reinecke]

On 10/14/24 21:38, Eric Biggers wrote:
> On Fri, Oct 11, 2024 at 05:54:22PM +0200, Hannes Reinecke wrote:
>> Separate out the HKDF functions into a separate module to
>> to make them available to other callers.
>> And add a testsuite to the module with test vectors
>> from RFC 5869 to ensure the integrity of the algorithm.
> 
> integrity => correctness
> 
Okay.

>> +config CRYPTO_HKDF
>> +	tristate
>> +	select CRYPTO_SHA1 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
>> +	select CRYPTO_SHA256 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
>> +	select CRYPTO_HASH2
> 
> Any thoughts on including SHA512 tests instead of SHA1, given that SHA1 is
> obsolete and should not be used?
> 
Hmm. The original implementation did use SHA1, so I used that.
But sure I can look into changing that.

>> +int hkdf_expand(struct crypto_shash *hmac_tfm,
>> +		const u8 *info, unsigned int infolen,
>> +		u8 *okm, unsigned int okmlen)
>> +{
>> +	SHASH_DESC_ON_STACK(desc, hmac_tfm);
>> +	unsigned int i, hashlen = crypto_shash_digestsize(hmac_tfm);
>> +	int err;
>> +	const u8 *prev = NULL;
>> +	u8 counter = 1;
>> +	u8 tmp[HASH_MAX_DIGESTSIZE];
>> +
>> +	if (WARN_ON(okmlen > 255 * hashlen ||
>> +		    hashlen > HASH_MAX_DIGESTSIZE))
>> +		return -EINVAL;
> 
> The crypto API guarantees HASH_MAX_DIGESTSIZE, so checking that again is not
> very useful.
> 
Okay, will be removing that check.

>> +
>> +	memzero_explicit(tmp, HASH_MAX_DIGESTSIZE);
> 
> The zeroization above is unnecessary.  If it's done anyway, it is just an
> initialization, so it should use an initializer '= {}' instead of
> memzero_explicit() which is intended for "destruction".
> 
Ok.

>> +MODULE_ALIAS_CRYPTO("hkdf");
> 
> This alias does not make sense and is unnecessary.  These are library functions
> that are not exposed by name through the crypto API, so there is no need to wire
> them up to the module autoloading accordingly.
> 
Ok.

>> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
>> new file mode 100644
>> index 000000000000..c1f23a32a6b6
>> --- /dev/null
>> +++ b/include/crypto/hkdf.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
>> + *
>> + * Extracted from fs/crypto/hkdf.c, which has
>> + * Copyright 2019 Google LLC
>> + */
>> +
>> +#ifndef _CRYPTO_HKDF_H
>> +#define _CRYPTO_HKDF_H
>> +
>> +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
>> +		 unsigned int ikmlen, const u8 *salt, unsigned int saltlen,
>> +		 u8 *prk);
>> +int hkdf_expand(struct crypto_shash *hmac_tfm,
>> +		const u8 *info, unsigned int infolen,
>> +		u8 *okm, unsigned int okmlen);
>> +#endif
> 
> This needs to include <crypto/hash.h>.
> 
> Otherwise there will be errors if someone includes this as their first header.
> 
Will do.

Thanks for the 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