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

[Hannes Reinecke]

On 8/29/24 23:54, Eric Biggers wrote:
> On Thu, Aug 29, 2024 at 12:39:33PM +0200, Hannes Reinecke wrote:
>> 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?
> 
> No, if someone does pass NULL with a nonzero length there will be a crash.  But
> the same will happen with another invalid pointer that is not NULL.  It's just a
> bad practice to insert random NULL checks like this because it can hide bugs.
> Really a call like info=NULL, infolen=10 is ambiguous --- you've made it
> silently override infolen to 0 but how do you know the caller wanted that?
> 
Just wanted to clarify; different maintainers have different styles ...

>>>> +#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).
> 
> No, it doesn't mean that.  As long as the functions are not called when
> !CONFIG_CRYPTO_HKDF, it doesn't hurt to have declarations of them.
> 
Guess that is correct. Will be reposting the series.

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