crypto: api - fix unexpectedly getting generic implementation

From: Eric Biggers <[email protected]>

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

Fix this by making crypto_alg_tested() replace an already-set crypto_larval::adult when it has lower priority and the larval hasn't already been complete()d (by cryptomgr_probe()).

This also required adding crypto_alg_sem protection to completing the larval in cryptomgr_probe().

Also add some comments to crypto_alg_tested() to make it easier to understand what's going on.

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

Signed-off-by Eric Biggers <[email protected]>

---
 crypto/algapi.c   | 46 +++++++++++++++++++++++++++++++++++++++++-----
 crypto/algboss.c  |  4 ++++
 crypto/api.c      |  5 -----
 crypto/internal.h |  5 +++++
 4 files changed, 50 insertions(+), 10 deletions(-)

crypto/algapi.c +41 -5

index b052f38edba621..6c5406d342e2b7 100644
@@ -251,6 +251,17 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
        goto out;
 }
 
+/**
+ * crypto_alg_tested() - handle a self-test result
+ * @name: the driver name of the algorithm that was tested
+ * @err: 0 if testing passed, nonzero if testing failed
+ *
+ * If testing passed, mark the algorithm as tested and try to use it to fulfill
+ * any outstanding template instantiation requests.  Also remove any template
+ * instances that use a lower-priority implementation of the same algorithm.
+ *
+ * In any case, also wake up anyone waiting for the algorithm to be tested.
+ */
 void crypto_alg_tested(const char *name, int err)
 {
        struct crypto_larval *test;

@@ -258,6 +269,7 @@ void crypto_alg_tested(const char *name, int err)
        struct crypto_alg *q;
        LIST_HEAD(list);
 
+       /* Find the algorithm's test larval */
        down_write(&crypto_alg_sem);
        list_for_each_entry(q, &crypto_alg_list, cra_list) {
                if (crypto_is_moribund(q) || !crypto_is_larval(q))

@@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err)
                if (crypto_is_larval(q)) {
                        struct crypto_larval *larval = (void *)q;
 
+                       if (crypto_is_test_larval(larval))
+                               continue;
+
                        /*
-                        * Check to see if either our generic name or
-                        * specific name can satisfy the name requested
-                        * by the larval entry q.
+                        * Fulfill the request larval 'q' (set larval->adult) if
+                        * the tested algorithm is compatible with it, i.e. if
+                        * the request is for the same generic or driver name
+                        * and for compatible flags.
+                        *
+                        * If larval->adult is already set, replace it if the
+                        * tested algorithm is higher priority and the larval
+                        * hasn't been completed()d yet.  This is needed to
+                        * avoid users always getting the generic implementation
+                        * on first use when the extra self-tests are enabled.
                         */
+
                        if (strcmp(alg->cra_name, q->cra_name) &&
                            strcmp(alg->cra_driver_name, q->cra_name))
                                continue;
 
-                       if (larval->adult)
-                               continue;
                        if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
                                continue;
+
+                       if (larval->adult &&
+                           larval->adult->cra_priority >= alg->cra_priority)
+                               continue;
+
+                       if (completion_done(&larval->completion))
+                               continue;
+
                        if (!crypto_mod_get(alg))
                                continue;
 
+                       if (larval->adult)
+                               crypto_mod_put(larval->adult);
                        larval->adult = alg;
                        continue;
                }
 
+               /*
+                * Remove any template instances that use a lower-priority
+                * implementation of the same algorithm.
+                */
+
                if (strcmp(alg->cra_name, q->cra_name))
                        continue;
 

crypto/algboss.c +4 -0

index a62149d6c839f5..f2b3b3ab008334 100644
@@ -81,7 +81,11 @@ static int cryptomgr_probe(void *data)
        crypto_tmpl_put(tmpl);
 
 out:
+       /* crypto_alg_sem is needed to synchronize with crypto_alg_tested() */
+       down_write(&crypto_alg_sem);
        complete_all(&param->larval->completion);
+       up_write(&crypto_alg_sem);
+
        crypto_alg_put(&param->larval->alg);
        kfree(param);
        module_put_and_exit(0);

crypto/api.c +0 -5

index ef96142ceca746..855004cb0b4d59 100644
@@ -47,11 +47,6 @@ void crypto_mod_put(struct crypto_alg *alg)
 }
 EXPORT_SYMBOL_GPL(crypto_mod_put);
 
