Re: [v4 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Dec 05, 2019 at 12:55:45PM +0800, Herbert Xu wrote:
> On Wed, Dec 04, 2019 at 07:43:01PM -0800, Eric Biggers wrote:
> >
> > No, the problem I'm talking about is different and is new to your patch.  If
> > tmpl(X-accelerated) is registered while someone is doing crypto_alg_mod_lookup()
> > that triggered instantiation of tmpl(X-generic), then crypto_alg_mod_lookup()
> > could fail with ENOENT, instead of returning tmpl(X-generic) as it does
> > currently.  This is because the proposed new logic will not fulfill the request
> > larval if a better implementation of tmpl(X) is still being tested.  But there's
> > no guarantee that tmpl(X) will finish being tested by the time cryptomgr_probe()
> > thinks it is done and complete()s the request larval with 'adult' still NULL.
> > 
> > (I think; I haven't actually tested this, this analysis is just based on my
> > reading of the code...)
> 
> Right.  This is indeed a regression.  How about this patch then?
> We can simply punt and retry the lookup if we encounter a better
> larval.
> 
> ---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.  If we're not the best then we will ask all waiters on
> that larval request to retry the lookup.
> 
> Note that this patch introduces a behaviour change when the module
> providing the new algorithm is unregistered during the process.
> Previously we would have failed with ENOENT, after the patch we
> will instead redo the lookup.
>  
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against...")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against...")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against...")
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b052f38edba6..c7527ac614af 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -257,6 +257,7 @@ void crypto_alg_tested(const char *name, int err)
>  	struct crypto_alg *alg;
>  	struct crypto_alg *q;
>  	LIST_HEAD(list);
> +	bool best;
>  
>  	down_write(&crypto_alg_sem);
>  	list_for_each_entry(q, &crypto_alg_list, cra_list) {
> @@ -280,6 +281,21 @@ void crypto_alg_tested(const char *name, int err)
>  
>  	alg->cra_flags |= CRYPTO_ALG_TESTED;
>  
> +	/* Only satisfy larval waiters if we are the best. */
> +	best = true;
> +	list_for_each_entry(q, &crypto_alg_list, cra_list) {
> +		if (crypto_is_moribund(q) || !crypto_is_larval(q))
> +			continue;
> +
> +		if (strcmp(alg->cra_name, q->cra_name))
> +			continue;
> +
> +		if (q->cra_priority > alg->cra_priority) {
> +			best = false;
> +			break;
> +		}
> +	}
> +
>  	list_for_each_entry(q, &crypto_alg_list, cra_list) {
>  		if (q == alg)
>  			continue;
> @@ -289,6 +305,7 @@ void crypto_alg_tested(const char *name, int err)
>  
>  		if (crypto_is_larval(q)) {
>  			struct crypto_larval *larval = (void *)q;
> +			struct crypto_alg *r;
>  
>  			/*
>  			 * Check to see if either our generic name or
> @@ -303,8 +320,10 @@ void crypto_alg_tested(const char *name, int err)
>  				continue;
>  			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
>  				continue;
> -			if (!crypto_mod_get(alg))
> -				continue;
> +
> +			r = ERR_PTR(-EAGAIN);
> +			if (best && crypto_mod_get(alg))
> +				r = alg;
>  
>  			larval->adult = alg;
>  			continue;
> diff --git a/crypto/api.c b/crypto/api.c
> index 55bca28df92d..b5ad4cc1198a 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -97,7 +97,7 @@ static void crypto_larval_destroy(struct crypto_alg *alg)
>  	struct crypto_larval *larval = (void *)alg;
>  
>  	BUG_ON(!crypto_is_larval(alg));
> -	if (larval->adult)
> +	if (!IS_ERR_OR_NULL(larval->adult))
>  		crypto_mod_put(larval->adult);
>  	kfree(larval);
>  }
> @@ -178,6 +178,8 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
>  		alg = ERR_PTR(-ETIMEDOUT);
>  	else if (!alg)
>  		alg = ERR_PTR(-ENOENT);
> +	else if (IS_ERR(alg))
> +		;
>  	else if (crypto_is_test_larval(larval) &&
>  		 !(alg->cra_flags & CRYPTO_ALG_TESTED))
>  		alg = ERR_PTR(-EAGAIN);

Sorry, I didn't notice you had already sent another patch for this.  I think
this patch is okay, except that it's broken because it doesn't actually do
anything with the 'r' variable in crypto_alg_tested().  I suggest just removing
that variable and doing:

		if (best && crypto_mod_get(alg))
			larval->adult = alg;
		else
			larval->adult = ERR_PTR(-EAGAIN);

Also, it would be nice to also add a function comment for crypto_alg_tested(),
like I had in my original patch.  It's hard to understand this code.

- Eric