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

[Eric Biggers <ebiggers@kernel.org>]

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?

> > > +#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.

- Eric