| Message ID | 20221116161642.1670235-1-Jason@zx2c4.com |
|---|---|
| Headers | show |
| Series | Use EFI variables for random seed | expand |
On Wed 2022-11-16 17:16:38, Jason A. Donenfeld wrote: > Rather than polling every second, use the new notifier to do this at > exactly the right moment. Great news! > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -752,26 +753,21 @@ early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable); > > static bool filled_random_ptr_key __read_mostly; > static siphash_key_t ptr_key __read_mostly; > -static void fill_ptr_key_workfn(struct work_struct *work); > -static DECLARE_DELAYED_WORK(fill_ptr_key_work, fill_ptr_key_workfn); > > -static void fill_ptr_key_workfn(struct work_struct *work) > +static int fill_ptr_key(struct notifier_block *nb, unsigned long action, void *data) > { > - if (!rng_is_initialized()) { > - queue_delayed_work(system_unbound_wq, &fill_ptr_key_work, HZ * 2); > - return; > - } > - > get_random_bytes(&ptr_key, sizeof(ptr_key)); > > /* Pairs with smp_rmb() before reading ptr_key. */ > smp_wmb(); > WRITE_ONCE(filled_random_ptr_key, true); > + return 0; I believe that we should rather return NOTIFY_DONE here. It is rather a formal change. The value is 0 as well. That said, I have never really understood the difference between NOTIFY_OK and NOTIFY_DONE. > } > > static int __init vsprintf_init_hashval(void) > { > - fill_ptr_key_workfn(NULL); > + static struct notifier_block fill_ptr_key_nb = { .notifier_call = fill_ptr_key }; > + notify_on_rng_initialized(&fill_ptr_key_nb); > return 0; > } > subsys_initcall(vsprintf_init_hashval) Anyway, the code looks good to me: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On Fri, Nov 18, 2022 at 3:16 PM Petr Mladek <pmladek@suse.com> wrote: > > + return 0; > > I believe that we should rather return NOTIFY_DONE here. > It is rather a formal change. The value is 0 as well. > > That said, I have never really understood the difference between > NOTIFY_OK and NOTIFY_DONE. Ah yes, the varying degrees of apathy: #define NOTIFY_DONE 0x0000 /* Don't care */ #define NOTIFY_OK 0x0001 /* Suits me */ In a sense, the fact that there's this return value at all indicates a notifier block isn't *quite* the API we want, since this happens only once and it really should never stop. But it's so convenient and small to use that I think it's fine. Anyway, I'll use the right constant here as you suggested. Jason
This is a rough sketch of a proposal to use non-volatile EFI variables as random seeds for EFISTUB to manage. Patch 1 adds (back) the random.c async notifier, so we can learn when the RNG is initialized. Patch 2 uses it in vsprintf, because I promised Sebastian we'd do that if it ever gets added back for whatever reason. Patch 3 is already in efi.git and isn't new here, but is a pre-req for the next patch. Patch 4 uses the random seed from an EFI variable to pass to Linux. Patch 5 prevents the variable from being read by efivarfs. [Note: probably the legacy efifs needs updating too? Or has this been removed?] Patch 6 uses patch 1 to refresh the EFI variable when the RNG is initialized. If folks like this idea and it moves forward, 1,2,6 will be taken into my tree, and 3,4,5 will go via Ard's. Commit messages are rather sparse at the moment. I'll fill those out for the next non-RFC patchset if this idea isn't immediately demolished. The biggest consideration is wear leveling on the EFI variable flash chips. However, EFI *already* winds up writing to non-volatile memory on every single boot anyway, so maybe it's not actually a big deal? Thoughts? Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Lennart Poettering <lennart@poettering.net> Ard Biesheuvel (1): efi: random: combine bootloader provided RNG seed with RNG protocol output Jason A. Donenfeld (5): random: add back async readiness notifier vsprintf: initialize siphash key using notifier efi: stub: use random seed from EFI variable efi: efivarfs: prohibit reading random seed variables efi: refresh non-volatile random seed when RNG is initialized drivers/char/random.c | 30 +++++++++ drivers/firmware/efi/efi.c | 14 +++++ drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/random.c | 85 +++++++++++++++++++++++--- fs/efivarfs/file.c | 3 + include/linux/efi.h | 3 +- include/linux/random.h | 1 + lib/vsprintf.c | 14 ++--- 8 files changed, 131 insertions(+), 21 deletions(-)