On Sat, May 18, 2024 at 03:03:51PM +0800, Herbert Xu wrote:
> On Fri, May 17, 2024 at 09:31:15PM -0700, Eric Biggers wrote:
> >
> > This is "normal" behavior when the crypto API instantiates a template:
> >
> > 1. drbg.c asks for "hmac(sha512)"
> >
> > 2. The crypto API looks for a direct implementation of "hmac(sha512)".
> > This includes requesting a module with alias "crypto-hmac(sha512)".
> >
> > 3. If none is found, the "hmac" template is instantiated instead.
> >
> > There are two possible fixes for the bug. Either fix ecc_gen_privkey() to
> > just
> > use get_random_bytes() instead of the weird crypto API RNG, or make
> > drbg_init_hash_kernel() pass the CRYPTO_NOLOAD flag to crypto_alloc_shash().
> >
> > Or if the TPM driver could be changed to not need to generate an ECC
> > private key
> > at probe time, that would also avoid this problem.
>
> Thanks for diagnosing the problem. This is easy to fix though,
> we could simply reuse the static branch that was created for
> boot-time self-testing:
>
> ---8<---
> When the Crypto API is built into the kernel, it may be invoked
> during system initialisation before modules can be loaded. Ensure
> that it doesn't load modules if this is the case by checking
> crypto_boot_test_finished().
>
> Add a call to wait_for_device_probe so that the drivers that may
> call into the Crypto API have finished probing.
>
> Reported-by: Nícolas F. R. A. Prado" <nfrapr...@collabora.com>
> Reported-by: Eric Biggers <ebigg...@kernel.org>
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 85bc279b4233..c018bcbd1f46 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -7,6 +7,7 @@
>
> #include <crypto/algapi.h>
> #include <crypto/internal/simd.h>
> +#include <linux/device/driver.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> #include <linux/fips.h>
> @@ -1056,9 +1057,12 @@ EXPORT_SYMBOL_GPL(crypto_type_has_alg);
>
> static void __init crypto_start_tests(void)
> {
> - if (IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
> + if (!IS_BUILTIN(CONFIG_CRYPTO_ALGAPI))
> return;
>
> + if (IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
> + goto test_done;
> +
> for (;;) {
> struct crypto_larval *larval = NULL;
> struct crypto_alg *q;
> @@ -1092,6 +1096,8 @@ static void __init crypto_start_tests(void)
> crypto_wait_for_test(larval);
> }
>
> +test_done:
> + wait_for_device_probe();
> set_crypto_boot_test_finished();
> }
>
> diff --git a/crypto/api.c b/crypto/api.c
> index 6aa5a3b4ed5e..5c970af04ba9 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -31,9 +31,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
> BLOCKING_NOTIFIER_HEAD(crypto_chain);
> EXPORT_SYMBOL_GPL(crypto_chain);
>
> -#ifndef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +#if IS_BUILTIN(CONFIG_CRYPTO_ALGAPI)
> DEFINE_STATIC_KEY_FALSE(__crypto_boot_test_finished);
> -EXPORT_SYMBOL_GPL(__crypto_boot_test_finished);
> #endif
>
> static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
> @@ -280,7 +279,7 @@ static struct crypto_alg *crypto_larval_lookup(const char
> *name, u32 type,
> mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
>
> alg = crypto_alg_lookup(name, type, mask);
> - if (!alg && !(mask & CRYPTO_NOLOAD)) {
> + if (crypto_boot_test_finished() && !alg && !(mask & CRYPTO_NOLOAD)) {
> request_module("crypto-%s", name);
>
> if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask &
> diff --git a/crypto/internal.h b/crypto/internal.h
> index 63e59240d5fb..d27166a92eca 100644
> --- a/crypto/internal.h
> +++ b/crypto/internal.h
> @@ -66,7 +66,7 @@ extern struct blocking_notifier_head crypto_chain;
>
> int alg_test(const char *driver, const char *alg, u32 type, u32 mask);
>
> -#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +#if !IS_BUILTIN(CONFIG_CRYPTO_ALGAPI)
> static inline bool crypto_boot_test_finished(void)
> {
> return true;
> @@ -84,7 +84,7 @@ static inline void set_crypto_boot_test_finished(void)
> {
> static_branch_enable(&__crypto_boot_test_finished);
> }
> -#endif /* !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS */
> +#endif /* !IS_BUILTIN(CONFIG_CRYPTO_ALGAPI) */
>
> #ifdef CONFIG_PROC_FS
> void __init crypto_init_proc(void);
> --
Hi Herbert,
Unfortunately this patch didn't work either. The warning is still there
unchanged.
The warning is triggered by a driver that probes asynchronously registering a
TPM device, which ends up requesting a module to be loaded synchronously. So I
don't think waiting would even help here. I believe one possible solution would
be to request the module asynchronously and defer probe. Not ideal but I think
would work.
Alternatively, could the initialization code that loads this module
(hmac(sha512)) be run synchronously before the TPM device is registered? Or is
it device specific?
PS: the wait_for_device_probe() call in the patch didn't actually wait for
anything in this case since when the crypto tests code ran there weren't any
probes in progress
Thanks,
Nícolas
