| Message ID | ZkhS1zrobNwAuANI@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | crypto: api - Do not load modules until algapi is ready | expand |
On Sat May 18, 2024 at 10:03 AM EEST, Herbert Xu wrote: > 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" <nfraprado@collabora.com> > Reported-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Right does this mean for TPM driver that a crypto API invocation not having everthing needed loaded will block until this is not the case? BR, Jarkko
On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote: > > Right does this mean for TPM driver that a crypto API invocation not > having everthing needed loaded will block until this is not the case? All this does is disable module loading by the Crypto API (because there is no point and it may deadlock) until such a point where all/most drivers have finished loading. So if the algorithm is missing (which shouldn't happen because of Kconfig selects), then it will simply fail. Cheers,
On Sat May 18, 2024 at 3:32 PM EEST, Herbert Xu wrote: > On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote: > > > > Right does this mean for TPM driver that a crypto API invocation not > > having everthing needed loaded will block until this is not the case? > > All this does is disable module loading by the Crypto API (because > there is no point and it may deadlock) until such a point where > all/most drivers have finished loading. > > So if the algorithm is missing (which shouldn't happen because of > Kconfig selects), then it will simply fail. > > Cheers, Ok, you can add Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
On May 18, 2024 5:32:56 AM PDT, Herbert Xu <herbert@gondor.apana.org.au> wrote: >On Sat, May 18, 2024 at 02:04:18PM +0300, Jarkko Sakkinen wrote: >> >> Right does this mean for TPM driver that a crypto API invocation not >> having everthing needed loaded will block until this is not the case? > >All this does is disable module loading by the Crypto API (because >there is no point and it may deadlock) until such a point where >all/most drivers have finished loading. > >So if the algorithm is missing (which shouldn't happen because of >Kconfig selects), then it will simply fail. I have a curiosity question: if Eric is right and it's looking for an optional hmac accelerator module, why don't I see this? The only real config difference between what I tested and what Nicholas did is he's arm and I'm x86. James
On Sat, May 18, 2024 at 06:07:39AM -0700, James Bottomley wrote: > > I have a curiosity question: if Eric is right and it's looking for an optional hmac accelerator module, why don't I see this? The only real config difference between what I tested and what Nicholas did is he's arm and I'm x86. It depends on your kernel config. Perhaps you didn't build the TPM driver into the kernel? It's not an issue with an optional algorithm that's not included. It's the fact the Crypto API tries to load a module for any algorithm that requires instantiation (anything with "()" in its name). Cheers,
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" <nfraprado@collabora.com> > Reported-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Herbert Xu <herbert@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
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);