On Wed, Dec 04, 2019 at 05:19:10PM +0800, Herbert Xu wrote:
> I think this is a tad over-complicated. All we really need to do
> is avoid changing larval->adult if we are not the best larval.
> Something like this (totally untested!):
>
> ---8<---
> 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).
>
> This patch fixes this by only fulfilling the original request if
> we are currently the best outstanding larval as judged by the
> priority.
>
> 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")
> Reported-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b052f38edba6..3e65653735f4 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -280,6 +280,18 @@ void crypto_alg_tested(const char *name, int err)
>
> alg->cra_flags |= CRYPTO_ALG_TESTED;
>
> + /* Only satisfy larval waiters if we are the best. */
> + list_for_each_entry(q, &crypto_alg_list, cra_list) {
> + if (!crypto_is_larval(q))
> + continue;
> +
> + if (strcmp(alg->cra_name, q->cra_name))
> + continue;
> +
> + if (q->cra_priority > alg->cra_priority)
> + goto complete;
> + }
> +
I was going to do something like this originally (but also checking that 'q' is
not "moribund", is a test larval, and has compatible cra_flags). But I don't
think it will work because a higher priority implementation could be registered
while a lower priority one is being instantiated and tested. Based on this
logic, when the lower priority implementation finishes being tested,
larval->adult wouldn't be set since a higher priority implementation is still
being tested. But then cryptomgr_probe() will complete() the larval anyway and
for the user crypto_alloc_foo() will fail with ENOENT.
With my patch the user would get the lower priority implementation in this case,
since it would be the best one ready at the time cryptomgr_probe() finished.
- Eric
