[PATCH] crypto: testmgr - don't generate WARN for missing modules

[PATCH] crypto: testmgr - don't generate WARN for missing modules

[Robert Elliott]

This userspace command:
    modprobe tcrypt
or
    modprobe tcrypt mode=0

runs all the tcrypt test cases numbered <200 (i.e., all the
test cases calling tcrypt_test() and returning return values).

Tests are sparsely numbered from 0 to 1000. For example:
    modprobe tcrypt mode=12
tests sha512, and
    modprobe tcrypt mode=152
tests rfc4543(gcm(aes))) - AES-GCM as GMAC

The test manager generates WARNING crashdumps every time it attempts
a test using an algorithm that is not available (not built-in to the
kernel or available as a module):

    alg: skcipher: failed to allocate transform for ecb(arc4): -2
    ------------[ cut here ]-----------
    alg: self-tests for ecb(arc4) (ecb(arc4)) failed (rc=-2)
    WARNING: CPU: 9 PID: 4618 at crypto/testmgr.c:5777
alg_test+0x30b/0x510
    [50 more lines....]

    ---[ end trace 0000000000000000 ]---

If the kernel is compiled with CRYPTO_USER_API_ENABLE_OBSOLETE
disabled (the default), then these algorithms are not compiled into
the kernel or made into modules and trigger WARNINGs:
    arc4 tea xtea khazad anubis xeta seed

Additionally, any other algorithms that are not enabled in .config
will generate WARNINGs. In RHEL 9.0, for example, the default
selection of algorithms leads to 16 WARNING dumps.

One attempt to fix this was by modifying tcrypt_test() to check
crypto_has_alg() and immediately return 0 if crypto_has_alg() fails,
rather than proceed and return a non-zero error value that causes
the caller (alg_test() in crypto/testmgr.c) to invoke WARN().
That knocks out too many algorithms, though; some combinations
like ctr(des3_ede) would work.

Instead, change the condition on the WARN to ignore a return
value is ENOENT, which is the value returned when the algorithm
or combination of algorithms doesn't exist. Add a pr_warn to
communicate that information in case the WARN is skipped.

This approach allows algorithm tests to work that are combinations,
not provided by one driver, like ctr(blowfish).

Result - no more WARNINGs:
modprobe tcrypt
[  115.541765] tcrypt: testing md5
[  115.556415] tcrypt: testing sha1
[  115.570463] tcrypt: testing ecb(des)
[  115.585303] cryptomgr: alg: skcipher: failed to allocate transform for ecb(des): -2
[  115.593037] cryptomgr: alg: self-tests for ecb(des) using ecb(des) failed (rc=-2)
[  115.593038] tcrypt: testing cbc(des)
[  115.610641] cryptomgr: alg: skcipher: failed to allocate transform for cbc(des): -2
[  115.618359] cryptomgr: alg: self-tests for cbc(des) using cbc(des) failed (rc=-2)
...

Signed-off-by: Robert Elliott <elliott@hpe.com>
---
 crypto/testmgr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 5801a8f9f713..c28fb0a7811d 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -5774,8 +5774,11 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
 			      driver, alg,
 			      fips_enabled ? "fips" : "panic_on_fail");
 		}
