| Message ID | 20191202221319.258002-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: api - fix unexpectedly getting generic implementation | expand |
On Mon, 2 Dec 2019 at 22:13, Eric Biggers <ebiggers@kernel.org> wrote: > > From: Eric Biggers <ebiggers@google.com> > > When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an > algorithm that needs to be instantiated using a template will always get > the generic implementation, even when an accelerated one is available. > > This happens because the extra self-tests for the accelerated > implementation allocate the generic implementation for comparison > purposes, and then crypto_alg_tested() for the generic implementation > "fulfills" the original request (i.e. sets crypto_larval::adult). > > Fix this by making crypto_alg_tested() replace an already-set > crypto_larval::adult when it has lower priority and the larval hasn't > already been complete()d (by cryptomgr_probe()). > > This also required adding crypto_alg_sem protection to completing the > larval in cryptomgr_probe(). > > Also add some comments to crypto_alg_tested() to make it easier to > understand what's going on. > > Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation") > Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation") > Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation") > Signed-off-by: Eric Biggers <ebiggers@google.com> Thanks for fixing this. Acked-by: Ard Biesheuvel <ardb@kernel.org> > --- > crypto/algapi.c | 46 +++++++++++++++++++++++++++++++++++++++++----- > crypto/algboss.c | 4 ++++ > crypto/api.c | 5 ----- > crypto/internal.h | 5 +++++ > 4 files changed, 50 insertions(+), 10 deletions(-) > > diff --git a/crypto/algapi.c b/crypto/algapi.c > index b052f38edba621..6c5406d342e2b7 100644 > --- a/crypto/algapi.c > +++ b/crypto/algapi.c > @@ -251,6 +251,17 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg) > goto out; > } > > +/** > + * crypto_alg_tested() - handle a self-test result > + * @name: the driver name of the algorithm that was tested > + * @err: 0 if testing passed, nonzero if testing failed > + * > + * If testing passed, mark the algorithm as tested and try to use it to fulfill > + * any outstanding template instantiation requests. Also remove any template > + * instances that use a lower-priority implementation of the same algorithm. > + * > + * In any case, also wake up anyone waiting for the algorithm to be tested. > + */ > void crypto_alg_tested(const char *name, int err) > { > struct crypto_larval *test; > @@ -258,6 +269,7 @@ void crypto_alg_tested(const char *name, int err) > struct crypto_alg *q; > LIST_HEAD(list); > > + /* Find the algorithm's test larval */ > down_write(&crypto_alg_sem); > list_for_each_entry(q, &crypto_alg_list, cra_list) { > if (crypto_is_moribund(q) || !crypto_is_larval(q)) > @@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err) > if (crypto_is_larval(q)) { > struct crypto_larval *larval = (void *)q; > > + if (crypto_is_test_larval(larval)) > + continue; > + > /* > - * Check to see if either our generic name or > - * specific name can satisfy the name requested > - * by the larval entry q. > + * Fulfill the request larval 'q' (set larval->adult) if > + * the tested algorithm is compatible with it, i.e. if > + * the request is for the same generic or driver name > + * and for compatible flags. > + * > + * If larval->adult is already set, replace it if the > + * tested algorithm is higher priority and the larval > + * hasn't been completed()d yet. This is needed to > + * avoid users always getting the generic implementation > + * on first use when the extra self-tests are enabled. > */ > + > if (strcmp(alg->cra_name, q->cra_name) && > strcmp(alg->cra_driver_name, q->cra_name)) > continue; > > - if (larval->adult) > - continue; > if ((q->cra_flags ^ alg->cra_flags) & larval->mask) > continue; > + > + if (larval->adult && > + larval->adult->cra_priority >= alg->cra_priority) > + continue; > + > + if (completion_done(&larval->completion)) > + continue; > + > if (!crypto_mod_get(alg)) > continue; > > + if (larval->adult) > + crypto_mod_put(larval->adult); > larval->adult = alg; > continue; > } > > + /* > + * Remove any template instances that use a lower-priority > + * implementation of the same algorithm. > + */ > + > if (strcmp(alg->cra_name, q->cra_name)) > continue; > > diff --git a/crypto/algboss.c b/crypto/algboss.c > index a62149d6c839f5..f2b3b3ab008334 100644 > --- a/crypto/algboss.c > +++ b/crypto/algboss.c > @@ -81,7 +81,11 @@ static int cryptomgr_probe(void *data) > crypto_tmpl_put(tmpl); > > out: > + /* crypto_alg_sem is needed to synchronize with crypto_alg_tested() */ > + down_write(&crypto_alg_sem); > complete_all(¶m->larval->completion); > + up_write(&crypto_alg_sem); > + > crypto_alg_put(¶m->larval->alg); > kfree(param); > module_put_and_exit(0); > diff --git a/crypto/api.c b/crypto/api.c > index ef96142ceca746..855004cb0b4d59 100644 > --- a/crypto/api.c > +++ b/crypto/api.c > @@ -47,11 +47,6 @@ void crypto_mod_put(struct crypto_alg *alg) > } > EXPORT_SYMBOL_GPL(crypto_mod_put); > > -static inline int crypto_is_test_larval(struct crypto_larval *larval) > -{ > - return larval->alg.cra_driver_name[0]; > -} > - > static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type, > u32 mask) > { > diff --git a/crypto/internal.h b/crypto/internal.h > index ff06a3bd1ca10c..ba744ac2ee1f09 100644 > --- a/crypto/internal.h > +++ b/crypto/internal.h > @@ -110,6 +110,11 @@ static inline int crypto_is_larval(struct crypto_alg *alg) > return alg->cra_flags & CRYPTO_ALG_LARVAL; > } > > +static inline int crypto_is_test_larval(struct crypto_larval *larval) > +{ > + return larval->alg.cra_driver_name[0]; > +} > + > static inline int crypto_is_dead(struct crypto_alg *alg) > { > return alg->cra_flags & CRYPTO_ALG_DEAD; > -- > 2.24.0.393.g34dc348eaf-goog >
diff --git a/crypto/algapi.c b/crypto/algapi.c index b052f38edba621..6c5406d342e2b7 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -251,6 +251,17 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg) goto out; } +/** + * crypto_alg_tested() - handle a self-test result + * @name: the driver name of the algorithm that was tested + * @err: 0 if testing passed, nonzero if testing failed + * + * If testing passed, mark the algorithm as tested and try to use it to fulfill + * any outstanding template instantiation requests. Also remove any template + * instances that use a lower-priority implementation of the same algorithm. + * + * In any case, also wake up anyone waiting for the algorithm to be tested. + */ void crypto_alg_tested(const char *name, int err) { struct crypto_larval *test; @@ -258,6 +269,7 @@ void crypto_alg_tested(const char *name, int err) struct crypto_alg *q; LIST_HEAD(list); + /* Find the algorithm's test larval */ down_write(&crypto_alg_sem); list_for_each_entry(q, &crypto_alg_list, cra_list) { if (crypto_is_moribund(q) || !crypto_is_larval(q)) @@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err) if (crypto_is_larval(q)) { struct crypto_larval *larval = (void *)q; + if (crypto_is_test_larval(larval)) + continue; + /* - * Check to see if either our generic name or - * specific name can satisfy the name requested - * by the larval entry q. + * Fulfill the request larval 'q' (set larval->adult) if + * the tested algorithm is compatible with it, i.e. if + * the request is for the same generic or driver name + * and for compatible flags. + * + * If larval->adult is already set, replace it if the + * tested algorithm is higher priority and the larval + * hasn't been completed()d yet. This is needed to + * avoid users always getting the generic implementation + * on first use when the extra self-tests are enabled. */ + if (strcmp(alg->cra_name, q->cra_name) && strcmp(alg->cra_driver_name, q->cra_name)) continue; - if (larval->adult) - continue; if ((q->cra_flags ^ alg->cra_flags) & larval->mask) continue; + + if (larval->adult && + larval->adult->cra_priority >= alg->cra_priority) + continue; + + if (completion_done(&larval->completion)) + continue; + if (!crypto_mod_get(alg)) continue; + if (larval->adult) + crypto_mod_put(larval->adult); larval->adult = alg; continue; } + /* + * Remove any template instances that use a lower-priority + * implementation of the same algorithm. + */ + if (strcmp(alg->cra_name, q->cra_name)) continue; diff --git a/crypto/algboss.c b/crypto/algboss.c index a62149d6c839f5..f2b3b3ab008334 100644 --- a/crypto/algboss.c +++ b/crypto/algboss.c @@ -81,7 +81,11 @@ static int cryptomgr_probe(void *data) crypto_tmpl_put(tmpl); out: + /* crypto_alg_sem is needed to synchronize with crypto_alg_tested() */ + down_write(&crypto_alg_sem); complete_all(¶m->larval->completion); + up_write(&crypto_alg_sem); + crypto_alg_put(¶m->larval->alg); kfree(param); module_put_and_exit(0); diff --git a/crypto/api.c b/crypto/api.c index ef96142ceca746..855004cb0b4d59 100644 --- a/crypto/api.c +++ b/crypto/api.c @@ -47,11 +47,6 @@ void crypto_mod_put(struct crypto_alg *alg) } EXPORT_SYMBOL_GPL(crypto_mod_put); -static inline int crypto_is_test_larval(struct crypto_larval *larval) -{ - return larval->alg.cra_driver_name[0]; -} - static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type, u32 mask) { diff --git a/crypto/internal.h b/crypto/internal.h index ff06a3bd1ca10c..ba744ac2ee1f09 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -110,6 +110,11 @@ static inline int crypto_is_larval(struct crypto_alg *alg) return alg->cra_flags & CRYPTO_ALG_LARVAL; } +static inline int crypto_is_test_larval(struct crypto_larval *larval) +{ + return larval->alg.cra_driver_name[0]; +} + static inline int crypto_is_dead(struct crypto_alg *alg) { return alg->cra_flags & CRYPTO_ALG_DEAD;