diff mbox series

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

Message ID 20220904080247.7890-1-linux@dominikbrodowski.net (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series random / hw_random: core: start hwrng kthread also for untrusted sources | expand

Commit Message

Dominik Brodowski Sept. 4, 2022, 8:02 a.m. UTC
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(-)

Comments

Herbert Xu Sept. 7, 2022, 6:34 a.m. UTC | #1
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,
Dominik Brodowski Sept. 7, 2022, 6:54 a.m. UTC | #2
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
Jason A. Donenfeld Sept. 7, 2022, 1:05 p.m. UTC | #3
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
Jason A. Donenfeld Sept. 20, 2022, 2:21 p.m. UTC | #4
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
diff mbox series

Patch

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