[PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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 <ebiggers@google.com>
---

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

diff --git a/crypto/Kconfig b/crypto/Kconfig
index e9fee7818e270..8612ebf655647 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -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
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 72005074a5c26..32f753d6c4302 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -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
diff --git a/include/crypto/internal/simd.h b/include/crypto/internal/simd.h
index 7e7f1ac3b7fda..9e338e7aafbd9 100644
--- a/include/crypto/internal/simd.h
+++ b/include/crypto/internal/simd.h
@@ -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()
diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
index 3e79283b617d9..f9e44aac6619b 100644
--- a/lib/crypto/Makefile
+++ b/lib/crypto/Makefile
@@ -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
-- 
2.49.0

Re: [PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 10657 bytes --]

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

On Wed Jun 11, 2025 at 7:55 PM CEST, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> 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.

> 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

> 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 <ebiggers@google.com>
> ---
>
> 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(-)
>
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index e9fee7818e270..8612ebf655647 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -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
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 72005074a5c26..32f753d6c4302 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -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
> diff --git a/include/crypto/internal/simd.h b/include/crypto/internal/simd.h
> index 7e7f1ac3b7fda..9e338e7aafbd9 100644
> --- a/include/crypto/internal/simd.h
> +++ b/include/crypto/internal/simd.h
> @@ -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()
> diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
> index 3e79283b617d9..f9e44aac6619b 100644
> --- a/lib/crypto/Makefile
> +++ b/lib/crypto/Makefile
> @@ -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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Eric Biggers]

On Wed, Jun 11, 2025 at 08:53:17PM +0200, Diederik de Haas wrote:
> 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 (?).
> 
> On Wed Jun 11, 2025 at 7:55 PM CEST, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > 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.
> 
> > 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

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.

- Eric

Re: [PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

On Wed Jun 11, 2025 at 9:04 PM CEST, Eric Biggers wrote:
> On Wed, Jun 11, 2025 at 08:53:17PM +0200, Diederik de Haas wrote:
>> 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).

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

> 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

Another problem (IMO) is that a lot (?) of people (like myself) don't
(really) understand crypto and therefor rely on the description in the
Kconfig help text and make a choice based on that.
That's (one of) the reason(s) I was so happy with the clear text :-)

> 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).
But I don't have the knowledge to determine whether it's broken or not.
Unless there's a(n easy) tool for that (like 'rngtest' [1]).

[1] https://lore.kernel.org/linux-rockchip/6425788.NZdkxuyfQg@bagend/
resulting in f.e.
5afdb98dcc55 ("arm64: dts: rockchip: Describe why is HWRNG disabled in RK356x base dtsi")

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

Thanks for the additional info :-)

Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Eric Biggers]

On Wed, Jun 11, 2025 at 09:47:27PM +0200, Diederik de Haas wrote:
> On Wed Jun 11, 2025 at 9:04 PM CEST, Eric Biggers wrote:
> > On Wed, Jun 11, 2025 at 08:53:17PM +0200, Diederik de Haas wrote:
> >> 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).
> 
> That is recent and there's at least 1 person I recognize as having
> proper expertise in this matter ;-)

FWIW, here's an example from just today where the crypto self-tests prevented a
buggy driver from being used in Debian:
https://lore.kernel.org/linux-crypto/20250611101750.6839-1-AlanSong-oc@zhaoxin.com/

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

Often the hardware offloads are actually slower.  They require sending the CPU
an interrupt once the operation is done, which has a lot of overhead.  They also
tend to be optimized for throughput rather than latency, and only provide good
throughput when given a large number of concurrent requests.

Inline encryption does actually work, but that is a separate type of accelerator
and not what we're talking about here.

- Eric

Re: [PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Herbert Xu]

On Wed, Jun 11, 2025 at 10:55:25AM -0700, Eric Biggers wrote:
>
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index e9fee7818e270..8612ebf655647 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -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

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

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

Please remove the "default y".

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 - reinstate kconfig support for fast tests only

[Eric Biggers]

On Thu, Jun 12, 2025 at 01:55:21PM +0800, Herbert Xu wrote:
> On Wed, Jun 11, 2025 at 10:55:25AM -0700, Eric Biggers wrote:
> >
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index e9fee7818e270..8612ebf655647 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -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
> 
> Please restore the dependency on EXPERT.  I do not want random
> users exposed to this toggle.

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?

> > +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.
> 
> Please remove the "default y".

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.

- Eric

Re: [PATCH] crypto: testmgr - reinstate kconfig support for fast tests only

[Herbert Xu]

On Wed, Jun 11, 2025 at 11:09:31PM -0700, Eric Biggers wrote:
>
> 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.

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

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 - reinstate kconfig support for fast tests only

[Eric Biggers]

On Thu, Jun 12, 2025 at 05:03:35PM +0800, Herbert Xu wrote:
> On Wed, Jun 11, 2025 at 11:09:31PM -0700, Eric Biggers wrote:
> >
> > 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.
> 
> > 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.
> 

Crypto drivers need to be regularly tested and maintained, not just tested at
submission time.  Crypto drivers that don't achieve that should not be part of
the kernel.

- Eric