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
