[PATCH] random / hw_random: core: start hwrng kthread also for untrusted sources

[PATCH] random / hw_random: core: start hwrng kthread also for untrusted sources

[Dominik Brodowski]

Start the hwrng kthread even if the hwrng source has a quality setting
of zero. Then, every CRNG_RESEED_INTERVAL, one batch of data from this
zero-quality hwrng source will be mixed into the CRNG pool.

However, to avoid that an untrusted device assists in initializing the
CRNG, go to sleep in add_hwgenerator_randomness() in case the entropy
parameter passed to that function is zero.

This patch is based on the assumption that data from a hwrng source
will not actively harm the CRNG state, but that many hwrng sources
(such as TPM devices), even though they are assigend a quality level of
zero, actually provide some entropy, which is good to mix into the CRNG
pool every once in a while.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/hw_random/core.c | 36 ++++++++++-------------------------
 drivers/char/random.c         |  4 ++--
 2 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 16f227b995e8..edb86c0cccda 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -52,7 +52,7 @@ MODULE_PARM_DESC(default_quality,
 
 static void drop_current_rng(void);
 static int hwrng_init(struct hwrng *rng);
-static void hwrng_manage_rngd(struct hwrng *rng);
+static int hwrng_fillfn(void *unused);
 
 static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
 			       int wait);
@@ -96,6 +96,15 @@ static int set_current_rng(struct hwrng *rng)
 	drop_current_rng();
 	current_rng = rng;
 
+	/* if necessary, start hwrng thread */
+	if (!hwrng_fill) {
+		hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
+		if (IS_ERR(hwrng_fill)) {
+			pr_err("hwrng_fill thread creation failed\n");
+			hwrng_fill = NULL;
+		}
+	}
+
 	return 0;
 }
 
@@ -167,8 +176,6 @@ static int hwrng_init(struct hwrng *rng)
 		rng->quality = 1024;
 	current_quality = rng->quality; /* obsolete */
 
-	hwrng_manage_rngd(rng);
-
 	return 0;
 }
 
@@ -454,10 +461,6 @@ static ssize_t rng_quality_store(struct device *dev,
 	/* the best available RNG may have changed */
 	ret = enable_best_rng();
 
-	/* start/stop rngd if necessary */
-	if (current_rng)
-		hwrng_manage_rngd(current_rng);
-
 out:
 	mutex_unlock(&rng_mutex);
 	return ret ? ret : len;
@@ -509,9 +512,6 @@ static int hwrng_fillfn(void *unused)
 		mutex_unlock(&reading_mutex);
 		put_rng(rng);
 
-		if (!quality)
-			break;
-
 		if (rc <= 0) {
 			pr_warn("hwrng: no data available\n");
 			msleep_interruptible(10000);
@@ -533,22 +533,6 @@ static int hwrng_fillfn(void *unused)
 	return 0;
 }
 
-static void hwrng_manage_rngd(struct hwrng *rng)
-{
-	if (WARN_ON(!mutex_is_locked(&rng_mutex)))
-		return;
-
-	if (rng->quality == 0 && hwrng_fill)
-		kthread_stop(hwrng_fill);
-	if (rng->quality > 0 && !hwrng_fill) {
-		hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
-		if (IS_ERR(hwrng_fill)) {
-			pr_err("hwrng_fill thread creation failed\n");
-			hwrng_fill = NULL;
-		}
-	}
-}
-
 int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 79d7d4e4e582..b360ed4ece03 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -867,9 +867,9 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
 
 	/*
 	 * Throttle writing to once every CRNG_RESEED_INTERVAL, unless
-	 * we're not yet initialized.
+	 * we're not yet initialized or this source isn't trusted.
 	 */
-	if (!kthread_should_stop() && crng_ready())
+	if (!kthread_should_stop() && (crng_ready() || !entropy))
 		schedule_timeout_interruptible(CRNG_RESEED_INTERVAL);
 }
 EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
-- 
2.37.3

