Hi Ard,
Thanks for doing this. Some comments below:
On Thu, Oct 20, 2022 at 10:39:10AM +0200, Ard Biesheuvel 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,
> concatenate the existing and the new seeds, leaving it up to the core
> code to mix it in and credit it the way it sees fit.
>
> 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 disregarded when the seeds are
> concatenated.
Since this commit message will be used by other implementers inevitably,
you might want to include something about forward secrecy, memzeroing
old allocations, etc.
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
You can treat overwriting the old seed as an accidental bug, and Cc this
as stable like the rest of this series.
> + /*
> + * Check whether a seed was provided by a prior boot stage. In that
> + * case, instead of overwriting it, let's create a new buffer that can
> + * hold both, and concatenate the existing and the new seeds.
> + */
> + prev_seed = get_efi_config_table(LINUX_EFI_RANDOM_SEED_TABLE_GUID);
> + if (prev_seed) {
> + prev_seed_size = min(prev_seed->size, EFI_RANDOM_SEED_SIZE);
> + seed_size += prev_seed_size;
> + }
I think you can omit the min() here. If a bootloader passes a massive
seed such that allocations later fail, that's a bootloader problem, and
we should just complain loudly about it.
> + seed->size = seed_size;
> + if (prev_seed)
> + memcpy(seed->bits + EFI_RANDOM_SEED_SIZE, prev_seed->bits,
> + prev_seed_size);
You memcpy here, but then:
> +
> status = efi_bs_call(install_configuration_table, &rng_table_guid, seed);
> if (status != EFI_SUCCESS)
> goto err_freepool;
If the installation here fails, you abort, leaving the memcpy'd seed in
memory without memzeroing.
>
> + if (prev_seed) {
> + /* wipe and free the old seed if we managed to install the new one */
> + memzero_explicit(prev_seed->bits, prev_seed_size);
> + efi_bs_call(free_pool, prev_seed);
> + }
> return EFI_SUCCESS;
>
> err_freepool:
> + efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
> + prev_seed ? ", retaining bootloader supplied seed only" : "");
So maybe it makes sense to stick a memzero here too, just before
freeing?
Also, I don't think that efi_warn() makes sense. In the easy case, if
the bootloader provides a seed bu EFI doesn't have a RNG_PROTOCOL
implementation, then that's a firmware limitation the user likely can't
do anything about, in which case it's really good that systemd-boot is
providing something instead. So that's not something to worry about.
In the more complicated cases, you jump here when
install_configuration_table() fails, so that's something to warn about,
but perhaps just before goto. Then, you also return early from the
function up at the call to allocate_pool, so that'd be a place to warn
about too.
Not seen in this diff, but relevant too is that you currently exit early
from the function if locate_protocol fails. If there's a systemd-boot
seed already, this is premature. Instead, move the call to
locate_protocol down to just before the call to get_rng.
Jason
> efi_bs_call(free_pool, seed);
> return status;
> }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index cf96f8d5f15f..69454a7ccc6f 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1172,8 +1172,6 @@ void efi_check_for_embedded_firmwares(void);
> static inline void efi_check_for_embedded_firmwares(void) { }
> #endif
>
> -efi_status_t efi_random_get_seed(void);
> -
> #define arch_efi_call_virt(p, f, args...) ((p)->f(args))
>
> /*
> --
> 2.35.1
>
