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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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.

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.

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
>> > > > > 
>> > > > > 
>> > >