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

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

Commit Message

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

Comments

Eric Biggers Dec. 5, 2019, 3:43 a.m. UTC | #1
On Thu, Dec 05, 2019 at 09:58:11AM +0800, Herbert Xu wrote:
> 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.
> 

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

- Eric
Eric Biggers Dec. 5, 2019, 4:04 a.m. UTC | #2
On Thu, Dec 05, 2019 at 09:58:11AM +0800, Herbert Xu wrote:
> +	/* 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;
> +	}
> +

This logic doesn't make sense to me either.  It's supposed to be looking for a
"test larval", not a "request larval", right?  But it seems that larval->mask is
always 0 for "test larvals", so the flags check will never do anything...

Also, different "request larvals" can use different flags and masks.  So I don't
think it's possible to know whether 'q' can fulfill every outstanding request
that 'alg' can without actually going through and looking at the requests.  So
that's another case where users can start incorrectly getting ENOENT.

If we don't try to skip setting larval->adult, but rather override the existing
value (as my patch does), the incorrect ENOENTs are prevented.

- Eric
Herbert Xu Dec. 5, 2019, 4:23 a.m. UTC | #3
On Wed, Dec 04, 2019 at 08:04:42PM -0800, Eric Biggers wrote:
>
> This logic doesn't make sense to me either.  It's supposed to be looking for a
> "test larval", not a "request larval", right?  But it seems that larval->mask is
> always 0 for "test larvals", so the flags check will never do anything...

No we only care about request larvals.  Test larvals always have
a non-null adult set so they are irrelevant.

> Also, different "request larvals" can use different flags and masks.  So I don't
> think it's possible to know whether 'q' can fulfill every outstanding request
> that 'alg' can without actually going through and looking at the requests.  So
> that's another case where users can start incorrectly getting ENOENT.

That's a good point.  Let me think about this a bit more.

Thanks,
Herbert Xu Dec. 5, 2019, 4:27 a.m. UTC | #4
On Thu, Dec 05, 2019 at 12:23:57PM +0800, Herbert Xu wrote:
> On Wed, Dec 04, 2019 at 08:04:42PM -0800, Eric Biggers wrote:
> >
> > This logic doesn't make sense to me either.  It's supposed to be looking for a
> > "test larval", not a "request larval", right?  But it seems that larval->mask is
> > always 0 for "test larvals", so the flags check will never do anything...
> 
> No we only care about request larvals.  Test larvals always have
> a non-null adult set so they are irrelevant.

Scratch that.  You're right we only care about test larvals and
if we wanted to check the flags we need to drill down to the
actual tested algorithm using larval->adult.

Cheers,

Patch
diff mbox series

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;