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