[PATCH v3 0/3] efi: consume random seed provided by loader

[PATCH v3 0/3] efi: consume random seed provided by loader

[Ard Biesheuvel]

Getting a random seed into the kernel very early is important for data
structures that rely on the random number generator at initialization
time, but hardware based RNGs may not become available until much later
in the boot.

For this reason, the EFI stub will currently invoke the EFI RNG protocol
to get a random seed from the hardware before tearing down the EFI boot
services and performing the low level boot of the kernel proper. The
generated seed is passed via a EFI configuration table, which is
available very early, and so the random number generator comes up much
earlier as well.

Any boot stage preceding the EFI stub can install configuration tables,
so we can decide to expose the same mechanism to other loaders.  This
allows, e.g., systemd-boot to pass the seed it keeps in a file on the
ESP without having to rely on PID #1 dd'ing it into /dev/random, which
is much too late to be useful.

For maximum simplicity, just concatenate the existing seed with the one
obtained from EFI_RNG_PROTOCOL if both are available, and leave it to
the core kernel code to mix it in and credit it appropriately. This way,
we have no need for copies of the Blake2s library in the EFI stub and in
the zboot decompressor.

Older kernels will simply supersede the bootloader provided seed, unless
the RNG protocol is not available, in which case the bootloader seed
will be forwarded untouched if one is present. This should not be an
issue, but let's reduce the seed size to blake2's output size, and
switch to the correct memory type in separate changes so they can be
backported.

Changes since v2:
- rebase onto v6.1-rc1
- drop blake2s library from the stub - we'd need it in two places with
  the zboot changes landed, and concatenation is fine for our needs

Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Lennart Poettering <lennart@poettering.net>

Ard Biesheuvel (3):
  efi: random: reduce seed size to 32 bytes
  efi: random: Use 'ACPI reclaim' memory for random seed
  efi: random: combine bootloader provided RNG seed with RNG protocol
    output

 drivers/firmware/efi/efi.c             |  2 +-
 drivers/firmware/efi/libstub/efistub.h |  2 ++
 drivers/firmware/efi/libstub/random.c  | 31 +++++++++++++++++---
 include/linux/efi.h                    |  4 +--
 4 files changed, 31 insertions(+), 8 deletions(-)

-- 
2.35.1

[PATCH v3 1/3] efi: random: reduce seed size to 32 bytes

[Ard Biesheuvel]

We no longer need at least 64 bytes of random seed to permit the early
crng init to complete. The RNG is now based on Blake2s, so reduce the
EFI seed size to the Blake2s hash size, which is sufficient for our
purposes.

While at it, drop the READ_ONCE(), which was supposed to prevent size
from being evaluated after seed was unmapped. However, this cannot
actually happen, so READ_ONCE() is unnecessary here.

Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/firmware/efi/efi.c | 2 +-
 include/linux/efi.h        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 9624735f1575..a949509de62f 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -609,7 +609,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 		seed = early_memremap(efi_rng_seed, sizeof(*seed));
 		if (seed != NULL) {
-			size = READ_ONCE(seed->size);
+			size = min(seed->size, EFI_RANDOM_SEED_SIZE);
 			early_memunmap(seed, sizeof(*seed));
 		} else {
 			pr_err("Could not map UEFI random seed!\n");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index da3974bf05d3..cf96f8d5f15f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1225,7 +1225,7 @@ efi_status_t efi_random_get_seed(void);
 	arch_efi_call_virt_teardown();					\
 })
 
-#define EFI_RANDOM_SEED_SIZE		64U
+#define EFI_RANDOM_SEED_SIZE		32U // BLAKE2S_HASH_SIZE
 
 struct linux_efi_random_seed {
 	u32	size;
-- 
2.35.1

Re: [PATCH v3 1/3] efi: random: reduce seed size to 32 bytes

[Ilias Apalodimas]

On Thu, Oct 20, 2022 at 10:39:08AM +0200, Ard Biesheuvel wrote:
> We no longer need at least 64 bytes of random seed to permit the early
> crng init to complete. The RNG is now based on Blake2s, so reduce the
> EFI seed size to the Blake2s hash size, which is sufficient for our
> purposes.
> 
> While at it, drop the READ_ONCE(), which was supposed to prevent size
> from being evaluated after seed was unmapped. However, this cannot
> actually happen, so READ_ONCE() is unnecessary here.
> 
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/firmware/efi/efi.c | 2 +-
>  include/linux/efi.h        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 9624735f1575..a949509de62f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -609,7 +609,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>  
>  		seed = early_memremap(efi_rng_seed, sizeof(*seed));
>  		if (seed != NULL) {
> -			size = READ_ONCE(seed->size);
> +			size = min(seed->size, EFI_RANDOM_SEED_SIZE);
>  			early_memunmap(seed, sizeof(*seed));
>  		} else {
>  			pr_err("Could not map UEFI random seed!\n");
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index da3974bf05d3..cf96f8d5f15f 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1225,7 +1225,7 @@ efi_status_t efi_random_get_seed(void);
>  	arch_efi_call_virt_teardown();					\
>  })
>  
> -#define EFI_RANDOM_SEED_SIZE		64U
> +#define EFI_RANDOM_SEED_SIZE		32U // BLAKE2S_HASH_SIZE
>  
>  struct linux_efi_random_seed {
>  	u32	size;
> -- 
> 2.35.1
> 

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v3 2/3] efi: random: Use 'ACPI reclaim' memory for random seed

