efi: consume random seed provided by loader

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 <[email protected]> # v4.14+

Signed-off-by Ard Biesheuvel <[email protected]>

Reviewed-by Jason A. Donenfeld <[email protected]>

---
 drivers/firmware/efi/efi.c | 2 +-
 include/linux/efi.h        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/firmware/efi/efi.c +1 -1

index 9624735f1575..a949509de62f 100644
@@ -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");

include/linux/efi.h +1 -1

index da3974bf05d3..cf96f8d5f15f 100644
@@ -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;

Signature · 1 line

2.35.1

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 <[email protected]> # v4.14+

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/random.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/firmware/efi/libstub/random.c +1 -2

index 24aa37535372..183dc5cdb8ed 100644
@@ -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

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 <[email protected]>

drivers/firmware/efi/libstub/random.c +0 -0

 	efi_bs_call(free_pool, seed);
 	return status;
 }

include/linux/efi.h +0 -2

index cf96f8d5f15f..69454a7ccc6f 100644
@@ -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))
 
 /*

Signature · 1 line

2.35.1

Hi Ard,

Thanks for doing this. Some comments below:

Quoted 20 lines · 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:

Quoted 7 lines

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.

Quoted 2 lines

Signed-off-by Ard Biesheuvel <[email protected]>

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

drivers/firmware/efi/libstub/random.c efi_status_t efi_random_get_seed(void) · line 88

Quoted 10 lines

drivers/firmware/efi/libstub/random.c +10 -0

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

drivers/firmware/efi/libstub/random.c efi_status_t efi_random_get_seed(void) · line 113

Quoted 4 lines

drivers/firmware/efi/libstub/random.c +4 -0

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

Quoted 4 lines

+

      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.

Quoted 8 lines

drivers/firmware/efi/libstub/random.c +5 -0

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

drivers/firmware/efi/libstub/random.c efi_status_t efi_random_get_seed(void) · line 128

Quoted 3 lines

err_freepool:

drivers/firmware/efi/libstub/random.c +2 -0

+     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

Quoted 19 lines

      efi_bs_call(free_pool, seed);
      return status;

}

include/linux/efi.h +0 -3

index cf96f8d5f15f..69454a7ccc6f 100644
@@ -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

Quoted 34 lines · Jason A. Donenfeld

Hi Ard,

Thanks for doing this. Some comments below:

Quoted 27 lines · 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.

Quoted 4 lines

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?

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

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.

Quoted 5 lines · Ard Biesheuvel

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.

Quoted 8 lines · Jason A. Donenfeld

Quoted 5 lines · Ard Biesheuvel

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.

Quoted 3 lines

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?

Quoted 9 lines · Ard Biesheuvel

Quoted 8 lines · Jason A. Donenfeld

Quoted 5 lines · Ard Biesheuvel

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 15 lines · Jason A. Donenfeld

Quoted 13 lines · Ard Biesheuvel

Quoted 11 lines · Jason A. Donenfeld

Quoted 5 lines · Ard Biesheuvel

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.

Quoted 4 lines

Quoted 2 lines

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?