diff mbox series

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

Message ID 20221020083910.1902009-4-ardb@kernel.org
State New
Headers show
Series efi: consume random seed provided by loader | expand

Commit Message

Ard Biesheuvel Oct. 20, 2022, 8:39 a.m. UTC 
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(-)

Comments

Jason A. Donenfeld Oct. 20, 2022, 4:56 p.m. UTC | #1 
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
>
Ard Biesheuvel Oct. 20, 2022, 5:11 p.m. UTC | #2 
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
> >
Jason A. Donenfeld Oct. 20, 2022, 5:22 p.m. UTC | #3 
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
diff mbox series

Patch

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