[PATCH 0/8] crypto: api - priorize tested algorithms in lookup

[PATCH 0/8] crypto: api - priorize tested algorithms in lookup

[Nicolai Stange]

Hi all,

my initial goal had been to make the algorithm lookup prefer usable
instances over ones where the tests are still pending. In my first attempt
I realized that the code in crypto_alg_lookup() became quite convoluted
and that it could get streamlined somewhat by consolidating
CRYPTO_ALG_TESTED handling for lookup larvals a bit.

This cleanup can be found in [1/8] - [7/8] and might perhaps be worth
considering on its own, independent of whether or not you deem the
final [8/8], the patch implementing my original goal of priorizing
algorithms in working state, suitable for upstream inclusion.

This series is based on current herbert/cryptodev-2.6 master.

All patches have been tested with tcrypt.ko as well as the kernel/crypto
testcases from LTP.

Thanks!

Nicolai

Nicolai Stange (8):
  crypto: af_alg - reject requests for untested algorithms
  crypto: user - reject requests for untested algorithms
  crypto: api - only support lookups for specific CRYPTO_ALG_TESTED
    status
  crypto: api - don't add larvals for !(type & CRYPTO_ALG_TESTED)
    lookups
  crypto: api - always set CRYPTO_ALG_TESTED in lookup larvals'
    ->mask/type
  crypto: api - make crypto_alg_lookup() consistently check for failed
    algos
  crypto: api - lift common mask + type adjustment to
    crypto_larval_lookup()
  crypto: api - make the algorithm lookup priorize non-larvals

 crypto/af_alg.c           |  9 +++++++
 crypto/api.c              | 53 ++++++++++++++++++++++++++++++++-------
 crypto/crypto_user_base.c |  3 +++
 3 files changed, 56 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH 1/8] crypto: af_alg - reject requests for untested algorithms

[Nicolai Stange]

Currently it's possible for userspace to specify any combination of
->salg_feat and ->salg_mask with respect to CRYTPO_ALG_TESTED via the
af_alg interface.

As these are passed onwards to crypto_larval_lookup() unmodified as
'mask' and 'type' parameters eventually, this can lead to the creation of
obscure lookup larvals like e.g. (mask & CRYTPO_ALG_TESTED) but not
(type & CRYTPO_ALG_TESTED) or the other way around.

Userspace should have no business in asking for untested algorithms. Make
af_alg's alg_bind() reject nonsensical combinations of ->salg_feat and
->salg_mask with respect to CRYTPO_ALG_TESTED with -EINVAL.

Note that CRYTPO_ALG_TESTED not being set in either of mask and type is
considered equivalent to that flag being set in both and these two
combinations are the only ones supported as of now.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/af_alg.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 8bd288d2b089..83e68f3f71db 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -166,6 +166,15 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if ((sa->salg_feat & ~allowed) || (sa->salg_mask & ~allowed))
 		return -EINVAL;
 
+	/*
+	 * Don't allow requests for untested algorithms, i.e. those
+	 * where the selftests are still in progress or have failed.
+	 * CRYPTO_ALG_TESTED must be set either in none or both of
+	 * type and mask (which is equivalent).
+	 */
+	if ((sa->salg_feat ^ sa->salg_mask) & CRYPTO_ALG_TESTED)
+		return -EINVAL;
+
 	sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
 	sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;
 
-- 
2.26.2

[PATCH 2/8] crypto: user - reject requests for untested algorithms

[Nicolai Stange]

Currently it's possible for userspace to specify any combination of
->cru_type and ->cru_mask with respect to CRYTPO_ALG_TESTED via the
CRYPTO_MSG_NEWALG crypto_user interface.

As these are passed onwards to crypto_larval_lookup() unmodified as
'mask' and 'type' parameters eventually, this can lead to the creation of
obscure lookup larvals like e.g. (mask & CRYTPO_ALG_TESTED) but not
(type & CRYTPO_ALG_TESTED) or the other way around.

