diff mbox series

[v3,2/5] efi: stub: use random seed from EFI variable

Message ID 20221122020404.3476063-3-Jason@zx2c4.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series Use EFI variables for random seed | expand

Commit Message

Jason A. Donenfeld Nov. 22, 2022, 2:04 a.m. UTC
EFI has a rather unique benefit that it has access to some limited
non-volatile storage, where the kernel can store a random seed. Read
that seed in EFISTUB and concatenate it with other seeds we wind up
passing onward to the kernel in the configuration table. This is
complementary to the current other two sources - previous bootloaders,
and the EFI RNG protocol.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/firmware/efi/libstub/random.c | 55 +++++++++++++++++++++------
 1 file changed, 43 insertions(+), 12 deletions(-)

Comments

Matthew Garrett Nov. 27, 2022, 9:12 p.m. UTC | #1
On Tue, Nov 22, 2022 at 03:04:01AM +0100, Jason A. Donenfeld wrote:

> +		 * We delete the seed here, and /hope/ that this causes EFI to
> +		 * also zero out its representation on disk. This is somewhat

Several implementations I've worked with simply append a deletion marker 
or append a new variable value until the variable store fills up 
entirely, at which point a garbage collection event is either run or 
scheduled for the next reboot. The spec doesn't define how this is 
handled so unfortunately I don't think there's any way to get a pony 
here.
Jason A. Donenfeld Nov. 28, 2022, 1:12 a.m. UTC | #2
Hi,

On Sun, Nov 27, 2022 at 09:12:44PM +0000, Matthew Garrett wrote:
> On Tue, Nov 22, 2022 at 03:04:01AM +0100, Jason A. Donenfeld wrote:
> 
> > +		 * We delete the seed here, and /hope/ that this causes EFI to
> > +		 * also zero out its representation on disk. This is somewhat
> 
> Several implementations I've worked with simply append a deletion marker 
> or append a new variable value until the variable store fills up 
> entirely, at which point a garbage collection event is either run or 
> scheduled for the next reboot. The spec doesn't define how this is 
> handled so unfortunately I don't think there's any way to get a pony 
> here.

Yea this is a bummer. During my first attempt at this, I actually
overwrote the whole thing with zeros and then deleted it. But Ard
pointed out that this doesn't make a difference anyway. But, as it turns
out, that's more or less the same thing that happens with seed files on
SSDs (nobody calls fstrim after overwriting a seed file). So at the very
least, it's no worse?

Jason
Matthew Garrett Nov. 28, 2022, 1:35 a.m. UTC | #3
On Mon, Nov 28, 2022 at 02:12:38AM +0100, Jason A. Donenfeld wrote:

> Yea this is a bummer. During my first attempt at this, I actually
> overwrote the whole thing with zeros and then deleted it. But Ard
> pointed out that this doesn't make a difference anyway. But, as it turns
> out, that's more or less the same thing that happens with seed files on
> SSDs (nobody calls fstrim after overwriting a seed file). So at the very
> least, it's no worse?

Anyone with the ability to directly read the flash variable store is 
almost certainly in a position to do worse things, so I wouldn't worry 
about it.
diff mbox series

Patch

diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index f85d2c066877..64aa6e7f3a17 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -68,13 +68,23 @@  efi_status_t efi_random_get_seed(void)
 	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;
+	u8 nv_seed[EFI_RANDOM_SEED_SIZE];
+	unsigned long prev_seed_size = 0, nv_seed_size = sizeof(nv_seed), seed_size = 0, offset = 0;
 	efi_rng_protocol_t *rng = NULL;
 	efi_status_t status;
 
 	status = efi_bs_call(locate_protocol, &rng_proto, NULL, (void **)&rng);
-	if (status != EFI_SUCCESS)
-		return status;
+	if (status == EFI_SUCCESS)
+		seed_size += EFI_RANDOM_SEED_SIZE;
+
+	status = get_efi_var(L"RandomSeed", &rng_table_guid, NULL, &nv_seed_size, nv_seed);
+	if (status == EFI_SUCCESS)
+		seed_size += nv_seed_size;
+	else
+		nv_seed_size = 0;
+
+	if (!seed_size)
+		return EFI_NOT_FOUND;
 
 	/*
 	 * Check whether a seed was provided by a prior boot stage. In that
@@ -83,7 +93,7 @@  efi_status_t efi_random_get_seed(void)
 	 * 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);
+	prev_seed = get_efi_config_table(rng_table_guid);
 	if (prev_seed && prev_seed->size <= 512U) {
 		prev_seed_size = prev_seed->size;
 		seed_size += prev_seed_size;
@@ -103,7 +113,7 @@  efi_status_t efi_random_get_seed(void)
 	}
 
 	status = efi_call_proto(rng, get_rng, &rng_algo_raw,
-				EFI_RANDOM_SEED_SIZE, seed->bits);
+				EFI_RANDOM_SEED_SIZE, seed->bits + offset);
 
 	if (status == EFI_UNSUPPORTED)
 		/*
@@ -111,16 +121,37 @@  efi_status_t efi_random_get_seed(void)
 		 * is not implemented.
 		 */
 		status = efi_call_proto(rng, get_rng, NULL,
-					EFI_RANDOM_SEED_SIZE, seed->bits);
+					EFI_RANDOM_SEED_SIZE, seed->bits + offset);
 
-	if (status != EFI_SUCCESS)
+	if (status == EFI_SUCCESS)
+		offset += EFI_RANDOM_SEED_SIZE;
+
+	if (nv_seed_size) {
+		memcpy(seed->bits + offset, nv_seed, nv_seed_size);
+		memzero_explicit(nv_seed, nv_seed_size);
+		/*
+		 * We delete the seed here, and /hope/ that this causes EFI to
+		 * also zero out its representation on disk. This is somewhat
+		 * idealistic, but overwriting the variable with zeros is
+		 * likely just as fraught too. TODO: in the future, maybe we
+		 * can hash it forward instead, and write a new seed.
+		 */
+		status = set_efi_var(L"RandomSeed", &rng_table_guid, 0, 0, NULL);
+		if (status == EFI_SUCCESS)
+			offset += nv_seed_size;
+		else
+			memzero_explicit(seed->bits + offset, nv_seed_size);
+	}
+
+	if (!offset)
 		goto err_freepool;
 
-	seed->size = seed_size;
-	if (prev_seed_size)
-		memcpy(seed->bits + EFI_RANDOM_SEED_SIZE, prev_seed->bits,
-		       prev_seed_size);
+	if (prev_seed_size) {
+		memcpy(seed->bits + offset, prev_seed->bits, prev_seed_size);
+		offset += prev_seed_size;
+	}
 
+	seed->size = offset;
 	status = efi_bs_call(install_configuration_table, &rng_table_guid, seed);
 	if (status != EFI_SUCCESS)
 		goto err_freepool;
@@ -135,7 +166,7 @@  efi_status_t efi_random_get_seed(void)
 err_freepool:
 	memzero_explicit(seed, struct_size(seed, bits, seed_size));
 	efi_bs_call(free_pool, seed);
-	efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL\n");
+	efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL and EFI variable\n");
 err_warn:
 	if (prev_seed)
 		efi_warn("Retaining bootloader-supplied seed only");