-		WARN(1, "alg: self-tests for %s (%s) failed (rc=%d)",
-		     driver, alg, rc);
+		pr_warn("alg: self-tests for %s using %s failed (rc=%d)",
+			alg, driver, rc);
+		WARN(rc != -ENOENT,
+		     "alg: self-tests for %s using %s failed (rc=%d)",
+		     alg, driver, rc);
 	} else {
 		if (fips_enabled)
 			pr_info("alg: self-tests for %s (%s) passed\n",
-- 
2.37.1

Re: [PATCH] crypto: testmgr - don't generate WARN for missing modules

[Eric Biggers]

On Sat, Aug 13, 2022 at 06:14:43PM -0500, Robert Elliott wrote:
> This userspace command:
>     modprobe tcrypt
> or
>     modprobe tcrypt mode=0
> 
> runs all the tcrypt test cases numbered <200 (i.e., all the
> test cases calling tcrypt_test() and returning return values).
> 
> Tests are sparsely numbered from 0 to 1000. For example:
>     modprobe tcrypt mode=12
> tests sha512, and
>     modprobe tcrypt mode=152
> tests rfc4543(gcm(aes))) - AES-GCM as GMAC
> 
> The test manager generates WARNING crashdumps every time it attempts
> a test using an algorithm that is not available (not built-in to the
> kernel or available as a module):

Note that this is only a problem because tcrypt calls alg_test() directly.  The
normal way that alg_test() gets called is for the registration-time self-test.
It's not clear to me why tcrypt calls alg_test() directly; the registration-time
test should be enough.  Herbert, do you know?

- Eric

Re: [PATCH] crypto: testmgr - don't generate WARN for missing modules

[Herbert Xu]

On Mon, Aug 15, 2022 at 02:30:13PM -0700, Eric Biggers wrote:
>
> Note that this is only a problem because tcrypt calls alg_test() directly.  The
> normal way that alg_test() gets called is for the registration-time self-test.
> It's not clear to me why tcrypt calls alg_test() directly; the registration-time
> test should be enough.  Herbert, do you know?

The tcrypt code predates testmgr.  So at the beginning we only had
the enumerative testing.  Registration-time testing was added later.

We could remove the enumerative testing, but I think the FIPS people
have grown rather attached to it because it ticks some sort of a box
at boot-time.

Stephane, would it be a problem for FIPS if we simply got rid of the
enumerative testing in tcrypt and instead relied on registration-time
testing?

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] crypto: testmgr - don't generate WARN for missing modules

[Stephan Mueller]

Am Dienstag, 16. August 2022, 04:43:03 CEST schrieb Herbert Xu:

Hi Herbert,

> On Mon, Aug 15, 2022 at 02:30:13PM -0700, Eric Biggers wrote:
> > Note that this is only a problem because tcrypt calls alg_test() directly.
> >  The normal way that alg_test() gets called is for the registration-time
> > self-test. It's not clear to me why tcrypt calls alg_test() directly; the
> > registration-time test should be enough.  Herbert, do you know?
> 
> The tcrypt code predates testmgr.  So at the beginning we only had
> the enumerative testing.  Registration-time testing was added later.
> 
> We could remove the enumerative testing, but I think the FIPS people
> have grown rather attached to it because it ticks some sort of a box
> at boot-time.
> 
> Stephane, would it be a problem for FIPS if we simply got rid of the
> enumerative testing in tcrypt and instead relied on registration-time
> testing?

The tcrypt code has only one purpose for FIPS: to allocate all crypto 
algorithms at boot time and thus to trigger the self test during boot time. 
That was a requirement until some time ago. These requirements were relaxed a 
bit such that a self test before first use is permitted, i.e. the approach we 
have in testmgr.c.

Therefore, presently we do not need this boot-time allocation of an algorithm 
via tcrypt which means that from a FIPS perspective tcrypt is no longer 
required.
> 
> Cheers,


Ciao
Stephan

Re: [PATCH] crypto: testmgr - don't generate WARN for missing modules

[Herbert Xu]

On Tue, Aug 16, 2022 at 07:09:44AM +0200, Stephan Mueller wrote:
>
> The tcrypt code has only one purpose for FIPS: to allocate all crypto 
> algorithms at boot time and thus to trigger the self test during boot time. 
> That was a requirement until some time ago. These requirements were relaxed a 
> bit such that a self test before first use is permitted, i.e. the approach we 
> have in testmgr.c.
> 
> Therefore, presently we do not need this boot-time allocation of an algorithm 
> via tcrypt which means that from a FIPS perspective tcrypt is no longer 
> required.

Hi Stephan, Eric:

That makes sense.  So the tcrypt code also has the side-effect
of instantiating all the algorithms which testmgr does not do.

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] crypto: testmgr - don't generate WARN for missing modules

[Herbert Xu]

