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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Sun, Jan 02, 2022 at 10:06:11PM +0100, Heinrich Schuchardt wrote:
> Am 2. Januar 2022 21:50:35 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
> >Hi Heinrich,
> >
> >> > > > > 
> >
> >[...]
> >
> >> > > > > 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);
> >> 
> >> This function should only be invoked for CONFIG_EFI_TCG2_PROTOCOL=y.
> >
> >Why?  As we discussed the kernel ignores the kaslr-seed for the
> >physical randomization.  The only reason we would like to keep it is 
> >for the randomization of the virtual address.  But if the EFI
> >RNG protocol is installed the EFI stub is already doing the right thing. 
> >So I really think purging it if EFI RNG is installed is the best option
> >here (regardless of TPM measurements)
> >
> 
> The only reason to delete kaslr-seed is that it conflicts with measured boot. If an OS prefers the RNG protocol over kaslr-seed is the decision of the OS and nothing U-Boot has to care about.
> 

I thought Mark said OpenBSD has a similar approach and prefers RNG over
kaslr-seed.  I pretty much agree with Ard here [1] for a cleaner way forward.

> You will have to delete kaslr-seed no matter if you have a RNG protocol or not if and only if you want to use measured boot.

But an RNG protocol seems to take priority over kaslr-seed so why keep it?
Also having a TPM (and measured boot) means you can *always* have an RNG
protocol installed.

[1] https://lore.kernel.org/u-boot/CAMj1kXGpnmKaZLzQ5LuHA=CqEm=2zjyu9Ri7TZxbM-tE3ZzAew@mail.gmail.com/


Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> >> > > > > +
> >> > > > >   	/* 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;
> >> 
> >> There is not need to check if the RNG protocol is installed. If
> >> CONFIG_EFI_TCG2_PROTOCOL=y, you should unconditionally remove
> >> 'kaslr-seed' as it is incompatible with measured boot.
> >
> >That's not entirely correct.  Right now having the kaslr-seed hurts no one,
> >since we don't measure the DTB to begin with.  What I intend to do is
> >expose the RNG hardware of the TPM and use that if the hardware doesn't
> >provide one already.  This obviously means the kaslr-seed will be removed 
> >because the RNG protocol will always be installed with the current patch.
> >
> >I really don't see a connection between a *compile* time option which
> >might not even have any effect if a TPM is not present, with an entry in
> >the /chosen node.  IMHO we should merge this patch since it improves
> >existing use cases.  I'll work on the rest and send patches soon.
> >
> >Cheers
> >/Ilias
> >
> >
> >> 
> >> Best regards
> >> 
> >> Heinrich
> >> 
> >> > > > > +	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
> >> > > > > 
> >> > > > > 
> >> > > 
>