Re: [PATCH v2] efi_loader: Get rid of kaslr-seed

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Fri, 17 Dec 2021 13:23:59 +0200
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 
> Hi Mark, 
> 
> On Fri, Dec 17, 2021 at 12:13:20PM +0100, Mark Kettenis wrote:
> > > From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Date: Fri, 17 Dec 2021 09:06:44 +0200
> > > 
> > > Right now we unconditionally pass a 'kaslr-seed' property to the kernel
> > > if the DTB we ended up in EFI includes the entry.  However the kernel
> > > EFI stub completely ignores it and only relies on EFI_RNG_PROTOCOL for
> > > it's own randomness needs (i.e the randomization of the physical
> > > placement of the kernel).
> > > So let's get rid of it if EFI_RNG_PPROTOCOL is installed.
> > > 
> > > It's worth noting that TPMs also provide an RNG.  So if we tweak our
> > > EFI_RNG_PROTOCOL slightly and install the protocol when a TPM device
> > > is present the 'kaslr-seed' property will always be removed, allowing
> > > us to reliably measure our DTB as well.
> > 
> > That looks good to me.  But I think you should be honest here.  You're
> > removing the kaslr-seed property here because it is a part of the DTB
> > that changes on every boot, which interferes with the measurability.
> > Not because there is something wrong with passing along the
> > kaslr-seed.  
> 
> Yep never tried to hide that :)
> 
> > In the absence of EFI_RNG_PROTOCOL support passing
> > kaslr-seed still has a benefit as it will still be used for
> > randomization of the virtual placement of the kernel.  So I think the
> > comment could use some rewording (but the bit on randomization of the
> > physical placement is certainly accurate and worth keeping).
> > 
> > Oh, and please say "The Linux kernel's EFI STUB".
> 
> Sure something along the lines of:
> 
> "Right now we unconditionally keep a 'kaslr-seed' property in the DTB,
> if what we ended up in EFI includes the entry.  However the Linux kernel
> EFI-stub completely ignores it and only relies on EFI_RNG_PROTOCOL for
> it's own randomness needs (i.e the randomization of the physical
> placement of the kernel).  In fact it (blindly) overwrites the existing 
> seed if the EFI_RNG_PROTOCOL protocol is installed.  However it still uses
> it for randomization of the virtual placement of the kernel if the EFI
> protocol is not installed. 
> So let's get rid of it if EFI_RNG_PPROTOCOL is installed.
> 
> It's worth noting that TPMs also provide an RNG.  So if we tweak our
> EFI_RNG_PROTOCOL slightly and install the protocol when a TPM device
> is present the 'kaslr-seed' property will always be removed, allowing
> us to reliably measure our DTB as well."

I would go in the other direction.  I'd explain why you want to remove
kaslr-seed first (Property changes every boot; interferes with
measured boot) and then explain why removing it in the presence of
EFI_RNG_PROTOCOL support is fine (Linux kernel EFI STUB doesn't use
for randomizing physical placement; overwrites it if EFI_RNG_PROTOCOL
is implemented).

I think describing it that way will prevent confusion in the future.

Cheers,

Mark

> > Cheers,
> > 
> > Mark
> > 
> > > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > changes since v1:
> > > - Only removing the property if EFI_RNG_PROTOCOL is installed, since some
> > >   OS'es rely on kaslr-seed
> > > - Only display an error message if the kaslr-seed entry was found but not
> > >   removed
> > >  cmd/bootefi.c                 |  2 ++
> > >  include/efi_loader.h          |  2 ++
> > >  lib/efi_loader/efi_dt_fixup.c | 33 +++++++++++++++++++++++++++++++++
> > >  3 files changed, 37 insertions(+)
> > > 
> > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > index d77d3b6e943d..57f13ce701ec 100644
> > > --- a/cmd/bootefi.c
> > > +++ b/cmd/bootefi.c
> > > @@ -310,6 +310,8 @@ efi_status_t efi_install_fdt(void *fdt)
> > >  	/* Create memory reservations as indicated by the device tree */
> > >  	efi_carve_out_dt_rsv(fdt);
> > >  
> > > +	efi_try_purge_kaslr_seed(fdt);
> > > +
> > >  	/* Install device tree as UEFI table */
> > >  	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> > >  	if (ret != EFI_SUCCESS) {
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 9dd6c2033634..1fe003db69e0 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -519,6 +519,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
> > >  					void **address);
> > >  /* Carve out DT reserved memory ranges */
> > >  void efi_carve_out_dt_rsv(void *fdt);
> > > +/* Purge unused kaslr-seed */
> > > +void efi_try_purge_kaslr_seed(void *fdt);
> > >  /* Called by bootefi to make console interface available */
> > >  efi_status_t efi_console_register(void);
> > >  /* Called by bootefi to make all disk storage accessible as EFI objects */
> > > diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c
> > > index b6fe5d2e5a34..d3923e5dba1b 100644
> > > --- a/lib/efi_loader/efi_dt_fixup.c
> > > +++ b/lib/efi_loader/efi_dt_fixup.c
> > > @@ -8,6 +8,7 @@
> > >  #include <common.h>
> > >  #include <efi_dt_fixup.h>
> > >  #include <efi_loader.h>
> > > +#include <efi_rng.h>
> > >  #include <fdtdec.h>
> > >  #include <mapmem.h>
> > >  
> > > @@ -40,6 +41,38 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
> > >  			addr, size);
> > >  }
> > >  
> > > +/**
> > > + * efi_try_purge_kaslr_seed() - Remove unused kaslr-seed
> > > + *
> > > + * Kernel's EFI STUB only relies on EFI_RNG_PROTOCOL for randomization
> > > + * and completely ignores the kaslr-seed for its own randomness needs
> > > + * (i.e the randomization of the physical placement of the kernel).
> > > + * Weed it out from the DTB we hand over, which would mess up our DTB
> > > + * TPM measurements as well.
> > > + *
> > > + * @fdt: Pointer to device tree
> > > + */
> > > +void efi_try_purge_kaslr_seed(void *fdt)
> > > +{
> > > +	const efi_guid_t efi_guid_rng_protocol = EFI_RNG_PROTOCOL_GUID;
> > > +	struct efi_handler *handler;
> > > +	efi_status_t ret;
> > > +	int nodeoff = 0;
> > > +	int err = 0;
> > > +
> > > +	ret = efi_search_protocol(efi_root, &efi_guid_rng_protocol, &handler);
> > > +	if (ret != EFI_SUCCESS)
> > > +		return;
> > > +
> > > +	nodeoff = fdt_path_offset(fdt, "/chosen");
> > > +	if (nodeoff < 0)
> > > +		return;
> > > +
> > > +	err = fdt_delprop(fdt, nodeoff, "kaslr-seed");
> > > +	if (err < 0 && err != -FDT_ERR_NOTFOUND)
> > > +		log_err("Error deleting kaslr-seed\n");
> > > +}
> > > +
> > >  /**
> > >   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
> > >   *
> > > -- 
> > > 2.30.2
> > > 
> > > 
>