Robert Elliott <elliott@hpe.com> wrote:
> This userspace command:
>    modprobe tcrypt
> or
>    modprobe tcrypt mode=0
> 
> runs all the tcrypt test cases numbered <200 (i.e., all the
> test cases calling tcrypt_test() and returning return values).
> 
> Tests are sparsely numbered from 0 to 1000. For example:
>    modprobe tcrypt mode=12
> tests sha512, and
>    modprobe tcrypt mode=152
> tests rfc4543(gcm(aes))) - AES-GCM as GMAC
> 
> The test manager generates WARNING crashdumps every time it attempts
> a test using an algorithm that is not available (not built-in to the
> kernel or available as a module):
> 
>    alg: skcipher: failed to allocate transform for ecb(arc4): -2
>    ------------[ cut here ]-----------
>    alg: self-tests for ecb(arc4) (ecb(arc4)) failed (rc=-2)
>    WARNING: CPU: 9 PID: 4618 at crypto/testmgr.c:5777
> alg_test+0x30b/0x510
>    [50 more lines....]
> 
>    ---[ end trace 0000000000000000 ]---
> 
> If the kernel is compiled with CRYPTO_USER_API_ENABLE_OBSOLETE
> disabled (the default), then these algorithms are not compiled into
> the kernel or made into modules and trigger WARNINGs:
>    arc4 tea xtea khazad anubis xeta seed
> 
> Additionally, any other algorithms that are not enabled in .config
> will generate WARNINGs. In RHEL 9.0, for example, the default
> selection of algorithms leads to 16 WARNING dumps.
> 
> One attempt to fix this was by modifying tcrypt_test() to check
> crypto_has_alg() and immediately return 0 if crypto_has_alg() fails,
> rather than proceed and return a non-zero error value that causes
> the caller (alg_test() in crypto/testmgr.c) to invoke WARN().
> That knocks out too many algorithms, though; some combinations
> like ctr(des3_ede) would work.
> 
> Instead, change the condition on the WARN to ignore a return
> value is ENOENT, which is the value returned when the algorithm
> or combination of algorithms doesn't exist. Add a pr_warn to
> communicate that information in case the WARN is skipped.
> 
> This approach allows algorithm tests to work that are combinations,
> not provided by one driver, like ctr(blowfish).
> 
> Result - no more WARNINGs:
> modprobe tcrypt
> [  115.541765] tcrypt: testing md5
> [  115.556415] tcrypt: testing sha1
> [  115.570463] tcrypt: testing ecb(des)
> [  115.585303] cryptomgr: alg: skcipher: failed to allocate transform for ecb(des): -2
> [  115.593037] cryptomgr: alg: self-tests for ecb(des) using ecb(des) failed (rc=-2)
> [  115.593038] tcrypt: testing cbc(des)
> [  115.610641] cryptomgr: alg: skcipher: failed to allocate transform for cbc(des): -2
> [  115.618359] cryptomgr: alg: self-tests for cbc(des) using cbc(des) failed (rc=-2)
> ...
> 
> Signed-off-by: Robert Elliott <elliott@hpe.com>
> ---
> crypto/testmgr.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)

Patch applied.  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] crypto: testmgr - don't generate WARN for missing modules

[Eric Biggers]

