crypto: testmgr - reinstate kconfig support for fast tests only

From: Eric Biggers <[email protected]>

Commit 698de822780f ("crypto: testmgr - make it easier to enable the full set of tests") removed support for building kernels that run only the "fast" set of crypto self-tests by default. This assumed that nearly everyone actually wanted the full set of tests, *if* they had already chosen to enable the tests at all.

Unfortunately, it turns out that both Debian and Fedora have the crypto self-tests enabled in their production kernels, and they seem to want to

keep them enabled. The full set of tests isn't great for that, since

they take significantly longer to run and slow down the boot.

One issue is that the crypto self-tests are being (mis)used to meet FIPS 140-3 pre-operational testing requirements. But actually the more fundamental issue is that the crypto/ subsystem has many buggy and untested drivers for off-CPU hardware accelerators on rare platforms. As a result, apparently in some cases the tests are actually being relied on *in production kernels* to stop buggy drivers from being used. I think this is kind of crazy (untested drivers should just not be enabled at all), but that seems to be how things work currently.

Thus, reintroduce a kconfig option that controls the level of testing. Instead of the original CRYPTO_MANAGER_EXTRA_TESTS which was confusing and disabled by default, go with CRYPTO_SELFTESTS_FULL which is enabled by default (but dependent on CRYPTO_SELFTESTS, of course).

Given the "production kernel" use cases, also remove the dependency on DEBUG_KERNEL from CRYPTO_SELFTESTS. It was introduced by commit 40b9969796bf ("crypto: testmgr - replace CRYPTO_MANAGER_DISABLE_TESTS with CRYPTO_SELFTESTS") and wasn't present on the original option.

I also haven't reinstated all the #ifdefs in crypto/testmgr.c. Instead, just rely on the compiler to optimize out unused code.

Fixes 40b9969796bf ("crypto: testmgr - replace CRYPTO_MANAGER_DISABLE_TESTS with CRYPTO_SELFTESTS")

Fixes 698de822780f ("crypto: testmgr - make it easier to enable the full set of tests")

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

---

This patch is targeting the crypto tree for 6.16.

 crypto/Kconfig                 | 18 ++++++++++++++----
 crypto/testmgr.c               | 15 ++++++++++++---
 include/crypto/internal/simd.h |  6 ++++--
 lib/crypto/Makefile            |  2 +-
 4 files changed, 31 insertions(+), 10 deletions(-)

crypto/Kconfig +14 -4

index e9fee7818e270..8612ebf655647 100644
@@ -174,20 +174,30 @@ config CRYPTO_USER
 	  Userspace configuration for cryptographic instantiations such as
 	  cbc(aes).
 
 config CRYPTO_SELFTESTS
 	bool "Enable cryptographic self-tests"
-	depends on DEBUG_KERNEL
 	help
 	  Enable the cryptographic self-tests.
 
 	  The cryptographic self-tests run at boot time, or at algorithm
 	  registration time if algorithms are dynamically loaded later.
 
-	  This is primarily intended for developer use.  It should not be
-	  enabled in production kernels, unless you are trying to use these
-	  tests to fulfill a FIPS testing requirement.
+config CRYPTO_SELFTESTS_FULL
+	bool "Enable the full set of cryptographic self-tests"
+	depends on CRYPTO_SELFTESTS
+	default y
+	help
+	  Enable the full set of cryptographic self-tests for each algorithm.
+
+	  For development and pre-release testing, leave this as 'y'.
+
+	  If you're keeping the crypto self-tests enabled in a production
+	  kernel, you likely want to set this to 'n' to speed up the boot.  This
+	  will cause the "slow" tests to be skipped.  This may suffice for a
+	  quick sanity check of drivers and for FIPS 140-3 pre-operational self-
+	  testing, but some issues can be found only by the full set of tests.
 
 config CRYPTO_NULL
 	tristate "Null algorithms"
 	select CRYPTO_ALGAPI
 	select CRYPTO_SKCIPHER

crypto/testmgr.c +12 -3

