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

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

[Ilias Apalodimas]

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.

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

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

[Heinrich Schuchardt]

On 12/17/21 08:06, Ilias Apalodimas wrote:
> 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.
>
> 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

Each TPMv2 provides a hardware RNG. So you can unconditionally remove
the kaslr-seed and create a new one by calling TPM2_GetRandom().

It would further be useful to provide a DM RNG driver using
TPM2_GetRandom().

Best regards

Heinrich

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

[Ilias Apalodimas]

Hi Heinrich,


On Fri, 17 Dec 2021 at 12:59, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/17/21 08:06, Ilias Apalodimas wrote:
> > 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.
> >
> > 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
>
> Each TPMv2 provides a hardware RNG. So you can unconditionally remove
> the kaslr-seed and create a new one by calling TPM2_GetRandom().
>
> It would further be useful to provide a DM RNG driver using
> TPM2_GetRandom().

Yes it would, but that's orthogonal to this patch. I did look at this
and there's a bit of plumbing still missing, which I'll fix when I
post the series for measuring a DTB.

Cheers
/Ilias
>
> Best regards
>
> Heinrich

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

[Mark Kettenis]

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

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

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

[Ilias Apalodimas]

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

Cheers
/Ilias
> 
> 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
> > 
> > 

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

[Mark Kettenis]

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

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

[Heinrich Schuchardt]

On 12/17/21 12:33, Mark Kettenis wrote:
>> 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);

This function should only be invoked for CONFIG_EFI_TCG2_PROTOCOL=y.

>>>> +
>>>>   	/* 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.

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

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

[Ilias Apalodimas]

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)

> > > > > +
> > > > >   	/* 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
> > > > > 
> > > > > 
> > > 

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

[Heinrich Schuchardt]

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

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

[Mark Kettenis]

> Date: Sun, 02 Jan 2022 22:06:11 +0100
> From: 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.

But it is also up to the OS to decide whether it cares about measured
boot or not.
 
> 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.

So you shouldn't just unconditionally delete kaslr-seed if the
CONFIG_EFI_TCG2_PROTOCOL has been enabled, but only if it is actually
used...  But is that even possible?

So maybe you should just specify the certain properties (such as
kaslr-seed) should be skipped when calculating the hash of the device
tree.

> >> > > > > +
> >> > > > >   	/* 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
> >> > > > > 
> >> > > > > 
> >> > > 
> 
> 

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

[Ilias Apalodimas]

Hi Mark, 

On Sun, Jan 02, 2022 at 10:27:50PM +0100, Mark Kettenis wrote:
> > Date: Sun, 02 Jan 2022 22:06:11 +0100
> > From: 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.
> 
> But it is also up to the OS to decide whether it cares about measured
> boot or not.
>  
> > 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.
> 
> So you shouldn't just unconditionally delete kaslr-seed if the
> CONFIG_EFI_TCG2_PROTOCOL has been enabled, but only if it is actually
> used...  But is that even possible?

I can't think of any (sane) way to figure this out.

> 
> So maybe you should just specify the certain properties (such as
> kaslr-seed) should be skipped when calculating the hash of the device
> tree.
> 

We could, but I'd like to avoid that unless we don't have a better
alternative.

Thanks
/Ilias

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

[Ilias Apalodimas]

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