On Fri, Aug 19, 2022 at 07:05:41PM +0800, Herbert Xu wrote:
> Robert Elliott <elliott@hpe.com> wrote:
> > This userspace command:
> >    modprobe tcrypt
> > or
> >    modprobe tcrypt mode=0
> > 
> > runs all the tcrypt test cases numbered <200 (i.e., all the
> > test cases calling tcrypt_test() and returning return values).
> > 
> > Tests are sparsely numbered from 0 to 1000. For example:
> >    modprobe tcrypt mode=12
> > tests sha512, and
> >    modprobe tcrypt mode=152
> > tests rfc4543(gcm(aes))) - AES-GCM as GMAC
> > 
> > The test manager generates WARNING crashdumps every time it attempts
> > a test using an algorithm that is not available (not built-in to the
> > kernel or available as a module):
> > 
> >    alg: skcipher: failed to allocate transform for ecb(arc4): -2
> >    ------------[ cut here ]-----------
> >    alg: self-tests for ecb(arc4) (ecb(arc4)) failed (rc=-2)
> >    WARNING: CPU: 9 PID: 4618 at crypto/testmgr.c:5777
> > alg_test+0x30b/0x510
> >    [50 more lines....]
> > 
> >    ---[ end trace 0000000000000000 ]---
> > 
> > If the kernel is compiled with CRYPTO_USER_API_ENABLE_OBSOLETE
> > disabled (the default), then these algorithms are not compiled into
> > the kernel or made into modules and trigger WARNINGs:
> >    arc4 tea xtea khazad anubis xeta seed
> > 
> > Additionally, any other algorithms that are not enabled in .config
> > will generate WARNINGs. In RHEL 9.0, for example, the default
> > selection of algorithms leads to 16 WARNING dumps.
> > 
> > One attempt to fix this was by modifying tcrypt_test() to check
> > crypto_has_alg() and immediately return 0 if crypto_has_alg() fails,
> > rather than proceed and return a non-zero error value that causes
> > the caller (alg_test() in crypto/testmgr.c) to invoke WARN().
> > That knocks out too many algorithms, though; some combinations
> > like ctr(des3_ede) would work.
> > 
> > Instead, change the condition on the WARN to ignore a return
> > value is ENOENT, which is the value returned when the algorithm
> > or combination of algorithms doesn't exist. Add a pr_warn to
> > communicate that information in case the WARN is skipped.
> > 
> > This approach allows algorithm tests to work that are combinations,
> > not provided by one driver, like ctr(blowfish).
> > 
> > Result - no more WARNINGs:
> > modprobe tcrypt
> > [  115.541765] tcrypt: testing md5
> > [  115.556415] tcrypt: testing sha1
> > [  115.570463] tcrypt: testing ecb(des)
> > [  115.585303] cryptomgr: alg: skcipher: failed to allocate transform for ecb(des): -2
> > [  115.593037] cryptomgr: alg: self-tests for ecb(des) using ecb(des) failed (rc=-2)
> > [  115.593038] tcrypt: testing cbc(des)
> > [  115.610641] cryptomgr: alg: skcipher: failed to allocate transform for cbc(des): -2
> > [  115.618359] cryptomgr: alg: self-tests for cbc(des) using cbc(des) failed (rc=-2)
> > ...
> > 
> > Signed-off-by: Robert Elliott <elliott@hpe.com>
> > ---
> > crypto/testmgr.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Patch applied.  Thanks.

I thought the conclusion from the discussion was that this should instead be
solved by a tcrypt change?  Either dropping the enumerative testing support from
tcrypt, or making tcrypt just try to allocate the algorithms (relying on the
registration-time self-tests) rather than call alg_test() directly.

- Eric

RE: [PATCH] crypto: testmgr - don't generate WARN for missing modules

[Elliott, Robert (Servers)]

