On Mon, 2 Dec 2019 at 22:13, Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
Thanks for fixing this.
Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> 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
>
