[PATCH 0/3] crypto: jitterentropy - bound collection loop

[PATCH 0/3] crypto: jitterentropy - bound collection loop

[Nicolai Stange]

Hi,

the sampling loop in jent_gen_entropy() can potentially run indefinitely
w/o making any forward progress, namely if only stuck samples are taken
for whatever reason.

There's a straight-forward way to make the entropy collection more robust,
namely to terminate the loop and report an error if this happens. This
patchset here implements that.

Applies to herbert/cryptodev-2.6.git master.

Thanks!

Nicolai

Nicolai Stange (3):
  crypto: drbg - ignore jitterentropy errors if not in FIPS mode
  crypto: jitter - don't limit ->health_failure check to FIPS mode
  crypto: jitter - quit sample collection loop upon RCT failure

 crypto/drbg.c                | 7 +++++--
 crypto/jitterentropy-kcapi.c | 6 ------
 crypto/jitterentropy.c       | 6 +-----
 crypto/jitterentropy.h       | 1 -
 4 files changed, 6 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH 1/3] crypto: drbg - ignore jitterentropy errors if not in FIPS mode

[Nicolai Stange]

A subsequent patch will make the jitterentropy RNG to unconditionally
report health test errors back to callers, independent of whether
fips_enabled is set or not. The DRBG needs access to a functional
jitterentropy instance only in FIPS mode (because it's the only SP800-90B
compliant entropy source as it currently stands). Thus, it is perfectly
fine for the DRBGs to obtain entropy from the jitterentropy source only
on a best effort basis if fips_enabled is off.