> -----Original Message-----
> From: Eric Biggers <ebiggers@kernel.org>
> Sent: Friday, August 19, 2022 6:15 PM
> To: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Elliott, Robert (Servers) <elliott@hpe.com>;
> tim.c.chen@linux.intel.com; davem@davemloft.net; linux-
> crypto@vger.kernel.org; Kani, Toshi <toshi.kani@hpe.com>; Wright, Randy
> (HPE Servers Linux) <rwright@hpe.com>
> Subject: Re: [PATCH] crypto: testmgr - don't generate WARN for missing
> modules
> 
> On Fri, Aug 19, 2022 at 07:05:41PM +0800, Herbert Xu wrote:
> > Robert Elliott <elliott@hpe.com> wrote:
> > > This userspace command:
> > >    modprobe tcrypt
> > > or
> > >    modprobe tcrypt mode=0
> > >
> > > runs all the tcrypt test cases numbered <200 (i.e., all the
> > > test cases calling tcrypt_test() and returning return values).
> > >
> > > Tests are sparsely numbered from 0 to 1000. For example:
> > >    modprobe tcrypt mode=12
> > > tests sha512, and
> > >    modprobe tcrypt mode=152
> > > tests rfc4543(gcm(aes))) - AES-GCM as GMAC
> > >
> > > The test manager generates WARNING crashdumps every time it
> attempts
> > > a test using an algorithm that is not available (not built-in to
> the
> > > kernel or available as a module):
> > >
> > >    alg: skcipher: failed to allocate transform for ecb(arc4): -2
> > >    ------------[ cut here ]-----------
> > >    alg: self-tests for ecb(arc4) (ecb(arc4)) failed (rc=-2)
> > >    WARNING: CPU: 9 PID: 4618 at crypto/testmgr.c:5777
> > > alg_test+0x30b/0x510
> > >    [50 more lines....]
> > >
> > >    ---[ end trace 0000000000000000 ]---
> > >
> > > If the kernel is compiled with CRYPTO_USER_API_ENABLE_OBSOLETE
> > > disabled (the default), then these algorithms are not compiled into
> > > the kernel or made into modules and trigger WARNINGs:
> > >    arc4 tea xtea khazad anubis xeta seed
> > >
> > > Additionally, any other algorithms that are not enabled in .config
> > > will generate WARNINGs. In RHEL 9.0, for example, the default
> > > selection of algorithms leads to 16 WARNING dumps.
> > >
> > > One attempt to fix this was by modifying tcrypt_test() to check
> > > crypto_has_alg() and immediately return 0 if crypto_has_alg()
> fails,
> > > rather than proceed and return a non-zero error value that causes
> > > the caller (alg_test() in crypto/testmgr.c) to invoke WARN().
> > > That knocks out too many algorithms, though; some combinations
> > > like ctr(des3_ede) would work.
> > >
> > > Instead, change the condition on the WARN to ignore a return
> > > value is ENOENT, which is the value returned when the algorithm
> > > or combination of algorithms doesn't exist. Add a pr_warn to
> > > communicate that information in case the WARN is skipped.
> > >
> > > This approach allows algorithm tests to work that are combinations,
> > > not provided by one driver, like ctr(blowfish).
> > >
> > > Result - no more WARNINGs:
> > > modprobe tcrypt
> > > [  115.541765] tcrypt: testing md5
> > > [  115.556415] tcrypt: testing sha1
> > > [  115.570463] tcrypt: testing ecb(des)
> > > [  115.585303] cryptomgr: alg: skcipher: failed to allocate
> transform for ecb(des): -2
> > > [  115.593037] cryptomgr: alg: self-tests for ecb(des) using
> ecb(des) failed (rc=-2)
> > > [  115.593038] tcrypt: testing cbc(des)
> > > [  115.610641] cryptomgr: alg: skcipher: failed to allocate
> transform for cbc(des): -2
> > > [  115.618359] cryptomgr: alg: self-tests for cbc(des) using
> cbc(des) failed (rc=-2)
> > > ...
> > >
> > > Signed-off-by: Robert Elliott <elliott@hpe.com>
> > > ---
> > > crypto/testmgr.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Patch applied.  Thanks.
> 
> I thought the conclusion from the discussion was that this should
> instead be solved by a tcrypt change?  Either dropping the enumerative
> testing support from tcrypt, or making tcrypt just try to allocate the
> algorithms (relying on the registration-time self-tests) rather than
> call alg_test() directly.
> 
> - Eric

Per Stephan, it sounds like this was a hacky way to get some/most of
the modules loaded.

It'd be good if there was a way to run all registered tests on all
available modules, not just the ones that someone remembered to put
in tcrypt.c.

I do worry this WARN() isn't really helpful even for real self-test
failures - it's dumping the call trace to alg_test(), not the
trace to whatever crypto function alg_test called that is failing. 
With Linus always expressing concern with too many BUG and WARN
calls, it might be better as just pr_warn() or pr_err().

Re: [PATCH] crypto: testmgr - don't generate WARN for missing modules

[Eric Biggers]

On Sat, Aug 20, 2022 at 12:15:41AM +0000, Elliott, Robert (Servers) wrote:
> Per Stephan, it sounds like this was a hacky way to get some/most of
> the modules loaded.
> 
> It'd be good if there was a way to run all registered tests on all
> available modules, not just the ones that someone remembered to put
> in tcrypt.c.

Most algorithms can be allocated via a userspace program using AF_ALG.  The only
exception is algorithm types that AF_ALG doesn't support.

> I do worry this WARN() isn't really helpful even for real self-test
> failures - it's dumping the call trace to alg_test(), not the
> trace to whatever crypto function alg_test called that is failing. 
> With Linus always expressing concern with too many BUG and WARN
> calls, it might be better as just pr_warn() or pr_err().

It's very helpful because WARN is the standard way for the kernel to report that
a kernel bug has been encountered.  A test failure is a kernel bug.

The stack trace printed by WARN indeed isn't useful here, as it will always be
the same.  But it's just a side effect.  The important things here are that a
WARN is triggered at all, and that some log messages that describe what failed
are printed.

- Eric