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@xxxxxxx +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
