[PATCH v4] efi: random: combine bootloader provided RNG seed with RNG protocol output

[PATCH v4] 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.

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.

Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Lennart Poettering <lennart@poettering.net>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
v4:
- expand commit log
- memzero new seed if we failed to install it - the old seed will be
  passed on unmodified in that case
- bump the prev seed size sanity check to 512 bytes, and ignore it
  entirely if the size exceeds that (which likely means the table was
  corrupted somehow)
- drop patches 1/3 and 2/3, which have been merged into mainline as
  fixes

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

diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 576e5cfec9804b04..ce10b7bbd3f71d54 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -933,6 +933,8 @@ efi_status_t efi_random_alloc(unsigned long size, unsigned long align,
 			      unsigned long *addr, unsigned long random_seed,
 			      int memory_type);
 
+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 33ab567695951d6c..370a93ada948c6c1 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -67,21 +67,35 @@ 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 = 0, 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.
+	 * Note that we should read the seed size with caution, in case the
+	 * table got corrupted in memory somehow.
+	 */
+	prev_seed = get_efi_config_table(LINUX_EFI_RANDOM_SEED_TABLE_GUID);
+	if (prev_seed && prev_seed->size <= 512U) {
+		prev_seed_size = prev_seed->size;
+		seed_size += prev_seed_size;
+	}
+
 	/*
 	 * Use EFI_ACPI_RECLAIM_MEMORY here so that it is guaranteed that the
 	 * allocation will survive a kexec reboot (although we refresh the seed
 	 * beforehand)
 	 */
 	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;
@@ -100,14 +114,26 @@ 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_size)
+		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" : "");
+	memzero_explicit(seed, struct_size(seed, bits, seed_size));
 	efi_bs_call(free_pool, seed);
 	return status;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 60c73fc840a10c8d..1109f9d43b8e7fd4 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1132,8 +1132,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 v4] efi: random: combine bootloader provided RNG seed with RNG protocol output

[Jason A. Donenfeld]

Hi Ard,

Looking good! Thanks. A few brief comments.

On Wed, Nov 09, 2022 at 10:55:58AM +0100, Ard Biesheuvel wrote:
> The recommended seed
> size is 32 bytes, anything beyond that is disregarded when the seeds are
> concatenated.

This should read, "The recommended seed size is 32 bytes, and seeds
larger than 512 bytes are considered corrupted and ignored entirely."

> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Lennart Poettering <lennart@poettering.net>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

I was thinking it might be best to add:

   Cc: stable@vger.kernel.org

"Don't clobber existing GUID" seems like it's arguably a fix.

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

Should this print "Failed to something something, retaining
bootloader-supplied seed only", like the err_freepool case?

> +	efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
> +		 prev_seed ? ", retaining bootloader supplied seed only" : "");

"bootloader-supplied" with the hyphen, right?

If you make any of the above changes, feel free to do it when you commit
this. IOW, my comments are nits, so:

    Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>

Also, I verified that kexec correctly sets everything back to 32 bytes
with this line:
    size = min(seed->size, EFI_RANDOM_SEED_SIZE);
So that's good.

Jason

Re: [PATCH v4] efi: random: combine bootloader provided RNG seed with RNG protocol output

[Ard Biesheuvel]

On Wed, 9 Nov 2022 at 11:34, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> Looking good! Thanks. A few brief comments.
>
> On Wed, Nov 09, 2022 at 10:55:58AM +0100, Ard Biesheuvel wrote:
> > The recommended seed
> > size is 32 bytes, anything beyond that is disregarded when the seeds are
> > concatenated.
>
> This should read, "The recommended seed size is 32 bytes, and seeds
> larger than 512 bytes are considered corrupted and ignored entirely."
>

Ack.

> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> > Cc: Lennart Poettering <lennart@poettering.net>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> I was thinking it might be best to add:
>
>    Cc: stable@vger.kernel.org
>
> "Don't clobber existing GUID" seems like it's arguably a fix.
>

Yeah, I'm on the fence about that one. I'll leave it out for now, and
we can always get it into -stable later. I am not going to queue it as
a fix now in any case.


> >       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;
>
> Should this print "Failed to something something, retaining
> bootloader-supplied seed only", like the err_freepool case?
>

Yeah, it's just something that is exceedingly unlikely to ever occur
in practice, and if it does, whether or not the efi_warn() will
produce any observable output is anyone's guess.

But i'll add it just for good measure.

> > +     efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
> > +              prev_seed ? ", retaining bootloader supplied seed only" : "");
>
> "bootloader-supplied" with the hyphen, right?
>

Ack.

> If you make any of the above changes, feel free to do it when you commit
> this. IOW, my comments are nits, so:
>
>     Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
>

Thanks.

> Also, I verified that kexec correctly sets everything back to 32 bytes
> with this line:
>     size = min(seed->size, EFI_RANDOM_SEED_SIZE);
> So that's good.
>
> Jason