Re: [PATCH] random / hw_random: core: start hwrng kthread also for untrusted sources

[Herbert Xu]

Dominik Brodowski <linux@dominikbrodowski.net> wrote:
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 79d7d4e4e582..b360ed4ece03 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -867,9 +867,9 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
> 
>        /*
>         * Throttle writing to once every CRNG_RESEED_INTERVAL, unless
> -        * we're not yet initialized.
> +        * we're not yet initialized or this source isn't trusted.
>         */
> -       if (!kthread_should_stop() && crng_ready())
> +       if (!kthread_should_stop() && (crng_ready() || !entropy))
>                schedule_timeout_interruptible(CRNG_RESEED_INTERVAL);
> }
> EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);

Couldn't you split this bit out?

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] random / hw_random: core: start hwrng kthread also for untrusted sources

[Dominik Brodowski]

Am Wed, Sep 07, 2022 at 02:34:01PM +0800 schrieb Herbert Xu:
> Dominik Brodowski <linux@dominikbrodowski.net> wrote:
> >
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index 79d7d4e4e582..b360ed4ece03 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -867,9 +867,9 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
> > 
> >        /*
> >         * Throttle writing to once every CRNG_RESEED_INTERVAL, unless
> > -        * we're not yet initialized.
> > +        * we're not yet initialized or this source isn't trusted.
> >         */
> > -       if (!kthread_should_stop() && crng_ready())
> > +       if (!kthread_should_stop() && (crng_ready() || !entropy))
> >                schedule_timeout_interruptible(CRNG_RESEED_INTERVAL);
> > }
> > EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
> 
> Couldn't you split this bit out?

I could, but this would need to get merged before the patch to the hwrng
core gets applied. What do you (and Jason) prefer?

Thanks,
	Dominik

Re: [PATCH] random / hw_random: core: start hwrng kthread also for untrusted sources

[Jason A. Donenfeld]

On Wed, Sep 07, 2022 at 08:54:10AM +0200, Dominik Brodowski wrote:
> Am Wed, Sep 07, 2022 at 02:34:01PM +0800 schrieb Herbert Xu:
> > Dominik Brodowski <linux@dominikbrodowski.net> wrote:
> > >
> > > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > > index 79d7d4e4e582..b360ed4ece03 100644
> > > --- a/drivers/char/random.c
> > > +++ b/drivers/char/random.c
> > > @@ -867,9 +867,9 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
> > > 
> > >        /*
> > >         * Throttle writing to once every CRNG_RESEED_INTERVAL, unless
> > > -        * we're not yet initialized.
> > > +        * we're not yet initialized or this source isn't trusted.
> > >         */
> > > -       if (!kthread_should_stop() && crng_ready())
> > > +       if (!kthread_should_stop() && (crng_ready() || !entropy))
> > >                schedule_timeout_interruptible(CRNG_RESEED_INTERVAL);
> > > }
> > > EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
> > 
> > Couldn't you split this bit out?
> 
> I could, but this would need to get merged before the patch to the hwrng
> core gets applied. What do you (and Jason) prefer?

Just split this out and send it to me, and I'll push it early in 6.1 so
that it makes rc1, and then Herbert can apply the hwrng part separately
whenever he sees fit.

Jason

Re: [PATCH] random / hw_random: core: start hwrng kthread also for untrusted sources

[Jason A. Donenfeld]

On Wed, Sep 7, 2022 at 8:34 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> Dominik Brodowski <linux@dominikbrodowski.net> wrote:
> >
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index 79d7d4e4e582..b360ed4ece03 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -867,9 +867,9 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
> >
> >        /*
> >         * Throttle writing to once every CRNG_RESEED_INTERVAL, unless
> > -        * we're not yet initialized.
> > +        * we're not yet initialized or this source isn't trusted.
> >         */
> > -       if (!kthread_should_stop() && crng_ready())
> > +       if (!kthread_should_stop() && (crng_ready() || !entropy))
> >                schedule_timeout_interruptible(CRNG_RESEED_INTERVAL);
> > }
> > EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>
> Couldn't you split this bit out?

