| Message ID | YanOIvAV1iPBEXR3@light.dominikbrodowski.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] random: fix crash on multiple early calls to add_bootloader_randomness() | expand |
On Fri, Dec 3, 2021 at 3:59 PM Dominik Brodowski <linux@dominikbrodowski.net> wrote: > > Hi Jason, > > Am Thu, Dec 02, 2021 at 11:55:10AM -0500 schrieb Jason A. Donenfeld: > > Thanks for the patch. One trivial nit and one question: > > Thanks for your review! > > > On Thu, Dec 2, 2021 at 6:35 AM Dominik Brodowski > > <linux@dominikbrodowski.net> wrote: > > > + /* We cannot do much with the input pool until it is set up in > > > + * rand_initalize(); therefore just mix into the crng state. > > > > I think you meant "rand_initialize()" here (missing 'i'). > > Indeed, sorry about that. > > > > If the added entropy suffices to increase crng_init to 1, future calls > > > to add_bootloader_randomness() or add_hwgenerator_randomness() used to > > > progress to credit_entropy_bits(). However, if the input pool is not yet > > > properly set up, the cmpxchg call within that function can lead to an > > > infinite recursion. > > > > I see what this patch does with crng_global_init_time, and that seems > > probably sensible, but I didn't understand this part of the reasoning > > in the commit message; I might just be a bit slow here. Where's the > > recursion exactly? Or even an infinite loop? > > On arm64, it was actually a NULL pointer dereference reported by Ivan T. > Ivanov; see > > https://lore.kernel.org/lkml/20211012082708.121931-1-iivanov@suse.de/ > > Trying to reproduce this rather bluntly on x86/qemu by multiple manual calls > to add_bootloader_randomness(), I mis-interpreted the symptoms to point to an > infinite recursion. The real problem seems to be that crng_reseed() isn't > ready to be called too early in the boot process, in particular before > workqueues are ready (see the call to numa_crng_init()). > > However, there seem be additional issues with add_bootloader_randomness() > not yet addressed (or worsened) by my patch: > > - If CONFIG_RANDOM_TRUST_BOOTLOADER is enabled and crng_init==0, > add_hwgenerator_randomness() calls crng_fast_load() and returns > immediately. If it is disabled and crng_init==0, > add_device_randnomness() calls crng_slow_load() but still > continues to call _mix_pool_bytes(). That means the seed is > used more extensively if CONFIG_RANDOM_TRUST_BOOTLOADER is not > set! If called by the crng_slow_load(), it's mixed into the pool but we're not trusting it. But in crng_fast_load() we're using it to init crng. > > - If CONFIG_RANDOM_TRUST_BOOTLOADER is enabled and crng_init==0, > the entropy is not credited -- same as if > CONFIG_RANDOM_TRUST_BOOTLOADER is not set. Only subsequent calls In crng_fast_load(), the seed would be mixed to primary_crng.state[4], and then crng_init will be 1 if the added seed is enough. rng-seed in dt (called in early_init_dt_scan_chosen()) also needs to use this function to init crng. With the patch, we're seeing [ 0.000000] random: get_random_u64 called from __kmem_cache_create+0x34/0x270 with crng_init=0 While before it should be [ 0.000000] random: get_random_u64 called from __kmem_cache_create+0x34/0x280 with crng_init=1 > to add_bootloader_randomness() would credit entropy, but that > causes the issue NULL pointer dereference or the hang... > > - As crng_fast_load() returns early, that actually means that my > patch causes the additional entropy submitted to > add_hwgenerator_randomness() by subsequent calls to be completely > lost. Only when crng_init==0, if crng is initialized, it would continue with credit_entropy_bits(). > > - For add_bootloader_randomness(), it makes no sense at all to call > wait_event_interruptible(). > > Therefore, it might make more sense to > > - modify add_bootloader_randomness() to always call > add_device_randomness(), and if CONFIG_RANDOM_TRUST_BOOTLOADER is > enabled, to call credit_entropy_bits(). > > - update credit_entropy_bits() to not call credit_entropy_bits() > if crng_global_init_time==0, as workqueues (and possibly other > infrastructure) might not be available at that time. > > What do you think? Draft patch below. @Ivan: Could you re-test on your > system, please? > > Thanks, > Dominik > > --- > > Currently, if CONFIG_RANDOM_TRUST_BOOTLOADER is enabled, mutliple 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. > However, no entropy is currently credited for that, even though the > name and description of CONFIG_RANDOM_TRUST_BOOTLOADER states otherwise. > > On subsequent calls to add_bootloader_randomness() and then to > add_hwgenerator_randomness(), crng_fast_load() will be skipped. Instead, > wait_event_interruptible() (which makes no sense for the init process) > 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, crng_reseed() may depend on workqueues being available, which > is not the case early during boot. > > To fix these issues, unconditionally call add_device_randomness() but not > add_hwgenerator_randomness() in add_bootloader_randomness(). This has the > additional advantage that the seed provided by the first call to > add_bootloader_randomness() is not only used by crng_{fast,slow}_load(), > but also mixed into the input pool. If CONFIG_RANDOM_TRUST_BOOTLOADER is > set, explicitly credit the entropy. However, avoid a call to crng_reseed() > too early during boot. It is safe to be called after rand_initialize(), > so use crng_global_init_time (which is set to != 0 in that function) to > determine which branch to take. > > Reported-by: Ivan T. Ivanov <iivanov@suse.de> > Fixes: 18b915ac6b0a ("efi/random: Treat EFI_RNG_PROTOCOL output as bootloader randomness") > Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> > > --- > v3->v4: complete rewrite > v2->v3: only one unlikely (Ard Biesheuvel) > v1->v2: fix commit message; unmerge Reported-and-tested-by-tag (Ard Biesheuvel) > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 605969ed0f96..d8614b426dfb 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -722,7 +722,8 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits) > if (r == &input_pool) { > int entropy_bits = entropy_count >> ENTROPY_SHIFT; > > - if (crng_init < 2 && entropy_bits >= 128) > + if (crng_init < 2 && entropy_bits >= 128 && > + crng_global_init_time > 0) > crng_reseed(&primary_crng, r); > } > } > @@ -1763,8 +1764,8 @@ static void __init init_std_data(struct entropy_store *r) > } > > /* > - * Note that setup_arch() may call add_device_randomness() > - * long before we get here. This allows seeding of the pools > + * add_device_randomness() or add_bootloader_randomness() may be > + * called long before we get here. This allows seeding of the pools > * with some platform dependent data very early in the boot > * process. But it limits our options here. We must use > * statically allocated structures that already have all > @@ -2291,15 +2292,13 @@ void add_hwgenerator_randomness(const char *buffer, size_t count, > EXPORT_SYMBOL_GPL(add_hwgenerator_randomness); > > /* Handle random seed passed by bootloader. > - * If the seed is trustworthy, it would be regarded as hardware RNGs. Otherwise > - * it would be regarded as device data. > + * If the seed is trustworthy, its entropy will be credited. > * The decision is controlled by CONFIG_RANDOM_TRUST_BOOTLOADER. > */ > void add_bootloader_randomness(const void *buf, unsigned int size) > { > + add_device_randomness(buf, size); > if (IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER)) > - add_hwgenerator_randomness(buf, size, size * 8); > - else > - add_device_randomness(buf, size); > + credit_entropy_bits(&input_pool, size * 8); > } > EXPORT_SYMBOL_GPL(add_bootloader_randomness);
On Tue, Dec 7, 2021 at 4:58 AM Dominik Brodowski <linux@dominikbrodowski.net> wrote: > > Am Mon, Dec 06, 2021 at 01:42:01PM +0800 schrieb Hsin-Yi Wang: > > On Fri, Dec 3, 2021 at 3:59 PM Dominik Brodowski > > <linux@dominikbrodowski.net> wrote: > > > > > > Hi Jason, > > > > > > Am Thu, Dec 02, 2021 at 11:55:10AM -0500 schrieb Jason A. Donenfeld: > > > > Thanks for the patch. One trivial nit and one question: > > > > > > Thanks for your review! > > > > > > > On Thu, Dec 2, 2021 at 6:35 AM Dominik Brodowski > > > > <linux@dominikbrodowski.net> wrote: > > > > > + /* We cannot do much with the input pool until it is set up in > > > > > + * rand_initalize(); therefore just mix into the crng state. > > > > > > > > I think you meant "rand_initialize()" here (missing 'i'). > > > > > > Indeed, sorry about that. > > > > > > > > If the added entropy suffices to increase crng_init to 1, future calls > > > > > to add_bootloader_randomness() or add_hwgenerator_randomness() used to > > > > > progress to credit_entropy_bits(). However, if the input pool is not yet > > > > > properly set up, the cmpxchg call within that function can lead to an > > > > > infinite recursion. > > > > > > > > I see what this patch does with crng_global_init_time, and that seems > > > > probably sensible, but I didn't understand this part of the reasoning > > > > in the commit message; I might just be a bit slow here. Where's the > > > > recursion exactly? Or even an infinite loop? > > > > > > On arm64, it was actually a NULL pointer dereference reported by Ivan T. > > > Ivanov; see > > > > > > https://lore.kernel.org/lkml/20211012082708.121931-1-iivanov@suse.de/ > > > > > > Trying to reproduce this rather bluntly on x86/qemu by multiple manual calls > > > to add_bootloader_randomness(), I mis-interpreted the symptoms to point to an > > > infinite recursion. The real problem seems to be that crng_reseed() isn't > > > ready to be called too early in the boot process, in particular before > > > workqueues are ready (see the call to numa_crng_init()). > > > > > > However, there seem be additional issues with add_bootloader_randomness() > > > not yet addressed (or worsened) by my patch: > > > > > > - If CONFIG_RANDOM_TRUST_BOOTLOADER is enabled and crng_init==0, > > > add_hwgenerator_randomness() calls crng_fast_load() and returns > > > immediately. If it is disabled and crng_init==0, > > > add_device_randnomness() calls crng_slow_load() but still > > > continues to call _mix_pool_bytes(). That means the seed is > > > used more extensively if CONFIG_RANDOM_TRUST_BOOTLOADER is not > > > set! > > If called by the crng_slow_load(), it's mixed into the pool but we're > > not trusting it. But in crng_fast_load() we're using it to init crng. > > > > > > > > - If CONFIG_RANDOM_TRUST_BOOTLOADER is enabled and crng_init==0, > > > the entropy is not credited -- same as if > > > CONFIG_RANDOM_TRUST_BOOTLOADER is not set. Only subsequent calls > > > > In crng_fast_load(), the seed would be mixed to primary_crng.state[4], > > Actually, that is also the case for crng_slow_load() (see dest_buf there). > Right, but the difference is if we want to credit(trust) that for crng init. > > and then crng_init will be 1 if the added seed is enough. > > rng-seed in dt (called in early_init_dt_scan_chosen()) also needs to > > use this function to init crng. > > Indeed, crng_init should be set to 1 in that case. > > > With the patch, we're seeing > > [ 0.000000] random: get_random_u64 called from > > __kmem_cache_create+0x34/0x270 with crng_init=0 > > > > While before it should be > > [ 0.000000] random: get_random_u64 called from > > __kmem_cache_create+0x34/0x280 with crng_init=1 > > > > > to add_bootloader_randomness() would credit entropy, but that > > > causes the issue NULL pointer dereference or the hang... > > > > > > - As crng_fast_load() returns early, that actually means that my > > > patch causes the additional entropy submitted to > > > add_hwgenerator_randomness() by subsequent calls to be completely > > > lost. > > Only when crng_init==0, if crng is initialized, it would continue with > > credit_entropy_bits(). > > However, if workqueues are not up and running (yet), it will fail. > > New draft below! Thanks, the new draft now takes care of the crng init. [ 0.000000] random: get_random_u64 called from __kmem_cache_create+0x34/0x270 with crng_init=1 > > Thanks, > Dominik > > --- > > Currently, if CONFIG_RANDOM_TRUST_BOOTLOADER is enabled, mutliple 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. > However, no entropy is currently credited for that, even though the > name and description of CONFIG_RANDOM_TRUST_BOOTLOADER states otherwise. > > On subsequent calls to add_bootloader_randomness() and then to > add_hwgenerator_randomness(), crng_fast_load() will be skipped. Instead, > wait_event_interruptible() (which makes no sense for the init process) > 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, crng_reseed() may depend on workqueues being available, which > is not the case early during boot. > > To fix these issues, explicitly call crng_fast_load() or crng_slow_load() > depending on whether the bootloader is trusted -- only in the first > instance, crng_init may progress to 1. Also, mix the seed into the > input pool unconditionally, and credit the entropy for that iff > CONFIG_RANDOM_TRUST_BOOTLOADER is set. However, avoid a call to > crng_reseed() too early during boot. It is safe to be called after > rand_initialize(), so use crng_global_init_time (which is set to != 0 > in that function) to determine which branch to take. > > Reported-by: Ivan T. Ivanov <iivanov@suse.de> > Fixes: 18b915ac6b0a ("efi/random: Treat EFI_RNG_PROTOCOL output as bootloader randomness") > Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 605969ed0f96..abe4571fd2c0 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -722,7 +722,8 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits) > if (r == &input_pool) { > int entropy_bits = entropy_count >> ENTROPY_SHIFT; > > - if (crng_init < 2 && entropy_bits >= 128) > + if (crng_init < 2 && entropy_bits >= 128 && > + crng_global_init_time > 0) > crng_reseed(&primary_crng, r); > } > } > @@ -1763,8 +1764,8 @@ static void __init init_std_data(struct entropy_store *r) > } > > /* > - * Note that setup_arch() may call add_device_randomness() > - * long before we get here. This allows seeding of the pools > + * add_device_randomness() or add_bootloader_randomness() may be > + * called long before we get here. This allows seeding of the pools > * with some platform dependent data very early in the boot > * process. But it limits our options here. We must use > * statically allocated structures that already have all > @@ -2291,15 +2292,29 @@ void add_hwgenerator_randomness(const char *buffer, size_t count, > EXPORT_SYMBOL_GPL(add_hwgenerator_randomness); > > /* Handle random seed passed by bootloader. > - * If the seed is trustworthy, it would be regarded as hardware RNGs. Otherwise > - * it would be regarded as device data. > + * If the seed is trustworthy, its entropy will be credited. > * The decision is controlled by CONFIG_RANDOM_TRUST_BOOTLOADER. > */ > void add_bootloader_randomness(const void *buf, unsigned int size) > { > - if (IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER)) > - add_hwgenerator_randomness(buf, size, size * 8); > - else > - add_device_randomness(buf, size); > + unsigned long time = random_get_entropy() ^ jiffies; > + unsigned long flags; > + > + if (!crng_ready() && size) { size is checked here but not below? > +#ifdef CONFIG_RANDOM_TRUST_BOOTLOADER > + crng_fast_load(buf, size); > +#else > + crng_slow_load(buf, size); > +#endif /* CONFIG_RANDOM_TRUST_BOOTLOADER */ > + } > + > + spin_lock_irqsave(&input_pool.lock, flags); > + _mix_pool_bytes(&input_pool, buf, size); > + _mix_pool_bytes(&input_pool, &time, sizeof(time)); > + spin_unlock_irqrestore(&input_pool.lock, flags); > + > +#ifdef CONFIG_RANDOM_TRUST_BOOTLOADER > + credit_entropy_bits(&input_pool, size * 8); > +#endif > } > EXPORT_SYMBOL_GPL(add_bootloader_randomness);
Am Tue, Dec 07, 2021 at 03:09:21PM +0800 schrieb Hsin-Yi Wang: > On Tue, Dec 7, 2021 at 4:58 AM Dominik Brodowski > <linux@dominikbrodowski.net> wrote: > > > > Am Mon, Dec 06, 2021 at 01:42:01PM +0800 schrieb Hsin-Yi Wang: > > > On Fri, Dec 3, 2021 at 3:59 PM Dominik Brodowski > > > <linux@dominikbrodowski.net> wrote: > > > > > > > > Hi Jason, > > > > > > > > Am Thu, Dec 02, 2021 at 11:55:10AM -0500 schrieb Jason A. Donenfeld: > > > > > Thanks for the patch. One trivial nit and one question: > > > > > > > > Thanks for your review! > > > > > > > > > On Thu, Dec 2, 2021 at 6:35 AM Dominik Brodowski > > > > > <linux@dominikbrodowski.net> wrote: > > > > > > + /* We cannot do much with the input pool until it is set up in > > > > > > + * rand_initalize(); therefore just mix into the crng state. > > > > > > > > > > I think you meant "rand_initialize()" here (missing 'i'). > > > > > > > > Indeed, sorry about that. > > > > > > > > > > If the added entropy suffices to increase crng_init to 1, future calls > > > > > > to add_bootloader_randomness() or add_hwgenerator_randomness() used to > > > > > > progress to credit_entropy_bits(). However, if the input pool is not yet > > > > > > properly set up, the cmpxchg call within that function can lead to an > > > > > > infinite recursion. > > > > > > > > > > I see what this patch does with crng_global_init_time, and that seems > > > > > probably sensible, but I didn't understand this part of the reasoning > > > > > in the commit message; I might just be a bit slow here. Where's the > > > > > recursion exactly? Or even an infinite loop? > > > > > > > > On arm64, it was actually a NULL pointer dereference reported by Ivan T. > > > > Ivanov; see > > > > > > > > https://lore.kernel.org/lkml/20211012082708.121931-1-iivanov@suse.de/ > > > > > > > > Trying to reproduce this rather bluntly on x86/qemu by multiple manual calls > > > > to add_bootloader_randomness(), I mis-interpreted the symptoms to point to an > > > > infinite recursion. The real problem seems to be that crng_reseed() isn't > > > > ready to be called too early in the boot process, in particular before > > > > workqueues are ready (see the call to numa_crng_init()). > > > > > > > > However, there seem be additional issues with add_bootloader_randomness() > > > > not yet addressed (or worsened) by my patch: > > > > > > > > - If CONFIG_RANDOM_TRUST_BOOTLOADER is enabled and crng_init==0, > > > > add_hwgenerator_randomness() calls crng_fast_load() and returns > > > > immediately. If it is disabled and crng_init==0, > > > > add_device_randnomness() calls crng_slow_load() but still > > > > continues to call _mix_pool_bytes(). That means the seed is > > > > used more extensively if CONFIG_RANDOM_TRUST_BOOTLOADER is not > > > > set! > > > If called by the crng_slow_load(), it's mixed into the pool but we're > > > not trusting it. But in crng_fast_load() we're using it to init crng. > > > > > > > > > > > - If CONFIG_RANDOM_TRUST_BOOTLOADER is enabled and crng_init==0, > > > > the entropy is not credited -- same as if > > > > CONFIG_RANDOM_TRUST_BOOTLOADER is not set. Only subsequent calls > > > > > > In crng_fast_load(), the seed would be mixed to primary_crng.state[4], > > > > Actually, that is also the case for crng_slow_load() (see dest_buf there). > > > Right, but the difference is if we want to credit(trust) that for crng init. ... which is, unfortunately, not the only difference between slow and fast... > > > and then crng_init will be 1 if the added seed is enough. > > > rng-seed in dt (called in early_init_dt_scan_chosen()) also needs to > > > use this function to init crng. > > > > Indeed, crng_init should be set to 1 in that case. > > > > > With the patch, we're seeing > > > [ 0.000000] random: get_random_u64 called from > > > __kmem_cache_create+0x34/0x270 with crng_init=0 > > > > > > While before it should be > > > [ 0.000000] random: get_random_u64 called from > > > __kmem_cache_create+0x34/0x280 with crng_init=1 > > > > > > > to add_bootloader_randomness() would credit entropy, but that > > > > causes the issue NULL pointer dereference or the hang... > > > > > > > > - As crng_fast_load() returns early, that actually means that my > > > > patch causes the additional entropy submitted to > > > > add_hwgenerator_randomness() by subsequent calls to be completely > > > > lost. > > > Only when crng_init==0, if crng is initialized, it would continue with > > > credit_entropy_bits(). > > > > However, if workqueues are not up and running (yet), it will fail. > > > > New draft below! > > Thanks, the new draft now takes care of the crng init. > [ 0.000000] random: get_random_u64 called from > __kmem_cache_create+0x34/0x270 with crng_init=1 Thanks for testing! > > --- > > > > Currently, if CONFIG_RANDOM_TRUST_BOOTLOADER is enabled, mutliple 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. > > However, no entropy is currently credited for that, even though the > > name and description of CONFIG_RANDOM_TRUST_BOOTLOADER states otherwise. > > > > On subsequent calls to add_bootloader_randomness() and then to > > add_hwgenerator_randomness(), crng_fast_load() will be skipped. Instead, > > wait_event_interruptible() (which makes no sense for the init process) > > 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, crng_reseed() may depend on workqueues being available, which > > is not the case early during boot. > > > > To fix these issues, explicitly call crng_fast_load() or crng_slow_load() > > depending on whether the bootloader is trusted -- only in the first > > instance, crng_init may progress to 1. Also, mix the seed into the > > input pool unconditionally, and credit the entropy for that iff > > CONFIG_RANDOM_TRUST_BOOTLOADER is set. However, avoid a call to > > crng_reseed() too early during boot. It is safe to be called after > > rand_initialize(), so use crng_global_init_time (which is set to != 0 > > in that function) to determine which branch to take. > > > > Reported-by: Ivan T. Ivanov <iivanov@suse.de> > > Fixes: 18b915ac6b0a ("efi/random: Treat EFI_RNG_PROTOCOL output as bootloader randomness") > > Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > index 605969ed0f96..abe4571fd2c0 100644 > > --- a/drivers/char/random.c > > +++ b/drivers/char/random.c > > @@ -722,7 +722,8 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits) > > if (r == &input_pool) { > > int entropy_bits = entropy_count >> ENTROPY_SHIFT; > > > > - if (crng_init < 2 && entropy_bits >= 128) > > + if (crng_init < 2 && entropy_bits >= 128 && > > + crng_global_init_time > 0) > > crng_reseed(&primary_crng, r); > > } > > } > > @@ -1763,8 +1764,8 @@ static void __init init_std_data(struct entropy_store *r) > > } > > > > /* > > - * Note that setup_arch() may call add_device_randomness() > > - * long before we get here. This allows seeding of the pools > > + * add_device_randomness() or add_bootloader_randomness() may be > > + * called long before we get here. This allows seeding of the pools > > * with some platform dependent data very early in the boot > > * process. But it limits our options here. We must use > > * statically allocated structures that already have all > > @@ -2291,15 +2292,29 @@ void add_hwgenerator_randomness(const char *buffer, size_t count, > > EXPORT_SYMBOL_GPL(add_hwgenerator_randomness); > > > > /* Handle random seed passed by bootloader. > > - * If the seed is trustworthy, it would be regarded as hardware RNGs. Otherwise > > - * it would be regarded as device data. > > + * If the seed is trustworthy, its entropy will be credited. > > * The decision is controlled by CONFIG_RANDOM_TRUST_BOOTLOADER. > > */ > > void add_bootloader_randomness(const void *buf, unsigned int size) > > { > > - if (IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER)) > > - add_hwgenerator_randomness(buf, size, size * 8); > > - else > > - add_device_randomness(buf, size); > > + unsigned long time = random_get_entropy() ^ jiffies; > > + unsigned long flags; > > + > > + if (!crng_ready() && size) { > size is checked here but not below? credit_entropy_bits() returns early if bits==0. Thanks, Dominik
Hi Dominik, Thanks for the followup patch. Some comments below: On Mon, Dec 6, 2021 at 9:58 PM Dominik Brodowski <linux@dominikbrodowski.net> wrote: > @@ -722,7 +722,8 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits) > if (r == &input_pool) { > int entropy_bits = entropy_count >> ENTROPY_SHIFT; > > - if (crng_init < 2 && entropy_bits >= 128) > + if (crng_init < 2 && entropy_bits >= 128 && > + crng_global_init_time > 0) crng_global_init_time is being used because it's set in rand_initialize(), and rand_initialize() is called after workqueues have been set up. Fair enough. But: a) It's still set to `jiffies - 1` afterwards at runtime, via ioctl(RNDRESEEDCRNG). I'm not sure if we want to mess around with having a 1:2**32 chance of getting this at the unlucky 0 wraparound. It seems like that kind of strategy generally works if unlikely failure is tolerable, but it seems like that's not a game to play here. There are a few other options: b) Use another proxy for the state, like rand_initialize()->primary_crng.state (ugly) or something better. c) Add another static global state variable or static branch to random.c. d) Actually conditionalize this on whether or not workqueues have been init'd, which is *actually* the thing you care about, not whether rand_initialize() has fired. I think that of these, (d) seems the most correct. The thing you're *actually* trying to prevent is that `schedule_work(&numa_crng_init_work)` call being triggered before workqueues are up. It looks like system_wq is made non-NULL by workqueue_init_early(); perhaps you could get away with conditionalizing it on that instead? This also seems like a distinct state machine derp from the rest of the patch, so I wonder if it could be separated out into a more trivial patch, in case it does prove problematic somehow. > +#ifdef CONFIG_RANDOM_TRUST_BOOTLOADER Can you use IF_ENABLED() like the code that this replaces? Easier to read and ensures syntax doesn't regress on less-used configurations. > void add_bootloader_randomness(const void *buf, unsigned int size) > + unsigned long time = random_get_entropy() ^ jiffies; > + unsigned long flags; > + > + if (!crng_ready() && size) { > +#ifdef CONFIG_RANDOM_TRUST_BOOTLOADER > + crng_fast_load(buf, size); > +#else > + crng_slow_load(buf, size); > +#endif /* CONFIG_RANDOM_TRUST_BOOTLOADER */ > + } > + > + spin_lock_irqsave(&input_pool.lock, flags); > + _mix_pool_bytes(&input_pool, buf, size); > + _mix_pool_bytes(&input_pool, &time, sizeof(time)); > + spin_unlock_irqrestore(&input_pool.lock, flags); > + > +#ifdef CONFIG_RANDOM_TRUST_BOOTLOADER > + credit_entropy_bits(&input_pool, size * 8); > +#endif > } > EXPORT_SYMBOL_GPL(add_bootloader_randomness); Trying to understand this and compare it to what was here before. Two cases: trustbootloader and !trustbootloader. trustbootloader looks like this: unsigned long time = random_get_entropy() ^ jiffies; unsigned long flags; if (!crng_ready() && size) crng_fast_load(buf, size); spin_lock_irqsave(&input_pool.lock, flags); _mix_pool_bytes(&input_pool, buf, size); _mix_pool_bytes(&input_pool, &time, sizeof(time)); spin_unlock_irqrestore(&input_pool.lock, flags); credit_entropy_bits(&input_pool, size * 8); !trustbooloader looks like this: unsigned long time = random_get_entropy() ^ jiffies; unsigned long flags; if (!crng_ready() && size) crng_slow_load(buf, size); spin_lock_irqsave(&input_pool.lock, flags); _mix_pool_bytes(&input_pool, buf, size); _mix_pool_bytes(&input_pool, &time, sizeof(time)); spin_unlock_irqrestore(&input_pool.lock, flags); So this means !trustbootloader is the same as add_device_randomness (with the call to trace_add_device_randomness removed). It'd probably make sense to continue just branching to add_device_randomness on an IS_ENABLED(), then, so it's more clear what's up, rather than open coding the distinct paths and copying code around. But trustbootloader is a different case. Here the differences appear to be: 1) crng_fast_load is now called for crng_init==1||crng_init==0 [via crng_ready()] instead of for crng_init==0; 2) The function now continues onward after calling crng_fast_load instead of returning; 3) The useless call to wait_event_interruptible is now skipped; 4) _mix_pool_bytes(random_get_entropy() ^ jiffies) is now called in addition to _mix_pool_bytes(buf). I'd now like to map (1), (2), (3), and (4) back to the rationale in your commit message. (2) seems to be related to: > 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. > However, no entropy is currently credited for that, even though the > name and description of CONFIG_RANDOM_TRUST_BOOTLOADER states otherwise. But it seems like rather than going from: if (!ready) { crng_fast_load(buf, size); return; } to: if (!ready) crng_fast_load(buf, size); the actually corresponding fix would be to go instead to: if (!ready) crng_fast_load(buf, size); if (!ready) return; (3) seems to be related to: > wait_event_interruptible() (which makes no sense for the init process) which is fine. (1) and (4) I don't see justification for. Perhaps it's a mistake? Regards, Jason
Hey Dominik, Just following up here, as I never heard back from you. These seem like real bug I'd like to fix at some point. I was waiting to hear back about your latest patch, and perhaps you wanted to spin a new revision? Or not, in which case, please let me know? Thanks, Jason
diff --git a/drivers/char/random.c b/drivers/char/random.c index 605969ed0f96..d8614b426dfb 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -722,7 +722,8 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits) if (r == &input_pool) { int entropy_bits = entropy_count >> ENTROPY_SHIFT; - if (crng_init < 2 && entropy_bits >= 128) + if (crng_init < 2 && entropy_bits >= 128 && + crng_global_init_time > 0) crng_reseed(&primary_crng, r); } } @@ -1763,8 +1764,8 @@ static void __init init_std_data(struct entropy_store *r) } /* - * Note that setup_arch() may call add_device_randomness() - * long before we get here. This allows seeding of the pools + * add_device_randomness() or add_bootloader_randomness() may be + * called long before we get here. This allows seeding of the pools * with some platform dependent data very early in the boot * process. But it limits our options here. We must use * statically allocated structures that already have all @@ -2291,15 +2292,13 @@ void add_hwgenerator_randomness(const char *buffer, size_t count, EXPORT_SYMBOL_GPL(add_hwgenerator_randomness); /* Handle random seed passed by bootloader. - * If the seed is trustworthy, it would be regarded as hardware RNGs. Otherwise - * it would be regarded as device data. + * If the seed is trustworthy, its entropy will be credited. * The decision is controlled by CONFIG_RANDOM_TRUST_BOOTLOADER. */ void add_bootloader_randomness(const void *buf, unsigned int size) { + add_device_randomness(buf, size); if (IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER)) - add_hwgenerator_randomness(buf, size, size * 8); - else - add_device_randomness(buf, size); + credit_entropy_bits(&input_pool, size * 8); } EXPORT_SYMBOL_GPL(add_bootloader_randomness);