| Message ID | 20220919160931.2945427-1-ardb@kernel.org |
|---|---|
| Headers | show |
| Series | efi: consume random seed provided by loader | expand |
On Mon, Sep 19, 2022 at 6:09 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > Instead of blindly creating the EFI random seed configuration table if > the RNG protocol is implemented and works, check whether such a EFI > configuration table was provided by an earlier boot stage and if so, > combine its contents with a Linux specific personalization string, and > if available, mix in the output of the RNG protocol as well. > > This can be used for, e.g., systemd-boot, to pass an additional seed to > Linux in a way that can be consumed by the kernel very early. In that > case, the following definitions should be used to pass the seed to the > EFI stub: > > struct linux_efi_random_seed { > u32 size; // of the 'seed' array in bytes > u8 seed[]; > }; > > The memory for the struct must be allocated as EFI_ACPI_RECLAIM_MEMORY > pool memory, and the address of the struct in memory should be installed > as a EFI configuration table using the following GUID: > > LINUX_EFI_RANDOM_SEED_TABLE_GUID 1ce1e5bc-7ceb-42f2-81e5-8aadf180f57b > > Note that doing so is safe even on kernels that were built without this > patch applied, but the seed will simply be overwritten with a seed > derived from the EFI RNG protocol, if available. The recommended seed > size is 32 bytes, anything beyond that is mixed in but not reflected in > the final seed size. > > Suggested-by: "Jason A. Donenfeld" <Jason@zx2c4.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com> (And I suppose you can trim those quotation marks in the suggested-by tag, since it's a git trailer rather than an email header.)
On Mo, 19.09.22 18:09, Ard Biesheuvel (ardb@kernel.org) wrote: Heya! Looks excellent! I was wondering though, shouldn't the memory the seed data is stored in be zeroed out when you dispose of it, just for safety? > + if (rng) { > + const int sz = EFI_RANDOM_SEED_SIZE; > + u8 bits[EFI_RANDOM_SEED_SIZE]; > > - if (status == EFI_UNSUPPORTED) > - /* > - * Use whatever algorithm we have available if the raw algorithm > - * is not implemented. > - */ > - status = efi_call_proto(rng, get_rng, NULL, > - EFI_RANDOM_SEED_SIZE, seed->bits); > + status = efi_call_proto(rng, get_rng, &rng_algo_raw, sz, bits); > + if (status == EFI_UNSUPPORTED) > + /* > + * Use whatever algorithm we have available if the raw algorithm > + * is not implemented. > + */ > + status = efi_call_proto(rng, get_rng, NULL, sz, bits); > > + if (status == EFI_SUCCESS) { > + blake2s_update(&state, (void *)&sz, sizeof(sz)); > + blake2s_update(&state, bits, sz); So, here, shouldn't bitſ[] be zeroed out? > - seed->size = EFI_RANDOM_SEED_SIZE; > + blake2s_final(&state, seed->bits); And here, shouldn't the state struct be zeroed out? (or does blake2s_final() do that implicitly? Looks excellent otherwise! Will-be-used-by: systemd Reviewed-by: Lennart Poettering <mzxreary@0pointer.net>
On Tue, 20 Sept 2022 at 18:28, Lennart Poettering <lennart@poettering.net> wrote: > > On Mo, 19.09.22 18:09, Ard Biesheuvel (ardb@kernel.org) wrote: > > Heya! > > Looks excellent! > > I was wondering though, shouldn't the memory the seed data is stored > in be zeroed out when you dispose of it, just for safety? > > > + if (rng) { > > + const int sz = EFI_RANDOM_SEED_SIZE; > > + u8 bits[EFI_RANDOM_SEED_SIZE]; > > > > - if (status == EFI_UNSUPPORTED) > > - /* > > - * Use whatever algorithm we have available if the raw algorithm > > - * is not implemented. > > - */ > > - status = efi_call_proto(rng, get_rng, NULL, > > - EFI_RANDOM_SEED_SIZE, seed->bits); > > + status = efi_call_proto(rng, get_rng, &rng_algo_raw, sz, bits); > > + if (status == EFI_UNSUPPORTED) > > + /* > > + * Use whatever algorithm we have available if the raw algorithm > > + * is not implemented. > > + */ > > + status = efi_call_proto(rng, get_rng, NULL, sz, bits); > > > > + if (status == EFI_SUCCESS) { > > + blake2s_update(&state, (void *)&sz, sizeof(sz)); > > + blake2s_update(&state, bits, sz); > So, here, shouldn't bitſ[] be zeroed out? > > > - seed->size = EFI_RANDOM_SEED_SIZE; > > + blake2s_final(&state, seed->bits); > > And here, shouldn't the state struct be zeroed out? (or does > blake2s_final() do that implicitly? > Yes, Jason pointed that out as well. (Twice, actually :-)) > Looks excellent otherwise! > > Will-be-used-by: systemd > Reviewed-by: Lennart Poettering <mzxreary@0pointer.net> Thanks, much appreciated.
On Tue, Sep 20, 2022 at 6:28 PM Lennart Poettering <lennart@poettering.net> wrote: > I was wondering though, shouldn't the memory the seed data is stored > in be zeroed out when you dispose of it, just for safety? I mentioned the same. I think Ard is gonna handle that for v2, in addition to freeing the prior seed's allocation. > > + blake2s_final(&state, seed->bits); > > And here, shouldn't the state struct be zeroed out? (or does > blake2s_final() do that implicitly? In this case, blake2s_final does it implicitly.
Getting a random seed into the kernel very early is important for data structures that rely on the random number generator at initialization time, but hardware based RNGs may not become available until much later in the boot. For this reason, the EFI stub will currently invoke the EFI RNG protocol to get a random seed from the hardware before tearing down the EFI boot services and performing the low level boot of the kernel proper. The generated seed is passed via a EFI configuration table, which is available very early, and so the random number generator comes up much earlier as well. Any boot stage preceding the EFI stub can install configuration tables, so we can decide to expose the same mechanism to other loaders. This allows, e.g., systemd-boot to pass the seed it keeps in a file on the ESP without having to rely on PID #1 dd'ing it into /dev/random, which is much too late to be useful. As suggested by Jason, let's use blake2s to combine seeds obtained via the config table and via the protocol if both are available, and throw in a personalization string for good measure. Older kernels will simply supersede the bootloader provided seed, unless the RNG protocol is not available, in which case the bootloader seed will be forwarded untouched if one is present. This should not be an issue, but let's reduce the seed size to blake2's output size, and switch to the correct memory type in separate changes so they can be backported. Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Cc: Jason A. Donenfeld <Jason@zx2c4.com> Cc: Lennart Poettering <lennart@poettering.net> Cc: Herbert Xu <herbert@gondor.apana.org.au> Ard Biesheuvel (3): efi: random: reduce seed size to 32 bytes efi: random: Use 'ACPI reclaim' memory for random seed efi: random: combine bootloader provided RNG seed with RNG protocol output drivers/firmware/efi/efi.c | 2 +- drivers/firmware/efi/libstub/Makefile | 4 + drivers/firmware/efi/libstub/efistub.h | 2 + drivers/firmware/efi/libstub/random.c | 83 ++++++++++++-------- include/linux/efi.h | 4 +- lib/crypto/blake2s-generic.c | 2 + lib/crypto/blake2s.c | 7 +- 7 files changed, 64 insertions(+), 40 deletions(-)