| Message ID | 20220813231443.2706-1-elliott@hpe.com |
|---|---|
| State | Accepted |
| Commit | a76bd86a85cac9feddc66d38019f943d054f0218 |
| Headers | show |
| Series | crypto: testmgr - don't generate WARN for missing modules | expand |
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
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,
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
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,
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.
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
> -----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().
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
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",
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(-)