[Ard Biesheuvel]

EFI runtime services data is guaranteed to be preserved by the OS,
making it a suitable candidate for the EFI random seed table, which may
be passed to kexec kernels as well (after refreshing the seed), and so
we need to ensure that the memory is preserved without support from the
OS itself.

However, runtime services data is intended for allocations that are
relevant to the implementations of the runtime services themselves, and
so they are unmapped from the kernel linear map, and mapped into the EFI
page tables that are active while runtime service invocations are in
progress. None of this is needed for the RNG seed.

So let's switch to EFI 'ACPI reclaim' memory: in spite of the name,
there is nothing exclusively ACPI about it, it is simply a type of
allocation that carries firmware provided data which may or may not be
relevant to the OS, and it is left up to the OS to decide whether to
reclaim it after having consumed its contents.

Given that in Linux, we never reclaim these allocations, it is a good
choice for the EFI RNG seed, as the allocation is guaranteed to survive
kexec reboots.

One additional reason for changing this now is to align it with the
upcoming recommendation for EFI bootloader provided RNG seeds, which
must not use EFI runtime services code/data allocations.

Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/random.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 24aa37535372..183dc5cdb8ed 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -75,7 +75,7 @@ efi_status_t efi_random_get_seed(void)
 	if (status != EFI_SUCCESS)
 		return status;
 
-	status = efi_bs_call(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
+	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
 			     sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
 			     (void **)&seed);
 	if (status != EFI_SUCCESS)
-- 
2.35.1

Re: [PATCH v3 2/3] efi: random: Use 'ACPI reclaim' memory for random seed

[Ilias Apalodimas]

Hi Ard,

On Thu, 20 Oct 2022 at 11:40, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> EFI runtime services data is guaranteed to be preserved by the OS,
> making it a suitable candidate for the EFI random seed table, which may
> be passed to kexec kernels as well (after refreshing the seed), and so
> we need to ensure that the memory is preserved without support from the
> OS itself.
>
> However, runtime services data is intended for allocations that are
> relevant to the implementations of the runtime services themselves, and
> so they are unmapped from the kernel linear map, and mapped into the EFI
> page tables that are active while runtime service invocations are in
> progress. None of this is needed for the RNG seed.
>
> So let's switch to EFI 'ACPI reclaim' memory: in spite of the name,
> there is nothing exclusively ACPI about it, it is simply a type of
> allocation that carries firmware provided data which may or may not be
> relevant to the OS, and it is left up to the OS to decide whether to
> reclaim it after having consumed its contents.
>
> Given that in Linux, we never reclaim these allocations, it is a good
> choice for the EFI RNG seed, as the allocation is guaranteed to survive
> kexec reboots.

Can we add this as a comment right above the efi_bs_call()

>
> One additional reason for changing this now is to align it with the
> upcoming recommendation for EFI bootloader provided RNG seeds, which
> must not use EFI runtime services code/data allocations.
>
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/random.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
> index 24aa37535372..183dc5cdb8ed 100644
> --- a/drivers/firmware/efi/libstub/random.c
> +++ b/drivers/firmware/efi/libstub/random.c
> @@ -75,7 +75,7 @@ efi_status_t efi_random_get_seed(void)
>         if (status != EFI_SUCCESS)
>                 return status;
>
> -       status = efi_bs_call(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> +       status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
>                              sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
>                              (void **)&seed);
>         if (status != EFI_SUCCESS)
> --
> 2.35.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v3 3/3] efi: random: combine bootloader provided RNG seed with RNG protocol output