Make the DRBGs to ignore jitterentropy failures if fips_enabled is not set.

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

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 5977a72afb03..177983b6ae38 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1193,11 +1193,14 @@ static int drbg_seed(struct drbg_state *drbg, struct drbg_string *pers,
 			pr_devel("DRBG: (re)seeding with %u bytes of entropy\n",
 				 entropylen);
 		} else {
-			/* Get seed from Jitter RNG */
+			/*
+			 * Get seed from Jitter RNG, failures are
+			 * fatal only in FIPS mode.
+			 */
 			ret = crypto_rng_get_bytes(drbg->jent,
 						   entropy + entropylen,
 						   entropylen);
-			if (ret) {
+			if (fips_enabled && ret) {
 				pr_devel("DRBG: jent failed with %d\n", ret);
 
 				/*
-- 
2.26.2

[PATCH 2/3] crypto: jitter - don't limit ->health_failure check to FIPS mode

[Nicolai Stange]

The jitterentropy's Repetition Count Test (RCT) as well as the Adaptive
Proportion Test (APT) are run unconditionally on any collected samples.
However, their result, i.e. ->health_failure, will only get checked if
fips_enabled is set, c.f. the jent_health_failure() wrapper.

I would argue that a RCT or APT failure indicates that something's
seriously off and that this should always be reported as an error,
independently of whether FIPS mode is enabled or not: it should be up to
callers whether or not and how to handle jitterentropy failures.

Make jent_health_failure() to unconditionally return ->health_failure,
independent of whether fips_enabled is set.

Note that fips_enabled isn't accessed from the jitterentropy code anymore
now. Remove the linux/fips.h include as well as the jent_fips_enabled()
wrapper.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/jitterentropy-kcapi.c | 6 ------
 crypto/jitterentropy.c       | 4 ----
 crypto/jitterentropy.h       | 1 -
 3 files changed, 11 deletions(-)

diff --git a/crypto/jitterentropy-kcapi.c b/crypto/jitterentropy-kcapi.c
index e8a4165a1874..2d115bec15ae 100644
--- a/crypto/jitterentropy-kcapi.c
+++ b/crypto/jitterentropy-kcapi.c
@@ -40,7 +40,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/fips.h>
 #include <linux/time.h>
 #include <crypto/internal/rng.h>
 
@@ -60,11 +59,6 @@ void jent_zfree(void *ptr)
 	kfree_sensitive(ptr);
 }
 
-int jent_fips_enabled(void)
-{
-	return fips_enabled;
-}
-
 void jent_panic(char *s)
 {
 	panic("%s", s);
diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
index 788d90749715..24e087c3f526 100644
--- a/crypto/jitterentropy.c
+++ b/crypto/jitterentropy.c
@@ -298,10 +298,6 @@ static int jent_stuck(struct rand_data *ec, __u64 current_delta)
  */
 static int jent_health_failure(struct rand_data *ec)
 {
-	/* Test is only enabled in FIPS mode */
-	if (!jent_fips_enabled())
-		return 0;
-
 	return ec->health_failure;
 }
 
diff --git a/crypto/jitterentropy.h b/crypto/jitterentropy.h
index c83fff32d130..b7397b617ef0 100644
--- a/crypto/jitterentropy.h
+++ b/crypto/jitterentropy.h
@@ -2,7 +2,6 @@
 
 extern void *jent_zalloc(unsigned int len);
 extern void jent_zfree(void *ptr);
-extern int jent_fips_enabled(void);
 extern void jent_panic(char *s);
 extern void jent_memcpy(void *dest, const void *src, unsigned int n);
 extern void jent_get_nstime(__u64 *out);
-- 
2.26.2

[PATCH 3/3] crypto: jitter - quit sample collection loop upon RCT failure

[Nicolai Stange]

The jitterentropy collection loop in jent_gen_entropy() can in principle
run indefinitely without making any progress if it only receives stuck
measurements as determined by jent_stuck(). After 31 consecutive stuck
samples, the Repetition Count Test (RCT) would fail anyway and the
jitterentropy RNG instances moved into ->health_failure == 1 state.
jent_gen_entropy()'s caller, jent_read_entropy() would then check for
this ->health_failure condition and return an error if found set. It
follows that there's absolutely no point in continuing the collection loop
in jent_gen_entropy() once the RCT has failed.

Make the jitterentropy collection loop more robust by terminating it upon
jent_health_failure() so that it won't continue to run indefinitely without
making any progress.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/jitterentropy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
index 24e087c3f526..8f5283f28ed3 100644
--- a/crypto/jitterentropy.c
+++ b/crypto/jitterentropy.c
@@ -547,7 +547,7 @@ static void jent_gen_entropy(struct rand_data *ec)
 	/* priming of the ->prev_time value */
 	jent_measure_jitter(ec);
 
-	while (1) {
+	while (!jent_health_failure(ec)) {
 		/* If a stuck measurement is received, repeat measurement */
 		if (jent_measure_jitter(ec))
 			continue;
-- 
2.26.2

Re: [PATCH 1/3] crypto: drbg - ignore jitterentropy errors if not in FIPS mode

[Stephan Mueller]

Am Dienstag, 30. November 2021, 15:10:07 CET schrieb Nicolai Stange:

Hi Nicolai,

> A subsequent patch will make the jitterentropy RNG to unconditionally
> report health test errors back to callers, independent of whether
> fips_enabled is set or not. The DRBG needs access to a functional
> jitterentropy instance only in FIPS mode (because it's the only SP800-90B
> compliant entropy source as it currently stands). Thus, it is perfectly
> fine for the DRBGs to obtain entropy from the jitterentropy source only
> on a best effort basis if fips_enabled is off.
> 
> Make the DRBGs to ignore jitterentropy failures if fips_enabled is not set.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Mueller <smueller@chronox.de>

Thanks
Stephan
> ---
>  crypto/drbg.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index 5977a72afb03..177983b6ae38 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1193,11 +1193,14 @@ static int drbg_seed(struct drbg_state *drbg, struct
> drbg_string *pers, pr_devel("DRBG: (re)seeding with %u bytes of entropy\n",
>  				 entropylen);
>  		} else {
> -			/* Get seed from Jitter RNG */
> +			/*
> +			 * Get seed from Jitter RNG, failures are
> +			 * fatal only in FIPS mode.
> +			 */
>  			ret = crypto_rng_get_bytes(drbg->jent,
>  						   entropy + entropylen,
>  						   entropylen);
> -			if (ret) {
> +			if (fips_enabled && ret) {
>  				pr_devel("DRBG: jent failed with %d\n", ret);
> 
>  				/*


Ciao
Stephan

Re: [PATCH 2/3] crypto: jitter - don't limit ->health_failure check to FIPS mode

[Stephan Mueller]

Am Dienstag, 30. November 2021, 15:10:08 CET schrieb Nicolai Stange:

Hi Nicolai,

> The jitterentropy's Repetition Count Test (RCT) as well as the Adaptive
> Proportion Test (APT) are run unconditionally on any collected samples.
> However, their result, i.e. ->health_failure, will only get checked if
> fips_enabled is set, c.f. the jent_health_failure() wrapper.
> 
> I would argue that a RCT or APT failure indicates that something's
> seriously off and that this should always be reported as an error,
> independently of whether FIPS mode is enabled or not: it should be up to
> callers whether or not and how to handle jitterentropy failures.
> 
> Make jent_health_failure() to unconditionally return ->health_failure,
> independent of whether fips_enabled is set.
> 
> Note that fips_enabled isn't accessed from the jitterentropy code anymore
> now. Remove the linux/fips.h include as well as the jent_fips_enabled()
> wrapper.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Mueller <smueller@chronox.de>

Thanks
Stephan

> ---
>  crypto/jitterentropy-kcapi.c | 6 ------
>  crypto/jitterentropy.c       | 4 ----
>  crypto/jitterentropy.h       | 1 -
>  3 files changed, 11 deletions(-)
> 
> diff --git a/crypto/jitterentropy-kcapi.c b/crypto/jitterentropy-kcapi.c
> index e8a4165a1874..2d115bec15ae 100644
> --- a/crypto/jitterentropy-kcapi.c
> +++ b/crypto/jitterentropy-kcapi.c
> @@ -40,7 +40,6 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> -#include <linux/fips.h>
>  #include <linux/time.h>
>  #include <crypto/internal/rng.h>
> 
> @@ -60,11 +59,6 @@ void jent_zfree(void *ptr)
>  	kfree_sensitive(ptr);
>  }
> 
> -int jent_fips_enabled(void)
> -{
> -	return fips_enabled;
> -}
> -
>  void jent_panic(char *s)
>  {
>  	panic("%s", s);
> diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
> index 788d90749715..24e087c3f526 100644
> --- a/crypto/jitterentropy.c
> +++ b/crypto/jitterentropy.c
> @@ -298,10 +298,6 @@ static int jent_stuck(struct rand_data *ec, __u64
> current_delta) */
>  static int jent_health_failure(struct rand_data *ec)
>  {
> -	/* Test is only enabled in FIPS mode */
> -	if (!jent_fips_enabled())
> -		return 0;
> -
>  	return ec->health_failure;
>  }
> 
> diff --git a/crypto/jitterentropy.h b/crypto/jitterentropy.h
> index c83fff32d130..b7397b617ef0 100644
> --- a/crypto/jitterentropy.h
> +++ b/crypto/jitterentropy.h
> @@ -2,7 +2,6 @@
> 
>  extern void *jent_zalloc(unsigned int len);
>  extern void jent_zfree(void *ptr);
> -extern int jent_fips_enabled(void);
>  extern void jent_panic(char *s);
>  extern void jent_memcpy(void *dest, const void *src, unsigned int n);
>  extern void jent_get_nstime(__u64 *out);


Ciao
Stephan

Re: [PATCH 3/3] crypto: jitter - quit sample collection loop upon RCT failure

[Stephan Mueller]

Am Dienstag, 30. November 2021, 15:10:09 CET schrieb Nicolai Stange:

Hi Nicolai,

> The jitterentropy collection loop in jent_gen_entropy() can in principle
> run indefinitely without making any progress if it only receives stuck
> measurements as determined by jent_stuck(). After 31 consecutive stuck
> samples, the Repetition Count Test (RCT) would fail anyway and the
> jitterentropy RNG instances moved into ->health_failure == 1 state.
> jent_gen_entropy()'s caller, jent_read_entropy() would then check for
> this ->health_failure condition and return an error if found set. It
> follows that there's absolutely no point in continuing the collection loop
> in jent_gen_entropy() once the RCT has failed.
> 
> Make the jitterentropy collection loop more robust by terminating it upon
> jent_health_failure() so that it won't continue to run indefinitely without
> making any progress.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Reviewed-by: Stephan Mueller <smueller@chronox.de>

Thanks
Stephan

> ---
>  crypto/jitterentropy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/crypto/jitterentropy.c b/crypto/jitterentropy.c
> index 24e087c3f526..8f5283f28ed3 100644
> --- a/crypto/jitterentropy.c
> +++ b/crypto/jitterentropy.c
> @@ -547,7 +547,7 @@ static void jent_gen_entropy(struct rand_data *ec)
>  	/* priming of the ->prev_time value */
>  	jent_measure_jitter(ec);
> 
> -	while (1) {
> +	while (!jent_health_failure(ec)) {
>  		/* If a stuck measurement is received, repeat measurement */
>  		if (jent_measure_jitter(ec))
>  			continue;


Ciao
Stephan

Re: [PATCH 0/3] crypto: jitterentropy - bound collection loop

[Herbert Xu]

On Tue, Nov 30, 2021 at 03:10:06PM +0100, Nicolai Stange wrote:
> Hi,
> 
> the sampling loop in jent_gen_entropy() can potentially run indefinitely
> w/o making any forward progress, namely if only stuck samples are taken
> for whatever reason.
> 
> There's a straight-forward way to make the entropy collection more robust,
> namely to terminate the loop and report an error if this happens. This
> patchset here implements that.
> 
> Applies to herbert/cryptodev-2.6.git master.
> 
> Thanks!
> 
> Nicolai
> 
> Nicolai Stange (3):
>   crypto: drbg - ignore jitterentropy errors if not in FIPS mode
>   crypto: jitter - don't limit ->health_failure check to FIPS mode
>   crypto: jitter - quit sample collection loop upon RCT failure
> 
>  crypto/drbg.c                | 7 +++++--
>  crypto/jitterentropy-kcapi.c | 6 ------
>  crypto/jitterentropy.c       | 6 +-----
>  crypto/jitterentropy.h       | 1 -
>  4 files changed, 6 insertions(+), 14 deletions(-)
> 
> -- 
> 2.26.2

All 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