index 72005074a5c26..32f753d6c4302 100644
@@ -43,17 +43,22 @@ MODULE_IMPORT_NS("CRYPTO_INTERNAL");
 
 static bool notests;
 module_param(notests, bool, 0644);
 MODULE_PARM_DESC(notests, "disable all crypto self-tests");
 
+#ifdef CONFIG_CRYPTO_SELFTESTS_FULL
 static bool noslowtests;
 module_param(noslowtests, bool, 0644);
 MODULE_PARM_DESC(noslowtests, "disable slow crypto self-tests");
 
 static unsigned int fuzz_iterations = 100;
 module_param(fuzz_iterations, uint, 0644);
 MODULE_PARM_DESC(fuzz_iterations, "number of fuzz test iterations");
+#else
+#define noslowtests 1
+#define fuzz_iterations 0
+#endif
 
 #ifndef CONFIG_CRYPTO_SELFTESTS
 
 /* a perfect nop */
 int alg_test(const char *driver, const char *alg, u32 type, u32 mask)

@@ -317,13 +322,13 @@ struct testvec_config {
 
 #define TESTVEC_CONFIG_NAMELEN	192
 
 /*
  * The following are the lists of testvec_configs to test for each algorithm
- * type when the fast crypto self-tests are enabled.  They aim to provide good
- * test coverage, while keeping the test time much shorter than the full tests
- * so that the fast tests can be used to fulfill FIPS 140 testing requirements.
+ * type when the "fast" crypto self-tests are enabled.  They aim to provide good
+ * test coverage, while keeping the test time much shorter than the "full" tests
+ * so that the "fast" tests can be enabled in a wider range of circumstances.
  */
 
 /* Configs for skciphers and aeads */
 static const struct testvec_config default_cipher_testvec_configs[] = {
 	{

@@ -1181,18 +1186,22 @@ static void generate_random_testvec_config(struct rnd_state *rng,
 	WARN_ON_ONCE(!valid_testvec_config(cfg));
 }
 
 static void crypto_disable_simd_for_test(void)
 {
+#ifdef CONFIG_CRYPTO_SELFTESTS_FULL
 	migrate_disable();
 	__this_cpu_write(crypto_simd_disabled_for_test, true);
+#endif
 }
 
 static void crypto_reenable_simd_for_test(void)
 {
+#ifdef CONFIG_CRYPTO_SELFTESTS_FULL
 	__this_cpu_write(crypto_simd_disabled_for_test, false);
 	migrate_enable();
+#endif
 }
 
 /*
  * Given an algorithm name, build the name of the generic implementation of that
  * algorithm, assuming the usual naming convention.  Specifically, this appends

include/crypto/internal/simd.h +4 -2

index 7e7f1ac3b7fda..9e338e7aafbd9 100644
@@ -42,13 +42,15 @@ void simd_unregister_aeads(struct aead_alg *algs, int count,
  * crypto_simd_usable() - is it allowed at this time to use SIMD instructions or
  *			  access the SIMD register file?
  *
  * This delegates to may_use_simd(), except that this also returns false if SIMD
  * in crypto code has been temporarily disabled on this CPU by the crypto
- * self-tests, in order to test the no-SIMD fallback code.
+ * self-tests, in order to test the no-SIMD fallback code.  This override is
+ * currently limited to configurations where the "full" self-tests are enabled,
+ * because it might be a bit too invasive to be part of the "fast" self-tests.
  */
-#ifdef CONFIG_CRYPTO_SELFTESTS
+#ifdef CONFIG_CRYPTO_SELFTESTS_FULL
 DECLARE_PER_CPU(bool, crypto_simd_disabled_for_test);
 #define crypto_simd_usable() \
 	(may_use_simd() && !this_cpu_read(crypto_simd_disabled_for_test))
 #else
 #define crypto_simd_usable() may_use_simd()

lib/crypto/Makefile +1 -1

index 3e79283b617d9..f9e44aac6619b 100644
@@ -60,9 +60,9 @@ libsha256-y					:= sha256.o
 obj-$(CONFIG_CRYPTO_LIB_SHA256_GENERIC)		+= libsha256-generic.o
 libsha256-generic-y				:= sha256-generic.o
 
 obj-$(CONFIG_MPILIB) += mpi/
 
-obj-$(CONFIG_CRYPTO_SELFTESTS)			+= simd.o
+obj-$(CONFIG_CRYPTO_SELFTESTS_FULL)		+= simd.o
 
 obj-$(CONFIG_CRYPTO_LIB_SM3)			+= libsm3.o
 libsm3-y					:= sm3.o

base-commit: aef17cb3d3c43854002956f24c24ec8e1a0e3546

I was about to respond to your reply, but I guess this may be a better fit for it. The TL;DR: version is this:

If you think distros shouldn't enable it, as you initially clearly described and it seems to me you still think so, the right thing for distros to do, is to disable those test. Which in turn means the fast tests should not be reinstated (?).

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Commit 698de822780f ("crypto: testmgr - make it easier to enable the full set of tests") removed support for building kernels that run only the "fast" set of crypto self-tests by default. This assumed that nearly everyone actually wanted the full set of tests, *if* they had already chosen to enable the tests at all.

Unfortunately, it turns out that both Debian and Fedora have the crypto self-tests enabled in their production kernels, and they seem to want to

I explicitly referenced https://bugs.debian.org/599441 as that was the only justification I found for enabling it.
In it, on 2010-10-07 "Mario 'BitKoenig' Holbe" said:

I personally think (re)enabling these tests would be a way safer default for a distribution kernel which runs on lots of different hardware setups

Before I looked up that bug, I had not heard of that person, so I don't know if they're a crypto expert or just a random person on the internet. It also doesn't say *why* they thought it would be a good idea to enable those tests.
I have no idea what Fedora's reasoning was for enabling it. Maybe their reasons were sound; I think Debian's are rather thin (that I could find). And from ~ 15 years ago.

Quoted 1 line

keep them enabled. The full set of tests isn't great for that, since

I think the 'new' description is(/was) great. A subject matter expert says/said "don't enable this on production kernels". I wish all Kconfig help texts were this clear :-)
So based on the previous description, it seems wise that Debian (and Fedora) would update their kernel config and disable those test.

In *my* update to 6.16-rc1, I only 'converted' to new names. A change to my kernel config (ie disable the tests) would be in a separate commit (with an appropriate commit msg).
I hadn't done that yet as I was curious what the results would be.

So "they seem to want to keep them enabled" seems a premature conclusion; at least wrt Debian and AFAICT.
It's also possible that if/when people see the kernel warning, they'd file a new Debian bug to have it disabled.

(I've made some contributions in the past, but) I am not part of Debian's kernel team, so I don't know what they will decide.

I'll gladly leave it up to you if you still think reinstating the fast tests is worth it, but I felt a bit more context was warranted.

Cheers,
Diederik

Quoted 7 lines · Diederik de Haas

I was about to respond to your reply, but I guess this may be a better fit for it. The TL;DR: version is this:

If you think distros shouldn't enable it, as you initially clearly described and it seems to me you still think so, the right thing for distros to do, is to disable those test. Which in turn means the fast tests should not be reinstated (?).

I mean, not enabling the tests in production is how it should be.

But Fedora already enabled CRYPTO_SELFTESTS, apparently because of FIPS
(https://gitlab.com/cki-project/kernel-ark/-/merge_requests/3886).

You're right there doesn't seem to be an up-to-date bug for Debian
(https://bugs.debian.org/599441 is old), so maybe my conclusion is premature.

However, besides FIPS I think the problem is that the crypto/ philosophy is to

throw untested and broken hardware drivers over the wall at users. As long as

that's the case, the self-tests do actually have some value in protecting users from those drivers, even though that's not how it should be.

Quoted 9 lines · Eric Biggers

Quoted 7 lines · Diederik de Haas

I was about to respond to your reply, but I guess this may be a better fit for it. The TL;DR: version is this:

If you think distros shouldn't enable it, as you initially clearly described and it seems to me you still think so, the right thing for distros to do, is to disable those test. Which in turn means the fast tests should not be reinstated (?).

Quoted 4 lines

I mean, not enabling the tests in production is how it should be.

But Fedora already enabled CRYPTO_SELFTESTS, apparently because of FIPS
(https://gitlab.com/cki-project/kernel-ark/-/merge_requests/3886).

That is recent and there's at least 1 person I recognize as having proper expertise in this matter ;-)

Quoted 1 line

throw untested and broken hardware drivers over the wall at users. As long as

Only speaking for myself, my *assumption* is that crypto functionality in hardware is/should be faster and would lessen the load on the CPU (which with several SBCs seems really worthwhile).

crypto/Kconfig +4 -0

+config CRYPTO_SELFTESTS_FULL
+	bool "Enable the full set of cryptographic self-tests"
+	depends on CRYPTO_SELFTESTS
+	default y

Quoted 9 lines · Eric Biggers

crypto/Kconfig +0 -0

index e9fee7818e270..8612ebf655647 100644
@@ -174,20 +174,30 @@ config CRYPTO_USER
 	  Userspace configuration for cryptographic instantiations such as
 	  cbc(aes).
 

crypto/Kconfig config CRYPTO_USER · line 179

Quoted 3 lines

crypto/Kconfig +0 -1

 config CRYPTO_SELFTESTS
 	bool "Enable cryptographic self-tests"
-	depends on DEBUG_KERNEL

Please restore the dependency on EXPERT. I do not want random users exposed to this toggle.

Quoted 8 lines

crypto/Kconfig +8 -0

+config CRYPTO_SELFTESTS_FULL
+	bool "Enable the full set of cryptographic self-tests"
+	depends on CRYPTO_SELFTESTS
+	default y
+	help
+	  Enable the full set of cryptographic self-tests for each algorithm.
+
+	  For development and pre-release testing, leave this as 'y'.

crypto/Kconfig config CRYPTO_USER · line 198

Quoted 6 lines

crypto/Kconfig +6 -0

+
+	  If you're keeping the crypto self-tests enabled in a production
+	  kernel, you likely want to set this to 'n' to speed up the boot.  This
+	  will cause the "slow" tests to be skipped.  This may suffice for a
+	  quick sanity check of drivers and for FIPS 140-3 pre-operational self-
+	  testing, but some issues can be found only by the full set of tests.

Please remove the "default y".

It used to be:

config CRYPTO_MANAGER_DISABLE_TESTS
bool "Disable run-time self tests"
default y
help
Disable run-time self tests that normally take place at algorithm registration.

So the CONFIG_EXPERT dependency for the prompt would be new. Are you sure?

If you insist. I hoped to get the people working on drivers to actually run the tests that they are supposed to. The default y is appropriate for anyone actually doing development and/or testing, which is what the tests are supposed to be for.

But I guess that doesn't really happen, and distros are expected to run the reduced set of tests in production because upstream doesn't test the drivers. And they will want n here.

Quoted 11 lines · Eric Biggers

It used to be:

config CRYPTO_MANAGER_DISABLE_TESTS
bool "Disable run-time self tests"
default y
help
Disable run-time self tests that normally take place at algorithm registration.

So the CONFIG_EXPERT dependency for the prompt would be new. Are you sure?

When this was inverted I specifically asked for a dependency on EXPERT so that normal users won't be bothered by a question that had no relevance to them.

You then suggested a dependency on DEBUG_KERNEL which I accepted because EXPERT happens to select that so they're practically equivalent.

So make it depend on either DEBUG_KERNEL or EXPERT because normal users should never see this question. IOW we as developers should select a sane default, whatever that may be.

Quoted 8 lines

If you insist. I hoped to get the people working on drivers to actually run the tests that they are supposed to. The default y is appropriate for anyone actually doing development and/or testing, which is what the tests are supposed to be for.

But I guess that doesn't really happen, and distros are expected to run the reduced set of tests in production because upstream doesn't test the drivers. And they will want n here.

I share your concern. One idea is to calculate a hash based on the current time and print it out if and only if SELFTESTS_FULL is enabled.

Then we could require all driver submissions to include this message as proof that they enabled this option.