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

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Oct 11, 2024 at 05:54:22PM +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.

integrity => correctness

> +config CRYPTO_HKDF
> +	tristate
> +	select CRYPTO_SHA1 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +	select CRYPTO_SHA256 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +	select CRYPTO_HASH2

Any thoughts on including SHA512 tests instead of SHA1, given that SHA1 is
obsolete and should not be used?

> +int hkdf_expand(struct crypto_shash *hmac_tfm,
> +		const u8 *info, unsigned int infolen,
> +		u8 *okm, unsigned int okmlen)
> +{
> +	SHASH_DESC_ON_STACK(desc, hmac_tfm);
> +	unsigned int i, hashlen = crypto_shash_digestsize(hmac_tfm);
> +	int err;
> +	const u8 *prev = NULL;
> +	u8 counter = 1;
> +	u8 tmp[HASH_MAX_DIGESTSIZE];
> +
> +	if (WARN_ON(okmlen > 255 * hashlen ||
> +		    hashlen > HASH_MAX_DIGESTSIZE))
> +		return -EINVAL;

The crypto API guarantees HASH_MAX_DIGESTSIZE, so checking that again is not
very useful.

> +
> +	memzero_explicit(tmp, HASH_MAX_DIGESTSIZE);

The zeroization above is unnecessary.  If it's done anyway, it is just an
initialization, so it should use an initializer '= {}' instead of
memzero_explicit() which is intended for "destruction".

> +MODULE_ALIAS_CRYPTO("hkdf");

This alias does not make sense and is unnecessary.  These are library functions
that are not exposed by name through the crypto API, so there is no need to wire
them up to the module autoloading accordingly.

> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
> new file mode 100644
> index 000000000000..c1f23a32a6b6
> --- /dev/null
> +++ b/include/crypto/hkdf.h
> @@ -0,0 +1,18 @@
> +/* 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
> +
> +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);
> +#endif

This needs to include <crypto/hash.h>.

Otherwise there will be errors if someone includes this as their first header.

- Eric