Userspace should have no business in asking for untested algorithms. Make
crypto_user's crypto_add_alg() reject nonsensical combinations of
->cru_type and ->cru_mask with respect to CRYTPO_ALG_TESTED with -EINVAL.

Note that CRYTPO_ALG_TESTED not being set in either of mask and type is
considered equivalent to that flag being set in both and these two
combinations are the only ones supported as of now.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/crypto_user_base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/crypto/crypto_user_base.c b/crypto/crypto_user_base.c
index 3fa20f12989f..a9c06cd98a8c 100644
--- a/crypto/crypto_user_base.c
+++ b/crypto/crypto_user_base.c
@@ -352,6 +352,9 @@ static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (priority && !exact)
 		return -EINVAL;
 
+	if ((p->cru_type ^ p->cru_mask) & CRYPTO_ALG_TESTED)
+		return -EINVAL;
+
 	alg = crypto_alg_match(p, exact);
 	if (alg) {
 		crypto_mod_put(alg);
-- 
2.26.2

[PATCH 3/8] crypto: api - only support lookups for specific CRYPTO_ALG_TESTED status

[Nicolai Stange]

Commit ff753308d2f7 ("crypto: api - crypto_alg_mod_lookup either tested
or untested") introduced support to crypto_alg_lookup() for
ignoring the CRYPTO_ALG_TESTED status in the search. According to the
patch description, this had been needed at the time for "constructing
givcipher and aead". Callers of crypto_alg_lookup() would enable this
behaviour by the special combination of setting CRYPTO_ALG_TESTED in
type only, but not in mask.

However, the last user invoking this functionality has been gone with
commit 3a01d0ee2b99 ("crypto: skcipher - Remove top-level givcipher
interface"). With the previous two patches making the userspace facing APIs
to validate the mask and type values passed in, the combination of
(type & CRYPTO_ALG_TESTED), but !(mask & CRYPTO_ALG_TESTED) is not
possible in crypto_alg_lookup() anymore.

In preparation for subsequent patches, make this explicit by effectively
applying what is a revert of commit ff753308d2f7 ("crypto: api -
crypto_alg_mod_lookup either tested or untested") and adding a
corresponding WARN_ON_ONCE() to crypto_alg_mod_lookup().

There is no change in behaviour.

Note that it is now guaranteed that the first  __crypto_alg_lookup()
invocation from crypto_alg_mod_lookup() will always have CRYPTO_ALG_TESTED
in the mask passed in, which will be needed for a subsequent patch
enforcing the same for lookup larvals.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/api.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/api.c b/crypto/api.c
index ee5991fe11f8..5cced204b6b4 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -239,8 +239,10 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
 	struct crypto_alg *alg;
 	u32 test = 0;
 
-	if (!((type | mask) & CRYPTO_ALG_TESTED))
+	if (!(mask & CRYPTO_ALG_TESTED)) {
+		WARN_ON_ONCE(type & CRYPTO_ALG_TESTED);
 		test |= CRYPTO_ALG_TESTED;
+	}
 
 	down_read(&crypto_alg_sem);
 	alg = __crypto_alg_lookup(name, type | test, mask | test);
-- 
2.26.2

[PATCH 4/8] crypto: api - don't add larvals for !(type & CRYPTO_ALG_TESTED) lookups

[Nicolai Stange]

As of now, the only valid (and existent) usecase for requesting an
algorithm with !(type & CRYPTO_ALG_TESTED) and (mask & CRYPTO_ALG_TESTED)
via crypto_alg_mod_lookup() is the testmgr invoked upon a
CRYPTO_MSG_ALG_REGISTER notification.

However, in this case it is expected and required that the lookup returns
the actual crypto_alg instance subject to testing.

If OTOH we were to assume for a moment that the crypto_larval_lookup()
called from crypto_alg_mod_lookup() would allocate and return a lookup
larval (which cannot happen, except for perhaps in some corner case like
with concurrent crypto_alg unregistrations), the subsequent wait in
crypto_alg_mod_lookup() would timeout anyway.

Make crypto_larval_wait() skip the lookup larval allocation if the
specified values of mask and type indicate a lookup request for some
crypto_alg instance still not in CRYPTO_ALG_TESTED state and the search for
such one failed. Instead, fail the call with -ENOENT in this case.

This reduces the number of possible ->mask and ->type configurations with
respect to CRYPTO_ALG_TESTED for pending lookup larvals and will enable
further cleanup with subsequent patches.

There is no change in functionality.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/api.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/crypto/api.c b/crypto/api.c
index 5cced204b6b4..3bee52d7fe4b 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -283,8 +283,22 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 
 	if (!IS_ERR_OR_NULL(alg) && crypto_is_larval(alg))
 		alg = crypto_larval_wait(alg);
-	else if (!alg)
-		alg = crypto_larval_add(name, type, mask);
+	else if (!alg) {
+		/*
+		 * Only add a lookup larval if the request is for a
+		 * tested algorithm, everything else makes no sense.
+		 */
+		bool tested = type & mask & CRYPTO_ALG_TESTED;
+
+		if (!(mask & CRYPTO_ALG_TESTED)) {
+			WARN_ON_ONCE((type & CRYPTO_ALG_TESTED));
+			tested = true;
+		}
+		if (tested)
+			alg = crypto_larval_add(name, type, mask);
+		else
+			alg = ERR_PTR(-ENOENT);
+	}
 
 	return alg;
 }
-- 
2.26.2

[PATCH 5/8] crypto: api - always set CRYPTO_ALG_TESTED in lookup larvals' ->mask/type

[Nicolai Stange]

For crypto_alg_lookup(), the input parameter combination of mask and type
where both have CRYPTO_ALG_TESTED unset is special in that it is supposed
to be handled as if said flag had been set in both for convenience.

However, the code in crypto_alg_lookup() is not equivalent between these
two cases:
- If CRYPTO_ALG_TESTED is unset in both type and mask, a pending
  lookup larval with or without ->CRYPTO_ALG_TESTED being set in its
  ->mask and ->type can potentially get returned, whereas if
  CRYPTO_ALG_TESTED is set in both, only the former is possible.
- If CRYPTO_ALG_TESTED is unset in both type and mask, an error would get
  returned if there was one or more matching algorithms which all had
  failed their selftests. If OTOH CRYPTO_ALG_TESTED had been set in both
  type and mask, NULL would get returned in this case of universal failure
  and the caller would proceed to re-request the algorithm in question,
  which is not desired.

Even though the first issue is only a cosmetic one and does not affect
functionality in any way AFAICT, addressing it with this patch here will
improve consistency and allow for streamlining crypto_alg_lookup() a bit
later in this series. The second item in the list from above will be left
to a later patch.

Remember that crypto_larval_lookup() will only invoke crypto_larval_add()
if CRYPTO_ALG_TESTED has been set in either none or both of mask and type.
Make crypto_larval_lookup() always set CRYPTO_ALG_TESTED for the mask
and type values passed to crypto_larval_add(). That is, the "issue"
described above is being addressed by sticking to a canonical lookup larval
representation for the two equivalent cases.

There is no change in functionality.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/api.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/crypto/api.c b/crypto/api.c
index 3bee52d7fe4b..71406f486c65 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -284,17 +284,17 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 	if (!IS_ERR_OR_NULL(alg) && crypto_is_larval(alg))
 		alg = crypto_larval_wait(alg);
 	else if (!alg) {
+		if (!(mask & CRYPTO_ALG_TESTED)) {
+			WARN_ON_ONCE((type & CRYPTO_ALG_TESTED));
+			mask |= CRYPTO_ALG_TESTED;
+			type |= CRYPTO_ALG_TESTED;
+		}
+
 		/*
 		 * Only add a lookup larval if the request is for a
 		 * tested algorithm, everything else makes no sense.
 		 */
-		bool tested = type & mask & CRYPTO_ALG_TESTED;
-
-		if (!(mask & CRYPTO_ALG_TESTED)) {
-			WARN_ON_ONCE((type & CRYPTO_ALG_TESTED));
-			tested = true;
-		}
-		if (tested)
+		if (type & CRYPTO_ALG_TESTED)
 			alg = crypto_larval_add(name, type, mask);
 		else
 			alg = ERR_PTR(-ENOENT);
-- 
2.26.2

[PATCH 6/8] crypto: api - make crypto_alg_lookup() consistently check for failed algos

[Nicolai Stange]

For convenience, input parameters to crypto_alg_lookup() where
!(mask & CRYPTO_ALG_TESTED) holds are supposed to be equivalent to those
having CRYPTO_ALG_TESTED set in both type and mask.

However the crypto_alg_lookup() code does not behave equivalently in
these two cases, in particular not in regard to the additional check for
failed algorithms introduced with commit eb02c38f0197 ("crypto: api - Keep
failed instances alive").

That is, if the user had explicitly set CRYPTO_ALG_TESTED in mask, the
additional check for failed algorithms would never get to run.

Make crypto_alg_lookup() behave the same for parameter sets considered
equivalent.

Note that with the current code, the first invocation of
__crypto_alg_lookup() from crypto_alg_lookup() always receives a mask with
CRYPTO_ALG_TESTED being set: either because that flag had been set from the
beginning or because the local variable 'test' has been set to
CRYPTO_ALG_TESTED otherwise. As lookup larvals always have
CRYPTO_ALG_TESTED included in their ->mask by now, the first
__crypto_alg_lookup() is guaranteed to find such lookup larvals. In
particular, the second call of __crypto_alg_lookup(), which used to get
invoked with a mask having CRYPTO_ALG_TESTED clear, is not needed anymore
for matching any existing lookup larvals and, in fact, won't find any. In
this context, also c.f. commit b346e492d712 ("crypto: api - fix finding
algorithm currently being tested").

Moreover, because testing larvals have CRYPTO_ALG_TESTED set in their
->type, the first __crypto_alg_lookup() is and always has been able to
find those, for any suitable initial combination of mask and value.

In summary,
- the second __crypto_alg_lookup() won't ever return a larval by now and
- invoking it with the original values of mask and value, i.e. those
  specified by the user at function entry, is not needed anymore.

As it currently stands, the only purpose of that second
__crypto_alg_lookup() invocation is to check whether all matching
algorithms, if any, have failed their selftests.

This allows  for the elimination of the local 'test' variable and some
minor code simplification in crypto_alg_lookup(). More specifically,
rather than keeping track of whether or not CRYPTO_ALG_TESTED had
initially been set via said local 'test', simply set CRYPTO_ALG_TESTED
in both mask and type at function entry if it had initially been unset in
mask. That is, enforce equivalent behaviour for parameter sets considered
equivalent.

Adapt the if-condition guarding that second __crypto_alg_lookup()
invocation as well as the invocation itself accordingly: the search is to
be conducted only in the common case of the user asking for tested
algorithms and it should watch out for failed algorithms, i.e. those
crypto_alg instances having CRYPTO_ALG_TESTED clear in their ->type.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/api.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/crypto/api.c b/crypto/api.c
index 71406f486c65..b96b65b3d5c7 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -237,18 +237,24 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
 					    u32 mask)
 {
 	struct crypto_alg *alg;
-	u32 test = 0;
 
 	if (!(mask & CRYPTO_ALG_TESTED)) {
 		WARN_ON_ONCE(type & CRYPTO_ALG_TESTED);
-		test |= CRYPTO_ALG_TESTED;
+		mask |= CRYPTO_ALG_TESTED;
+		type |= CRYPTO_ALG_TESTED;
 	}
 
 	down_read(&crypto_alg_sem);
-	alg = __crypto_alg_lookup(name, type | test, mask | test);
-	if (!alg && test) {
+	alg = __crypto_alg_lookup(name, type, mask);
+	if (!alg && (type & CRYPTO_ALG_TESTED)) {
+		/*
+		 * Check whether there's an instance which failed the
+		 * selftests in order to avoid pointless retries.
+		 */
+		type &= ~CRYPTO_ALG_TESTED;
 		alg = __crypto_alg_lookup(name, type, mask);
-		if (alg && !crypto_is_larval(alg)) {
+		WARN_ON_ONCE(alg && crypto_is_larval(alg));
+		if (alg) {
 			/* Test failed */
 			crypto_mod_put(alg);
 			alg = ERR_PTR(-ELIBBAD);
-- 
2.26.2

[PATCH 7/8] crypto: api - lift common mask + type adjustment to crypto_larval_lookup()

[Nicolai Stange]

Both crypto_larval_lookup() as well as the crypto_alg_lookup() called
therefrom (and only therefrom) will apply the very same adjustments to
their mask and type parameters if the original mask value is found to not
have CRYPTO_ALG_TESTED set.

There is no point in having the code duplicated, do it once near the entry
of crypto_larval_lookup().

There is no change in behaviour.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/api.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/crypto/api.c b/crypto/api.c
index b96b65b3d5c7..594c494a27d9 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -238,12 +238,6 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
 {
 	struct crypto_alg *alg;
 
-	if (!(mask & CRYPTO_ALG_TESTED)) {
-		WARN_ON_ONCE(type & CRYPTO_ALG_TESTED);
-		mask |= CRYPTO_ALG_TESTED;
-		type |= CRYPTO_ALG_TESTED;
-	}
-
 	down_read(&crypto_alg_sem);
 	alg = __crypto_alg_lookup(name, type, mask);
 	if (!alg && (type & CRYPTO_ALG_TESTED)) {
@@ -276,6 +270,12 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 	type &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
 	mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD);
 
+	if (!(mask & CRYPTO_ALG_TESTED)) {
+		WARN_ON_ONCE(type & CRYPTO_ALG_TESTED);
+		mask |= CRYPTO_ALG_TESTED;
+		type |= CRYPTO_ALG_TESTED;
+	}
+
 	alg = crypto_alg_lookup(name, type, mask);
 	if (!alg && !(mask & CRYPTO_NOLOAD)) {
 		request_module("crypto-%s", name);
@@ -290,12 +290,6 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type,
 	if (!IS_ERR_OR_NULL(alg) && crypto_is_larval(alg))
 		alg = crypto_larval_wait(alg);
 	else if (!alg) {
-		if (!(mask & CRYPTO_ALG_TESTED)) {
-			WARN_ON_ONCE((type & CRYPTO_ALG_TESTED));
-			mask |= CRYPTO_ALG_TESTED;
-			type |= CRYPTO_ALG_TESTED;
-		}
-
 		/*
 		 * Only add a lookup larval if the request is for a
 		 * tested algorithm, everything else makes no sense.
-- 
2.26.2

[PATCH 8/8] crypto: api - make the algorithm lookup priorize non-larvals

[Nicolai Stange]

crypto_alg_mod_lookup() invokes the crypto_larval_lookup() helper
to run the actual search for matching crypto_alg implementation and
larval entries. The latter is currently considering only the individual
entries' relative ->cra_priority for determining which one out of multiple
matches to return. This means that it would potentially dismiss a matching
crypto_alg implementation in working state in favor of some pending
testing larval of higher ->cra_priority. Now, if the testmgr instance
invoked asynchronously on that testing larval came to the conclusion that
it should mark the tests as failed, any pending crypto_alg_mod_lookup()
waiting for it would be made to fail as well with -EAGAIN.

In summary, crypto_alg_mod_lookup() can fail spuriously with -EAGAIN even
though an implementation in working state would have been available, namely
if the testmgr asynchronously marked another, competing implementation of
higher ->cra_priority as failed.

This is normally not a problem at all with upstream, because the situation
where one algorithm passed its tests, but another competing one failed to
do so, would indicate a bug anyway.

However, for downstream distributions seeking FIPS certification, simply
amending the list in crypto/testmgr.c with ->fips_allowed = 0 entries
matching on ->cra_driver_name would provide a convenient way of
selectively blacklisting implementations from drivers/crypto in fips
mode. Note that in this scenario failure of competing crypto_alg
implementations would become more common, in particular during device
enumeration. If the algorithm in question happened to be needed for e.g.
module signature verification, module loading could spuriously fail during
bootup, which is certainly not desired.

For transparency: this has not actually been observed, I merely came to
the conclusion that it would be possible by reading the code.

Make crypto_alg_lookup() run an additional search for non-larval matches
upfront in the common case that the request has been made for
CRYPTO_ALG_TESTED instances.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/api.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/crypto/api.c b/crypto/api.c
index 594c494a27d9..4251eedef668 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -239,8 +239,25 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
 	struct crypto_alg *alg;
 
 	down_read(&crypto_alg_sem);
+	/*
+	 * If the request is for an algorithm having passed its
+	 * selftests, try to serve it with a matching instance already
+	 * in operational state. That is, skip pending larvals in a
+	 * first search: for these it is not guaranteed that they will
+	 * eventually turn out successful and it would be a pity to
+	 * potentially fail the request even though a working
+	 * implementation would have been available. If OTOH the
+	 * request is *not* for an algorithm having passed its
+	 * selftest (yet), no larval can match it anyway, so the
+	 * CRYPTO_ALG_LARVAL in the mask below won't make a
+	 * difference.
+	 */
+	alg = __crypto_alg_lookup(name, type, mask | CRYPTO_ALG_LARVAL);
+	if (alg || !(type & CRYPTO_ALG_TESTED))
+		goto out;
+
 	alg = __crypto_alg_lookup(name, type, mask);
-	if (!alg && (type & CRYPTO_ALG_TESTED)) {
+	if (!alg) {
 		/*
 		 * Check whether there's an instance which failed the
 		 * selftests in order to avoid pointless retries.
@@ -254,6 +271,8 @@ static struct crypto_alg *crypto_alg_lookup(const char *name, u32 type,
 			alg = ERR_PTR(-ELIBBAD);
 		}
 	}
+
+out:
 	up_read(&crypto_alg_sem);
 
 	return alg;
-- 
2.26.2

Re: [PATCH 8/8] crypto: api - make the algorithm lookup priorize non-larvals

[Herbert Xu]

On Sun, Oct 03, 2021 at 08:14:13PM +0200, Nicolai Stange wrote:
> crypto_alg_mod_lookup() invokes the crypto_larval_lookup() helper
> to run the actual search for matching crypto_alg implementation and
> larval entries. The latter is currently considering only the individual
> entries' relative ->cra_priority for determining which one out of multiple
> matches to return. This means that it would potentially dismiss a matching
> crypto_alg implementation in working state in favor of some pending
> testing larval of higher ->cra_priority. Now, if the testmgr instance
> invoked asynchronously on that testing larval came to the conclusion that
> it should mark the tests as failed, any pending crypto_alg_mod_lookup()
> waiting for it would be made to fail as well with -EAGAIN.
> 
> In summary, crypto_alg_mod_lookup() can fail spuriously with -EAGAIN even
> though an implementation in working state would have been available, namely
> if the testmgr asynchronously marked another, competing implementation of
> higher ->cra_priority as failed.
> 
> This is normally not a problem at all with upstream, because the situation
> where one algorithm passed its tests, but another competing one failed to
> do so, would indicate a bug anyway.
> 
> However, for downstream distributions seeking FIPS certification, simply
> amending the list in crypto/testmgr.c with ->fips_allowed = 0 entries
> matching on ->cra_driver_name would provide a convenient way of
> selectively blacklisting implementations from drivers/crypto in fips
> mode. Note that in this scenario failure of competing crypto_alg
> implementations would become more common, in particular during device
> enumeration. If the algorithm in question happened to be needed for e.g.
> module signature verification, module loading could spuriously fail during
> bootup, which is certainly not desired.
> 
> For transparency: this has not actually been observed, I merely came to
> the conclusion that it would be possible by reading the code.
> 
> Make crypto_alg_lookup() run an additional search for non-larval matches
> upfront in the common case that the request has been made for
> CRYPTO_ALG_TESTED instances.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>  crypto/api.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)

It's not clear that this new behaviour is desirable.  For example,
when we construct certain complex algorithms, they may depend on a
generic version of that same algorithm as a fallback.  We do not
want users to get the generic version while the better version is
being tested.

Can you please explain what your failure scenario and perhaps we
can come up with another way of resolving your problem?

Thanks,
-- 
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

Re: [PATCH 8/8] crypto: api - make the algorithm lookup priorize non-larvals

[Nicolai Stange]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Sun, Oct 03, 2021 at 08:14:13PM +0200, Nicolai Stange wrote:
>> crypto_alg_mod_lookup() invokes the crypto_larval_lookup() helper
>> to run the actual search for matching crypto_alg implementation and
>> larval entries. The latter is currently considering only the individual
>> entries' relative ->cra_priority for determining which one out of multiple
>> matches to return. This means that it would potentially dismiss a matching
>> crypto_alg implementation in working state in favor of some pending
>> testing larval of higher ->cra_priority. Now, if the testmgr instance
>> invoked asynchronously on that testing larval came to the conclusion that
>> it should mark the tests as failed, any pending crypto_alg_mod_lookup()
>> waiting for it would be made to fail as well with -EAGAIN.
>> 
>> In summary, crypto_alg_mod_lookup() can fail spuriously with -EAGAIN even
>> though an implementation in working state would have been available, namely
>> if the testmgr asynchronously marked another, competing implementation of
>> higher ->cra_priority as failed.
>> 
>> This is normally not a problem at all with upstream, because the situation
>> where one algorithm passed its tests, but another competing one failed to
>> do so, would indicate a bug anyway.
>> 
>> However, for downstream distributions seeking FIPS certification, simply
>> amending the list in crypto/testmgr.c with ->fips_allowed = 0 entries
>> matching on ->cra_driver_name would provide a convenient way of
>> selectively blacklisting implementations from drivers/crypto in fips
>> mode. Note that in this scenario failure of competing crypto_alg
>> implementations would become more common, in particular during device
>> enumeration. If the algorithm in question happened to be needed for e.g.
>> module signature verification, module loading could spuriously fail during
>> bootup, which is certainly not desired.
>> 
>> For transparency: this has not actually been observed, I merely came to
>> the conclusion that it would be possible by reading the code.
>> 
>> Make crypto_alg_lookup() run an additional search for non-larval matches
>> upfront in the common case that the request has been made for
>> CRYPTO_ALG_TESTED instances.
>> 
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>> ---
>>  crypto/api.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)

Hi Herbert,

> It's not clear that this new behaviour is desirable.  For example,
> when we construct certain complex algorithms, they may depend on a
> generic version of that same algorithm as a fallback.  We do not
> want users to get the generic version while the better version is
> being tested.

Ah I see, you mean something like having 3+ providers of "algXY",
 - algXY_driver0, ->cra_priority = 100
 - algXY_driver1, ->cra_priority = 200
 - algXY_driver1, ->cra_priority = 300
where the latter needs a different "algXY" as a fallback?

Hmm yes, I haven't thought of this and my patch here would indeed break
it.


> Can you please explain what your failure scenario and perhaps we
> can come up with another way of resolving your problem?

In order to keep a FIPS certification manageable in terms of scope,
we're looking for a way to disable everything under drivers/crypto iff
fips_enabled == 1. The most convenient way to achieve this downstream
would be to add dummy entries to testmgr.c like so:

static int alg_test_nop(const struct alg_test_desc *desc, const char *driver,
			u32 type, u32 mask)
{
	/* Succeed in non-FIPS mode. */
	return 0;
}

static const struct alg_test_desc alg_test_descs[] = {
	...,
	{
		.alg = "sha256-padlock-nano",
		.test = alg_test_nop,
		.fips_allowed = 0,
	},
        ...
};

The concern is about e.g the following sequence of events during boot:
- "sha256-padlock-nano" gets registered, the test gets scheduled.
- An unrelated modprobe is getting invoked. The signature verification
  code, e.g pkcs7_digest(), requests "sha256", finds the pending
  "sha256-padlock-nano" testing larval and puts itself in a wait for it.
- The scheduled "sha256-padlock-nano" test gets to run and, as per
  ->fips_allowed == 0, is forced to fail with -EINVAL.
- pkcs7_digest() wakes up and fails with -EAGAIN due to the "failed"
  testing larval.
- The unrelated modprobe fails even though sha256-generic would
  have been available all the time.

FWIW, I picked sha256-padlock-nano and modprobe at random for the sake
of providing an example here -- I haven't checked in detail, but I guess
that e.g. the combination of dm-crypt + a number of different FIPS
approved algorithms could be similarly susceptible, too.

So to recap, my initial approach for working around the above was to
make crypto_alg_lookup() to skip over testing larvals in an additional,
first search. As you said, this would break the "fallback" scenario
though.

As an alternative, how about not doing the additional search for
non-larvals upfront, but only as a resort in case crypto_larval_wait()
returned failure instead?

But granted, the scenario above is not an issue at all for upstream with
a pristine testmgr.c and it would be quite relatable if you wouldn't
like to get bothered with any of this. I'm only bringing it up because
others might perhaps come across this as well...


Thanks!

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 8/8] crypto: api - make the algorithm lookup priorize non-larvals

[Herbert Xu]

On Mon, Oct 11, 2021 at 10:34:11AM +0200, Nicolai Stange wrote:
>
> In order to keep a FIPS certification manageable in terms of scope,
> we're looking for a way to disable everything under drivers/crypto iff
> fips_enabled == 1. The most convenient way to achieve this downstream
> would be to add dummy entries to testmgr.c like so:

How about testing based on the flag CRYPTO_ALG_KERN_DRIVER_ONLY?
That flag is meant to correspond to pretty much exactly drivers/crypto.

Cheers,
-- 
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

Re: [PATCH 8/8] crypto: api - make the algorithm lookup priorize non-larvals

[Nicolai Stange]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Mon, Oct 11, 2021 at 10:34:11AM +0200, Nicolai Stange wrote:
>>
>> In order to keep a FIPS certification manageable in terms of scope,
>> we're looking for a way to disable everything under drivers/crypto iff
>> fips_enabled == 1. The most convenient way to achieve this downstream
>> would be to add dummy entries to testmgr.c like so:
>
> How about testing based on the flag CRYPTO_ALG_KERN_DRIVER_ONLY?
> That flag is meant to correspond to pretty much exactly
> drivers/crypto.

Hmm, I checked a couple of crypto/drivers and it seems like not all are
setting this flag: random examples would include cavium/nitrox/,
chelsio/, padlock*.c, vmx/, ...

Many thanks!

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 8/8] crypto: api - make the algorithm lookup priorize non-larvals

[Herbert Xu]

On Wed, Oct 27, 2021 at 11:59:26AM +0200, Nicolai Stange wrote:
>
> Hmm, I checked a couple of crypto/drivers and it seems like not all are
> setting this flag: random examples would include cavium/nitrox/,
> chelsio/, padlock*.c, vmx/, ...

For padlock and vmx this is correct as they're just like aesni.

The others are omissions and should be fixed.

Thanks,
-- 
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