[Ard Biesheuvel]

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.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efistub.h |  2 ++
 drivers/firmware/efi/libstub/random.c  | 29 ++++++++++++++++++--
 include/linux/efi.h                    |  2 --
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index a30fb5d8ef05..75280b800eee 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -882,6 +882,8 @@ efi_status_t efi_get_random_bytes(unsigned long size, u8 *out);
 efi_status_t efi_random_alloc(unsigned long size, unsigned long align,
 			      unsigned long *addr, unsigned long random_seed);
 
+efi_status_t efi_random_get_seed(void);
+
 efi_status_t check_platform_features(void);
 
 void *get_efi_config_table(efi_guid_t guid);
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 183dc5cdb8ed..080012e837c3 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -67,16 +67,28 @@ efi_status_t efi_random_get_seed(void)
 	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
 	efi_guid_t rng_algo_raw = EFI_RNG_ALGORITHM_RAW;
 	efi_guid_t rng_table_guid = LINUX_EFI_RANDOM_SEED_TABLE_GUID;
+	struct linux_efi_random_seed *prev_seed, *seed = NULL;
+	int prev_seed_size, seed_size = EFI_RANDOM_SEED_SIZE;
 	efi_rng_protocol_t *rng = NULL;
-	struct linux_efi_random_seed *seed = NULL;
 	efi_status_t status;
 
 	status = efi_bs_call(locate_protocol, &rng_proto, NULL, (void **)&rng);
 	if (status != EFI_SUCCESS)
 		return status;
 
+	/*
+	 * Check whether a seed was provided by a prior boot stage. In that
+	 * case, instead of overwriting it, let's create a new buffer that can
+	 * hold both, and concatenate the existing and the new seeds.
+	 */
+	prev_seed = get_efi_config_table(LINUX_EFI_RANDOM_SEED_TABLE_GUID);
+	if (prev_seed) {
+		prev_seed_size = min(prev_seed->size, EFI_RANDOM_SEED_SIZE);
+		seed_size += prev_seed_size;
+	}
+
 	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
-			     sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
+			     struct_size(seed, bits, seed_size),
 			     (void **)&seed);
 	if (status != EFI_SUCCESS)
 		return status;
@@ -95,14 +107,25 @@ efi_status_t efi_random_get_seed(void)
 	if (status != EFI_SUCCESS)
 		goto err_freepool;
 
-	seed->size = EFI_RANDOM_SEED_SIZE;
+	seed->size = seed_size;
+	if (prev_seed)
+		memcpy(seed->bits + EFI_RANDOM_SEED_SIZE, prev_seed->bits,
+		       prev_seed_size);
+
 	status = efi_bs_call(install_configuration_table, &rng_table_guid, seed);
 	if (status != EFI_SUCCESS)
 		goto err_freepool;
 
+	if (prev_seed) {
+		/* wipe and free the old seed if we managed to install the new one */
+		memzero_explicit(prev_seed->bits, prev_seed_size);
+		efi_bs_call(free_pool, prev_seed);
+	}
 	return EFI_SUCCESS;
 
 err_freepool:
+	efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
+		 prev_seed ? ", retaining bootloader supplied seed only" : "");
 	efi_bs_call(free_pool, seed);
 	return status;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index cf96f8d5f15f..69454a7ccc6f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1172,8 +1172,6 @@ void efi_check_for_embedded_firmwares(void);
 static inline void efi_check_for_embedded_firmwares(void) { }
 #endif
 
-efi_status_t efi_random_get_seed(void);
-
 #define arch_efi_call_virt(p, f, args...)	((p)->f(args))
 
 /*
-- 
2.35.1

Re: [PATCH v3 3/3] efi: random: combine bootloader provided RNG seed with RNG protocol output

[Jason A. Donenfeld]

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.


> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

You can treat overwriting the old seed as an accidental bug, and Cc this
as stable like the rest of this series.

> +	/*
> +	 * Check whether a seed was provided by a prior boot stage. In that
> +	 * case, instead of overwriting it, let's create a new buffer that can
> +	 * hold both, and concatenate the existing and the new seeds.
> +	 */
> +	prev_seed = get_efi_config_table(LINUX_EFI_RANDOM_SEED_TABLE_GUID);
> +	if (prev_seed) {
> +		prev_seed_size = min(prev_seed->size, EFI_RANDOM_SEED_SIZE);
> +		seed_size += prev_seed_size;
> +	}

I think you can omit the min() here. If a bootloader passes a massive
seed such that allocations later fail, that's a bootloader problem, and
we should just complain loudly about it.

> +	seed->size = seed_size;
> +	if (prev_seed)
> +		memcpy(seed->bits + EFI_RANDOM_SEED_SIZE, prev_seed->bits,
> +		       prev_seed_size);

You memcpy here, but then:

> +
>  	status = efi_bs_call(install_configuration_table, &rng_table_guid, seed);
>  	if (status != EFI_SUCCESS)
>  		goto err_freepool;

If the installation here fails, you abort, leaving the memcpy'd seed in
memory without memzeroing.

>  
> +	if (prev_seed) {
> +		/* wipe and free the old seed if we managed to install the new one */
> +		memzero_explicit(prev_seed->bits, prev_seed_size);
> +		efi_bs_call(free_pool, prev_seed);
> +	}
>  	return EFI_SUCCESS;
>  
>  err_freepool:
> +	efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
> +		 prev_seed ? ", retaining bootloader supplied seed only" : "");

So maybe it makes sense to stick a memzero here too, just before
freeing?

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.
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. Instead, move the call to
locate_protocol down to just before the call to get_rng.

Jason

>  	efi_bs_call(free_pool, seed);
>  	return status;
>  }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index cf96f8d5f15f..69454a7ccc6f 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1172,8 +1172,6 @@ void efi_check_for_embedded_firmwares(void);
>  static inline void efi_check_for_embedded_firmwares(void) { }
>  #endif
>  
> -efi_status_t efi_random_get_seed(void);
> -
>  #define arch_efi_call_virt(p, f, args...)	((p)->f(args))
>  
>  /*
> -- 
> 2.35.1
> 

Re: [PATCH v3 3/3] efi: random: combine bootloader provided RNG seed with RNG protocol output

[Ard Biesheuvel]

On Thu, 20 Oct 2022 at 18:56, Jason A. Donenfeld <Jason@zx2c4.com> 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?

>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> You can treat overwriting the old seed as an accidental bug, and Cc this
> as stable like the rest of this series.
>
> > +     /*
> > +      * Check whether a seed was provided by a prior boot stage. In that
> > +      * case, instead of overwriting it, let's create a new buffer that can
> > +      * hold both, and concatenate the existing and the new seeds.
> > +      */
> > +     prev_seed = get_efi_config_table(LINUX_EFI_RANDOM_SEED_TABLE_GUID);
> > +     if (prev_seed) {
> > +             prev_seed_size = min(prev_seed->size, EFI_RANDOM_SEED_SIZE);
> > +             seed_size += prev_seed_size;
> > +     }
>
> I think you can omit the min() here. If a bootloader passes a massive
> seed such that allocations later fail, that's a bootloader problem, and
> we should just complain loudly about 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.

> > +     seed->size = seed_size;
> > +     if (prev_seed)
> > +             memcpy(seed->bits + EFI_RANDOM_SEED_SIZE, prev_seed->bits,
> > +                    prev_seed_size);
>
> You memcpy here, but then:
>
> > +
> >       status = efi_bs_call(install_configuration_table, &rng_table_guid, seed);
> >       if (status != EFI_SUCCESS)
> >               goto err_freepool;
>
> If the installation here fails, you abort, leaving the memcpy'd seed in
> memory without memzeroing.
>

Indeed.

> >
> > +     if (prev_seed) {
> > +             /* wipe and free the old seed if we managed to install the new one */
> > +             memzero_explicit(prev_seed->bits, prev_seed_size);
> > +             efi_bs_call(free_pool, prev_seed);
> > +     }
> >       return EFI_SUCCESS;
> >
> >  err_freepool:
> > +     efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
> > +              prev_seed ? ", retaining bootloader supplied seed only" : "");
>
> So maybe it makes sense to stick a memzero here too, just before
> freeing?
>

Yes, that makes sense.

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

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