It's been two weeks and Dominik hasn't posted anything new, so I'm
going to do that for him. Patch incoming (retaining his authorship).

Jason

[PATCH v2] hw_random: core: start hwrng kthread also for untrusted sources

[Jason A. Donenfeld]

From: Dominik Brodowski <linux@dominikbrodowski.net>

Start the hwrng kthread even if the hwrng source has a quality setting
of zero. Then, every CRNG_RESEED_INTERVAL, one batch of data from this
zero-quality hwrng source will be mixed into the CRNG pool.

However, to avoid that an untrusted device assists in initializing the
CRNG, go to sleep in add_hwgenerator_randomness() in case the entropy
parameter passed to that function is zero.

This patch is based on the assumption that data from a hwrng source
will not actively harm the CRNG state, but that many hwrng sources
(such as TPM devices), even though they are assigend a quality level of
zero, actually provide some entropy, which is good to mix into the CRNG
pool every once in a while.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
This is Dominik's v1, verbatim, with the random.c changes split out, per
Herbert's request.

(It'd be nice if this would land soon, as there are other nice things
that could be layered on top of this change later.)

 drivers/char/hw_random/core.c | 36 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 16f227b995e8..edb86c0cccda 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -52,7 +52,7 @@ MODULE_PARM_DESC(default_quality,
 
 static void drop_current_rng(void);
 static int hwrng_init(struct hwrng *rng);
-static void hwrng_manage_rngd(struct hwrng *rng);
+static int hwrng_fillfn(void *unused);
 
 static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
 			       int wait);
@@ -96,6 +96,15 @@ static int set_current_rng(struct hwrng *rng)
 	drop_current_rng();
 	current_rng = rng;
 
+	/* if necessary, start hwrng thread */
+	if (!hwrng_fill) {
+		hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
+		if (IS_ERR(hwrng_fill)) {
+			pr_err("hwrng_fill thread creation failed\n");
+			hwrng_fill = NULL;
+		}
+	}
+
 	return 0;
 }
 
@@ -167,8 +176,6 @@ static int hwrng_init(struct hwrng *rng)
 		rng->quality = 1024;
 	current_quality = rng->quality; /* obsolete */
 
-	hwrng_manage_rngd(rng);
-
 	return 0;
 }
 
@@ -454,10 +461,6 @@ static ssize_t rng_quality_store(struct device *dev,
 	/* the best available RNG may have changed */
 	ret = enable_best_rng();
 
-	/* start/stop rngd if necessary */
-	if (current_rng)
-		hwrng_manage_rngd(current_rng);
-
 out:
 	mutex_unlock(&rng_mutex);
 	return ret ? ret : len;
@@ -509,9 +512,6 @@ static int hwrng_fillfn(void *unused)
 		mutex_unlock(&reading_mutex);
 		put_rng(rng);
 
-		if (!quality)
-			break;
-
 		if (rc <= 0) {
 			pr_warn("hwrng: no data available\n");
 			msleep_interruptible(10000);
@@ -533,22 +533,6 @@ static int hwrng_fillfn(void *unused)
 	return 0;
 }
 
-static void hwrng_manage_rngd(struct hwrng *rng)
-{
-	if (WARN_ON(!mutex_is_locked(&rng_mutex)))
-		return;
-
-	if (rng->quality == 0 && hwrng_fill)
-		kthread_stop(hwrng_fill);
-	if (rng->quality > 0 && !hwrng_fill) {
-		hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
-		if (IS_ERR(hwrng_fill)) {
-			pr_err("hwrng_fill thread creation failed\n");
-			hwrng_fill = NULL;
-		}
-	}
-}
-
 int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
-- 
2.37.3

[PATCH v3] hw_random: core: start hwrng kthread also for untrusted sources

[Dominik Brodowski]

Start the hwrng kthread even if the hwrng source has a quality setting
of zero. Then, every crng reseed interval, one batch of data from this
zero-quality hwrng source will be mixed into the CRNG pool.

This patch is based on the assumption that data from a hwrng source
will not actively harm the CRNG state. Instead, many hwrng sources
(such as TPM devices), even though they are assigend a quality level of
zero, actually provide some entropy, which is good enough to mix into
the CRNG pool every once in a while.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
Thanks to Jason for splitting my v1 into two parts, as per Herbert's
request. In comparison to v2, I've updated (and shortened) the commit
message.

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 16f227b995e8..edb86c0cccda 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -52,7 +52,7 @@ MODULE_PARM_DESC(default_quality,
 
 static void drop_current_rng(void);
 static int hwrng_init(struct hwrng *rng);
-static void hwrng_manage_rngd(struct hwrng *rng);
+static int hwrng_fillfn(void *unused);
 
 static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
 			       int wait);
@@ -96,6 +96,15 @@ static int set_current_rng(struct hwrng *rng)
 	drop_current_rng();
 	current_rng = rng;
 
+	/* if necessary, start hwrng thread */
+	if (!hwrng_fill) {
+		hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
+		if (IS_ERR(hwrng_fill)) {
+			pr_err("hwrng_fill thread creation failed\n");
+			hwrng_fill = NULL;
+		}
+	}
+
 	return 0;
 }
 
