| Message ID | 20220209011919.493762-7-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | random: cleanups around per-cpu crng & rdrand | expand |
Am Wed, Feb 09, 2022 at 02:19:16AM +0100 schrieb Jason A. Donenfeld: > During crng_init == 0, we never credit entropy in add_interrupt_ > randomness(), but instead dump it directly into the base_crng. That's > fine, except for the fact that we then wind up throwing away that > entropy later when we switch to extracting from the input pool and > overwriting the base_crng key. The two other early init sites -- > add_hwgenerator_randomness()'s use crng_fast_load() and add_device_ > randomness()'s use of crng_slow_load() -- always additionally give their > inputs to the input pool. But not add_interrupt_randomness(). Hm, up to this patch there is no base_crng key. So maybe change the ordering of the patches? > This commit fixes that shortcoming by calling mix_pool_bytes() after > crng_fast_load() in add_interrupt_randomness(). That's partially > verboten on PREEMPT_RT, where it implies taking spinlock_t from an IRQ > handler. But this also only happens during early boot and then never > again after that. Should this turn into a larger problem later for some > esoteric reason, we can always use `if (IS_ENABLED(PREEMPT_RT))` or > something similar to that. But for now, this should do. > > Cc: Theodore Ts'o <tytso@mit.edu> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Suggested-by: Eric Biggers <ebiggers@google.com> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/char/random.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 785a4545c9d7..20374bce9a07 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -864,6 +864,13 @@ void add_interrupt_randomness(int irq) > crng_fast_load((u8 *)fast_pool->pool, sizeof(fast_pool->pool)) > 0) { > fast_pool->count = 0; > fast_pool->last = now; > + > + /* Technically this call means that we're using a spinlock_t > + * in the IRQ handler, which isn't terrific for PREEMPT_RT. > + * However, this only happens during very early boot, and then Whether it's only during "very early" boot depends on how fast we progress to crng_init = 2. So maybe just "during boot"? Otherwise, all fine, so Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net> Thanks, Dominik
On Wed, Feb 9, 2022 at 9:31 AM Dominik Brodowski <linux@dominikbrodowski.net> wrote: > > Am Wed, Feb 09, 2022 at 02:19:16AM +0100 schrieb Jason A. Donenfeld: > > During crng_init == 0, we never credit entropy in add_interrupt_ > > randomness(), but instead dump it directly into the base_crng. That's > > fine, except for the fact that we then wind up throwing away that > > entropy later when we switch to extracting from the input pool and > > overwriting the base_crng key. The two other early init sites -- > > add_hwgenerator_randomness()'s use crng_fast_load() and add_device_ > > randomness()'s use of crng_slow_load() -- always additionally give their > > inputs to the input pool. But not add_interrupt_randomness(). > > Hm, up to this patch there is no base_crng key. So maybe change the ordering > of the patches? I'll fix the commit message, actually. Eric wrote in his review of v1 that he thinks this problem needs to be fixed before we move to overwriting keys in the subsequent patch and I agreed. Hence, this patch comes first. > > + > > + /* Technically this call means that we're using a spinlock_t > > + * in the IRQ handler, which isn't terrific for PREEMPT_RT. > > + * However, this only happens during very early boot, and then > > Whether it's only during "very early" boot depends on how fast we progress > to crng_init = 2. So maybe just "during boot"? Will do. Jason
diff --git a/drivers/char/random.c b/drivers/char/random.c index 785a4545c9d7..20374bce9a07 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -864,6 +864,13 @@ void add_interrupt_randomness(int irq) crng_fast_load((u8 *)fast_pool->pool, sizeof(fast_pool->pool)) > 0) { fast_pool->count = 0; fast_pool->last = now; + + /* Technically this call means that we're using a spinlock_t + * in the IRQ handler, which isn't terrific for PREEMPT_RT. + * However, this only happens during very early boot, and then + * never again, so we live with it. + */ + mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool)); } return; }
During crng_init == 0, we never credit entropy in add_interrupt_ randomness(), but instead dump it directly into the base_crng. That's fine, except for the fact that we then wind up throwing away that entropy later when we switch to extracting from the input pool and overwriting the base_crng key. The two other early init sites -- add_hwgenerator_randomness()'s use crng_fast_load() and add_device_ randomness()'s use of crng_slow_load() -- always additionally give their inputs to the input pool. But not add_interrupt_randomness(). This commit fixes that shortcoming by calling mix_pool_bytes() after crng_fast_load() in add_interrupt_randomness(). That's partially verboten on PREEMPT_RT, where it implies taking spinlock_t from an IRQ handler. But this also only happens during early boot and then never again after that. Should this turn into a larger problem later for some esoteric reason, we can always use `if (IS_ENABLED(PREEMPT_RT))` or something similar to that. But for now, this should do. Cc: Theodore Ts'o <tytso@mit.edu> Cc: Dominik Brodowski <linux@dominikbrodowski.net> Suggested-by: Eric Biggers <ebiggers@google.com> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/char/random.c | 7 +++++++ 1 file changed, 7 insertions(+)