> Instead, move the call to
> locate_protocol down to just before the call to get_rng.
>
> Jason
>
> >       efi_bs_call(free_pool, seed);
> >       return status;
> >  }
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index cf96f8d5f15f..69454a7ccc6f 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1172,8 +1172,6 @@ void efi_check_for_embedded_firmwares(void);
> >  static inline void efi_check_for_embedded_firmwares(void) { }
> >  #endif
> >
> > -efi_status_t efi_random_get_seed(void);
> > -
> >  #define arch_efi_call_virt(p, f, args...)    ((p)->f(args))
> >
> >  /*
> > --
> > 2.35.1
> >

Re: [PATCH v3 3/3] efi: random: combine bootloader provided RNG seed with RNG protocol output

[Jason A. Donenfeld]

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@zx2c4.com> 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

Re: [PATCH v3 0/3] efi: consume random seed provided by loader

[Jason A. Donenfeld]

On Thu, Oct 20, 2022 at 2:40 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> For maximum simplicity, just concatenate the existing seed with the one
> obtained from EFI_RNG_PROTOCOL if both are available, and leave it to
> the core kernel code to mix it in and credit it appropriately. This way,
> we have no need for copies of the Blake2s library in the EFI stub and in
> the zboot decompressor.

FTR, while I think this is okay for the final stage that the kernel's
EFI loader does, it's less good for earlier stages. So, for example,
systemd-boot should still use the hashing scheme we discussed.

Re: [PATCH v3 0/3] efi: consume random seed provided by loader

[Ard Biesheuvel]

On Thu, 20 Oct 2022 at 18:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Thu, Oct 20, 2022 at 2:40 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > For maximum simplicity, just concatenate the existing seed with the one
> > obtained from EFI_RNG_PROTOCOL if both are available, and leave it to
> > the core kernel code to mix it in and credit it appropriately. This way,
> > we have no need for copies of the Blake2s library in the EFI stub and in
> > the zboot decompressor.
>
> FTR, while I think this is okay for the final stage that the kernel's
> EFI loader does, it's less good for earlier stages. So, for example,
> systemd-boot should still use the hashing scheme we discussed.

Not sure I follow. systemd-boot will put a seed in memory and publish
it via the the table. How does hashing come into play here?

Re: [PATCH v3 0/3] efi: consume random seed provided by loader

[Jason A. Donenfeld]

On Thu, Oct 20, 2022 at 07:06:33PM +0200, Ard Biesheuvel wrote:
> On Thu, 20 Oct 2022 at 18:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 2:40 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > For maximum simplicity, just concatenate the existing seed with the one
> > > obtained from EFI_RNG_PROTOCOL if both are available, and leave it to
> > > the core kernel code to mix it in and credit it appropriately. This way,
> > > we have no need for copies of the Blake2s library in the EFI stub and in
> > > the zboot decompressor.
> >
> > FTR, while I think this is okay for the final stage that the kernel's
> > EFI loader does, it's less good for earlier stages. So, for example,
> > systemd-boot should still use the hashing scheme we discussed.
> 
> Not sure I follow. systemd-boot will put a seed in memory and publish
> it via the the table. How does hashing come into play here?

If systemd-boot is executed by another bootloader.

Re: [PATCH v3 0/3] efi: consume random seed provided by loader

[Ard Biesheuvel]

On Thu, 20 Oct 2022 at 19:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Thu, Oct 20, 2022 at 07:06:33PM +0200, Ard Biesheuvel wrote:
> > On Thu, 20 Oct 2022 at 18:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 2:40 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > For maximum simplicity, just concatenate the existing seed with the one
> > > > obtained from EFI_RNG_PROTOCOL if both are available, and leave it to
> > > > the core kernel code to mix it in and credit it appropriately. This way,
> > > > we have no need for copies of the Blake2s library in the EFI stub and in
> > > > the zboot decompressor.
> > >
> > > FTR, while I think this is okay for the final stage that the kernel's
> > > EFI loader does, it's less good for earlier stages. So, for example,
> > > systemd-boot should still use the hashing scheme we discussed.
> >
> > Not sure I follow. systemd-boot will put a seed in memory and publish
> > it via the the table. How does hashing come into play here?
>
> If systemd-boot is executed by another bootloader.

And that bootloader creates the same table, then systemd-boot does it, etc etc?

Re: [PATCH v3 0/3] efi: consume random seed provided by loader

[Jason A. Donenfeld]

On Thu, Oct 20, 2022 at 11:27 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 20 Oct 2022 at 19:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On Thu, Oct 20, 2022 at 07:06:33PM +0200, Ard Biesheuvel wrote:
> > > On Thu, 20 Oct 2022 at 18:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > >
> > > > On Thu, Oct 20, 2022 at 2:40 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > For maximum simplicity, just concatenate the existing seed with the one
> > > > > obtained from EFI_RNG_PROTOCOL if both are available, and leave it to
> > > > > the core kernel code to mix it in and credit it appropriately. This way,
> > > > > we have no need for copies of the Blake2s library in the EFI stub and in
> > > > > the zboot decompressor.
> > > >
> > > > FTR, while I think this is okay for the final stage that the kernel's
> > > > EFI loader does, it's less good for earlier stages. So, for example,
> > > > systemd-boot should still use the hashing scheme we discussed.
> > >
> > > Not sure I follow. systemd-boot will put a seed in memory and publish
> > > it via the the table. How does hashing come into play here?
> >
> > If systemd-boot is executed by another bootloader.
>
> And that bootloader creates the same table, then systemd-boot does it, etc etc?

Yea, the idea being all the bootloaders chain things forward by hashing.