-static inline int crypto_is_test_larval(struct crypto_larval *larval)
-{
-       return larval->alg.cra_driver_name[0];
-}
-
 static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type,
                                              u32 mask)
 {

crypto/internal.h +5 -1

index ff06a3bd1ca10c..ba744ac2ee1f09 100644
@@ -110,6 +110,11 @@ static inline int crypto_is_larval(struct crypto_alg *alg)
        return alg->cra_flags & CRYPTO_ALG_LARVAL;
 }
 
+static inline int crypto_is_test_larval(struct crypto_larval *larval)
+{
+       return larval->alg.cra_driver_name[0];
+}
+
 static inline int crypto_is_dead(struct crypto_alg *alg)
 {
        return alg->cra_flags & CRYPTO_ALG_DEAD;
--

2.24.0.393.g34dc348eaf-goog

From: Eric Biggers <[email protected]>

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

Fix this by making crypto_alg_tested() replace an already-set crypto_larval::adult when it has lower priority and the larval hasn't already been complete()d (by cryptomgr_probe()).

This also required adding crypto_alg_sem protection to completing the larval in cryptomgr_probe().

Also add some comments to crypto_alg_tested() to make it easier to understand what's going on.

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

Signed-off-by Eric Biggers <[email protected]>

---

crypto/algapi.c   | 46 +++++++++++++++++++++++++++++++++++++++++-----
crypto/algboss.c  |  4 ++++
crypto/api.c      |  5 -----
crypto/internal.h |  5 +++++
4 files changed, 50 insertions(+), 10 deletions(-)

(quoted diff) +20 -5

@@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err)
               if (crypto_is_larval(q)) {
                       struct crypto_larval *larval = (void *)q;
 
+                       if (crypto_is_test_larval(larval))
+                               continue;
+
                       /*
-                        * Check to see if either our generic name or
-                        * specific name can satisfy the name requested
-                        * by the larval entry q.
+                        * Fulfill the request larval 'q' (set larval->adult) if
+                        * the tested algorithm is compatible with it, i.e. if
+                        * the request is for the same generic or driver name
+                        * and for compatible flags.
+                        *
+                        * If larval->adult is already set, replace it if the
+                        * tested algorithm is higher priority and the larval
+                        * hasn't been completed()d yet.  This is needed to
+                        * avoid users always getting the generic implementation
+                        * on first use when the extra self-tests are enabled.
                        */
+
                       if (strcmp(alg->cra_name, q->cra_name) &&
                           strcmp(alg->cra_driver_name, q->cra_name))
                               continue;
 
-                       if (larval->adult)
-                               continue;
                       if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
                               continue;
+
+                       if (larval->adult &&
+                           larval->adult->cra_priority >= alg->cra_priority)
+                               continue;
+
+                       if (completion_done(&larval->completion))

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 <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

crypto/algapi.c +2 -0

index b052f38edba6..3e65653735f4 100644
@@ -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) {

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.

Quoted 3 lines · Eric Biggers

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.

Quoted 6 lines

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

Quoted 10 lines · Eric Biggers

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.

Quoted 2 lines

(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 <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

crypto/algapi.c +21 -2

index b052f38edba6..c7527ac614af 100644
@@ -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;

crypto/api.c +1 -1

index 55bca28df92d..b5ad4cc1198a 100644
@@ -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);
 }

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.

Quoted 4 lines · Eric Biggers

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

Oops.

Quoted 6 lines

that variable and doing:

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

OK I have made this change.

Quoted 2 lines

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.

Your original comments no longer apply but if you wish to make another patch to add more comments that would certainly be welcome.

Thanks,

---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 <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

crypto/algapi.c +8 -0

+	/* 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))

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.

Quoted 4 lines · Eric Biggers

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.