On Thu, Oct 20, 2022 at 07:11:33PM +0200, Ard Biesheuvel wrote:
> On Thu, 20 Oct 2022 at 18:56, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
> >
> > 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.
> >
>
> Good point. Did you have any prose in mind?
How about:
In order to preserve forward secrecy, seeds from previous bootloaders
are memzero'd out, and in order to preserve memory, those older seeds
are also freed from memory. Freeing from memory without first memzeroing
is not safe to do, as it's possible that nothing else will ever
overwrite those pages used by EFI.
Or something like it?
> I'm more worried about memory corruption here. If the memory that the
> table points to was overwritten arbitrarily, the size field will be
> bogus, and memcpy()ing that number of bytes is likely to lead to
> tears.
Gotcha. Maybe cap it to 512 bytes then? Something overly big, but not
too large? That's what I do in arch/mips/kernel/setup.c's
setup_rng_seed, for example.
> > 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.
>
> We don't hit the warn in that case. If EFI_RNG_PROTOCOL does not
> exist, we just bail and if the bootloader provided a seed as well, it
> just remains where it was.
Oh yea, good point. So that warning message triggers for when there is a
EFI_RNG_PROTOCOL, but for some reason it still fails when using it.
> > 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.
>
> Why? The bootloader seed will just be passed on in that case - no need
> to touch it if we're not going to combine it with anything else.
You're right.
Jason