@@ -167,8 +176,6 @@ static int hwrng_init(struct hwrng *rng)
 		rng->quality = 1024;
 	current_quality = rng->quality; /* obsolete */
 
-	hwrng_manage_rngd(rng);
-
 	return 0;
 }
 
@@ -454,10 +461,6 @@ static ssize_t rng_quality_store(struct device *dev,
 	/* the best available RNG may have changed */
 	ret = enable_best_rng();
 
-	/* start/stop rngd if necessary */
-	if (current_rng)
-		hwrng_manage_rngd(current_rng);
-
 out:
 	mutex_unlock(&rng_mutex);
 	return ret ? ret : len;
@@ -509,9 +512,6 @@ static int hwrng_fillfn(void *unused)
 		mutex_unlock(&reading_mutex);
 		put_rng(rng);
 
-		if (!quality)
-			break;
-
 		if (rc <= 0) {
 			pr_warn("hwrng: no data available\n");
 			msleep_interruptible(10000);
@@ -533,22 +533,6 @@ static int hwrng_fillfn(void *unused)
 	return 0;
 }
 
-static void hwrng_manage_rngd(struct hwrng *rng)
-{
-	if (WARN_ON(!mutex_is_locked(&rng_mutex)))
-		return;
-
-	if (rng->quality == 0 && hwrng_fill)
-		kthread_stop(hwrng_fill);
-	if (rng->quality > 0 && !hwrng_fill) {
-		hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
-		if (IS_ERR(hwrng_fill)) {
-			pr_err("hwrng_fill thread creation failed\n");
-			hwrng_fill = NULL;
-		}
-	}
-}
-
 int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;

Re: [PATCH v3] hw_random: core: start hwrng kthread also for untrusted sources

[Herbert Xu]

On Thu, Sep 22, 2022 at 03:59:31PM +0200, Dominik Brodowski wrote:
> Start the hwrng kthread even if the hwrng source has a quality setting
> of zero. Then, every crng reseed interval, one batch of data from this
> zero-quality hwrng source will be mixed into the CRNG pool.
> 
> This patch is based on the assumption that data from a hwrng source
> will not actively harm the CRNG state. Instead, many hwrng sources
> (such as TPM devices), even though they are assigend a quality level of
> zero, actually provide some entropy, which is good enough to mix into
> the CRNG pool every once in a while.
> 
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> ---
> Thanks to Jason for splitting my v1 into two parts, as per Herbert's
> request. In comparison to v2, I've updated (and shortened) the commit
> message.

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