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

[Herbert Xu <herbert@gondor.apana.org.au>]

On Wed, Dec 04, 2019 at 09:22:44AM -0800, Eric Biggers wrote:
>
> 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

You are right.  I'll add these tests to the patch.

> 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.

I think this is a different problem, one which we probably should
address but it already exists regardless of what we do here.  For
example, assuming that tmpl(X) does not currently exist, and I
request tmpl(X-generic) then tmpl(X-generic) along with X-generic
will be created in the system.  If someone then comes along and
asks for tmpl(X) then we'll simply give them tmpl(X-generic) even
if there exists an accelerated version of X.

The problem you describe is simply a racy version of the above
scenario where the requests for tmpl(X) and tmpl(X-generic) occur
about the same time.

We should certainly fix this, as otherwise anyone could force
the whole system to use the generic (or some other non-optimal
variant).  One possible solution is to mark any instance created
by a request like tmpl(X-generic) as non-optimal so that a subsequent
request for tmpl(X) has to go through the whole process again before
being fulfilled.

The hardest part of this is actually ensuring that X-generic does
not fulfil a subsequent request for X.  This is because X-generic
is probably going to be created by a module-load and the crypto
API is not driving the registration of X-generic.  We could
probably use some form of larvals that get created at registration
time to resolve this.

Cheers,

---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...")
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..390bdc874b61 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -280,6 +280,24 @@ 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) {
+		struct crypto_larval *larval;
+
+		if (crypto_is_moribund(q) || !crypto_is_larval(q))
+			continue;
+
+		if (strcmp(alg->cra_name, q->cra_name))
+			continue;
+
+		larval = (void *)q;
+		if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
+			continue;
+
+		if (q->cra_priority > alg->cra_priority)
+			goto complete;
+	}
+
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (q == alg)
 			continue;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt