| Message ID | YcTIM+MWEbMGLpRa@light.dominikbrodowski.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] random: fix crash on multiple early calls to add_bootloader_randomness() | expand |
Hi Dominik, Thanks. I like where this is going. It seems like we can tackle this problem and the other one separately like this as you've done. It'd be helpful though if you could send the patches for all the problems in one patchset so that I'm not left wondering, "hey what about this other thing; did you forget it?" Since these issues are kind of intermingled, it'd be nice to see how the puzzle pieces fit together. One question about this patch: - If crng_reseed is called early, before system_wq is non-NULL, that block will be skipped. When will it be called again? - There's no call to it in crng_initialize_primary/rand_initialize when not trusting the CPU and such, and that block there isn't quite the same as crng_reseed either. Also, something I noticed when looking at this, which I'm not sure has come up yet in the various problems identified: - If crng_reseed is called early, but not too early, and that block is called, we'll set crng_init=2 and do various things and print, "crng init done\n". - Later, when rand_initialize is called, if we're trusting the CPU and such, we'll re-initialize it and print, "crng done (trusting CPU's manufacturer)\n". That seems like a problem, though I assume we haven't hit that yet because the race window is pretty small, so we've mostly been crashing on a NULL system_wq instead, or having crng_reseed called _after_ crng_initialize_primary/rand_initialize anyway. Thanks a lot for working on this. If you get tired of this back and forth, by the way, and want me to start proposing patches for you to look at instead, we can trade off. I'm also happy to keep looking at what you send; it's just that as we're already on v6 here, I'm hoping you won't get too frustrated. Jason
diff --git a/drivers/char/random.c b/drivers/char/random.c index 13c968b950c5..3c44f5ff6cc4 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -993,7 +993,10 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r) memzero_explicit(&buf, sizeof(buf)); WRITE_ONCE(crng->init_time, jiffies); spin_unlock_irqrestore(&crng->lock, flags); - if (crng == &primary_crng && crng_init < 2) { + /* Only finalize initialization if workqueues are ready; otherwise + * numa_crng_init() and other things may go wrong. + */ + if (crng == &primary_crng && crng_init < 2 && system_wq) { invalidate_batched_entropy(); numa_crng_init(); crng_init = 2; @@ -2299,7 +2302,8 @@ void add_hwgenerator_randomness(const char *buffer, size_t count, * We'll be woken up again once below random_write_wakeup_thresh, * or when the calling thread is about to terminate. */ - wait_event_interruptible(random_write_wait, kthread_should_stop() || + wait_event_interruptible(random_write_wait, + !system_wq || kthread_should_stop() || ENTROPY_BITS(&input_pool) <= random_write_wakeup_bits); mix_pool_bytes(poolp, buffer, count); credit_entropy_bits(poolp, entropy);
Currently, if CONFIG_RANDOM_TRUST_BOOTLOADER is enabled, multiple calls to add_bootloader_randomness() are broken and can cause a NULL pointer dereference, as noted by Ivan T. Ivanov. This is not only a hypothetical problem, as qemu on arm64 may provide bootloader entropy via EFI and via devicetree. On the first call to add_hwgenerator_randomness(), crng_fast_load() is executed, and if the seed is long enough, crng_init will be set to 1. On subsequent calls to add_bootloader_randomness() and then to add_hwgenerator_randomness(), crng_fast_load() will be skipped. Instead, wait_event_interruptible() and then credit_entropy_bits() will be called. If the entropy count for that second seed is large enough, that proceeds to crng_reseed(). However, both wait_event_interruptible() and crng_reseed() depends (at least in numa_crng_init()) on workqueues. Therefore, test whether system_wq is already initialized, which is a sufficient indicator that workqueue_init_early() has progressed far enough. Reported-by: Ivan T. Ivanov <iivanov@suse.de> Fixes: 18b915ac6b0a ("efi/random: Treat EFI_RNG_PROTOCOL output as bootloader randomness") Cc: stable@vger.kernel.org Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> --- drivers/char/random.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) --- This is now a truly minimalist approach which tests for system_wq != NULL, as suggested by Jason. Another issue remains, though, but should be addressed separately: If one trusts the randnomness provided by the bootloader, and if the primary_crng is then seeded with 512 bits of entropy, warnings will still be emited that unseeded randomness is used with crng_init=1.