Re: [PATCH v3 3/3] efi: random: combine bootloader provided RNG seed with RNG protocol output

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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@kernel.org>

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
>