[v4] crypto: api - fix unexpectedly getting generic implementation
diff mbox series

Message ID 20191205045545.qernhqet4dx3b47b@gondor.apana.org.au
State Superseded
Delegated to: Herbert Xu
Headers show
Series
  • [v4] crypto: api - fix unexpectedly getting generic implementation
Related show

Commit Message

Herbert Xu Dec. 5, 2019, 4:55 a.m. UTC
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>

Comments

Eric Biggers Dec. 11, 2019, 2:26 a.m. UTC | #1
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

Patch
diff mbox series

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);