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

[Hannes Reinecke]

On 8/27/24 19:52, Eric Biggers wrote:
> On Tue, Aug 13, 2024 at 01:15:04PM +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.
[ .. ]
>> +	desc->tfm = hmac_tfm;
>> +
>> +	for (i = 0; i < okmlen; i += hashlen) {
>> +
>> +		err = crypto_shash_init(desc);
>> +		if (err)
>> +			goto out;
>> +
>> +		if (prev) {
>> +			err = crypto_shash_update(desc, prev, hashlen);
>> +			if (err)
>> +				goto out;
>> +		}
>> +
>> +		if (info && infolen) {
> 
> 'if (infolen)' instead of 'if (info && infolen)'.  The latter is a bad practice
> because it can hide bugs.
> 
Do I need to set a 'WARN_ON(!info)' (or something) in this case? Or are 
the '->update' callbacks expected to handle it themselves?

[ .. ]
>> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
>> new file mode 100644
>> index 000000000000..ee3e7d21a5fe
>> --- /dev/null
>> +++ b/include/crypto/hkdf.h
>> @@ -0,0 +1,34 @@
>> +/* 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
>> +
>> +#ifdef CONFIG_CRYPTO_HKDF
>> +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);
>> +#else
>> +static inline int hkdf_extract(struct crypto_shash *hmac_tfm,
>> +			       const u8 *ikm, unsigned int ikmlen,
>> +			       const u8 *salt, unsigned int saltlen,
>> +			       u8 *prk)
>> +{
>> +	return -ENOTSUP;
>> +}
>> +static inline int hkdf_expand(struct crypto_shash *hmac_tfm,
>> +			      const u8 *info, unsigned int infolen,
>> +			      u8 *okm, unsigned int okmlen)
>> +{
>> +	return -ENOTSUP;
>> +}
>> +#endif
>> +#endif
> 
> This header is missing <crypto/hash.h> which it depends on.
> 
> Also the !CONFIG_CRYPTO_HKDF stubs are unnecessary and should not be included.
> 
But that would mean that every call to '#include <crypto/hkdf.h>' would 
need to be encapsulated by 'CONFIG_CRYPTO_HKDF' (or the file itself is
conditionally compiled on that symbol).
Is that the direction you want to head?

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