This is a rough sketch of a proposal to use non-volatile EFI variables
as random seeds for EFISTUB to manage.
Patch 1 adds (back) the random.c async notifier, so we can learn when
the RNG is initialized.
Patch 2 uses it in vsprintf, because I promised Sebastian we'd do that
if it ever gets added back for whatever reason.
Patch 3 is already in efi.git and isn't new here, but is a pre-req for
the next patch.
Patch 4 uses the random seed from an EFI variable to pass to Linux.
Patch 5 prevents the variable from being read by efivarfs. [Note:
probably the legacy efifs needs updating too? Or has this been removed?]
Patch 6 uses patch 1 to refresh the EFI variable when the RNG is
initialized.
If folks like this idea and it moves forward, 1,2,6 will be taken into
my tree, and 3,4,5 will go via Ard's.
Commit messages are rather sparse at the moment. I'll fill those out for
the next non-RFC patchset if this idea isn't immediately demolished.
The biggest consideration is wear leveling on the EFI variable flash
chips. However, EFI *already* winds up writing to non-volatile memory on
every single boot anyway, so maybe it's not actually a big deal?
Thoughts?
Cc: Ard Biesheuvel <[email protected]>
Cc: Lennart Poettering <[email protected]>
Ard Biesheuvel (1):
efi: random: combine bootloader provided RNG seed with RNG protocol
output
Jason A. Donenfeld (5):
random: add back async readiness notifier
vsprintf: initialize siphash key using notifier
efi: stub: use random seed from EFI variable
efi: efivarfs: prohibit reading random seed variables
efi: refresh non-volatile random seed when RNG is initialized
drivers/char/random.c | 30 +++++++++
drivers/firmware/efi/efi.c | 14 +++++
drivers/firmware/efi/libstub/efistub.h | 2 +
drivers/firmware/efi/libstub/random.c | 85 +++++++++++++++++++++++---
fs/efivarfs/file.c | 3 +
include/linux/efi.h | 3 +-
include/linux/random.h | 1 +
lib/vsprintf.c | 14 ++---
8 files changed, 131 insertions(+), 21 deletions(-)
--
2.38.1
Since we have some storage, we can manage non-volatile seeds directly
from EFISTUB. This commit passes the contents of an EFI variable to the
kernel via the existing configuration table.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/firmware/efi/libstub/random.c | 59 +++++++++++++++++++++------
include/linux/efi.h | 1 +
2 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index f85d2c066877..1e72013a6457 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -67,14 +67,25 @@ 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;
+ efi_char16_t rng_nv_seed_var[] = LINUX_EFI_RANDOM_NV_SEED_VAR;
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(rng_nv_seed_var, &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 +94,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 +114,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 +122,40 @@ 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);
+ /*
+ * Zero it out before committing to using it. TODO: in the
+ * future, maybe we can hash it forward instead, which is
+ * better and also reduces the amount of writes here.
+ */
+ status = set_efi_var(rng_nv_seed_var, &rng_table_guid,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS, nv_seed_size, nv_seed);
+ if (status == EFI_SUCCESS)
+ status = set_efi_var(rng_nv_seed_var, &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 +170,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");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4aa1dbc7b064..0e93510b23b5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -408,6 +408,7 @@ void efi_native_runtime_setup(void);
#define LINUX_EFI_ARM_CPU_STATE_TABLE_GUID EFI_GUID(0xef79e4aa, 0x3c3d, 0x4989, 0xb9, 0x02, 0x07, 0xa9, 0x43, 0xe5, 0x50, 0xd2)
#define LINUX_EFI_LOADER_ENTRY_GUID EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf, 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
#define LINUX_EFI_RANDOM_SEED_TABLE_GUID EFI_GUID(0x1ce1e5bc, 0x7ceb, 0x42f2, 0x81, 0xe5, 0x8a, 0xad, 0xf1, 0x80, 0xf5, 0x7b)
+#define LINUX_EFI_RANDOM_NV_SEED_VAR L"RandomNVSeed"
#define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
#define LINUX_EFI_TPM_FINAL_LOG_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25)
#define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
--
2.38.1
Register a notifier so that when the RNG is initialized, the EFI
variable containing the random seed can be refreshed.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/firmware/efi/efi.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index a46df5d1d094..db247a52d3c2 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -337,6 +337,19 @@ static void __init efi_debugfs_init(void)
static inline void efi_debugfs_init(void) {}
#endif
+static int refresh_nv_rng_seed(struct notifier_block *nb, unsigned long action, void *data)
+{
+ u8 seed[EFI_RANDOM_SEED_SIZE];
+
+ get_random_bytes(seed, sizeof(seed));
+ efi.set_variable(LINUX_EFI_RANDOM_NV_SEED_VAR, &LINUX_EFI_RANDOM_SEED_TABLE_GUID,
+ EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS, sizeof(seed), seed);
+ memzero_explicit(seed, sizeof(seed));
+ return 0;
+}
+static struct notifier_block refresh_nv_rng_seed_nb = { .notifier_call = refresh_nv_rng_seed };
+
/*
* We register the efi subsystem with the firmware subsystem and the
* efivars subsystem with the efi subsystem, if the system was booted with
@@ -413,6 +426,7 @@ static int __init efisubsys_init(void)
platform_device_register_simple("efi_secret", 0, NULL, 0);
#endif
+ notify_on_rng_initialized(&refresh_nv_rng_seed_nb);
return 0;
err_remove_group:
--
2.38.1
From: Ard Biesheuvel <[email protected]>
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, and seeds larger than 512 bytes are considered
corrupted and ignored entirely.
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.
Signed-off-by: Ard Biesheuvel <[email protected]>
Reviewed-by: Jason A. Donenfeld <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/firmware/efi/libstub/efistub.h | 2 ++
drivers/firmware/efi/libstub/random.c | 42 ++++++++++++++++++++++----
include/linux/efi.h | 2 --
3 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index eb03d5a9aac8..900df67a2078 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 33ab56769595..f85d2c066877 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -67,27 +67,43 @@ 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;
+ if (status != EFI_SUCCESS) {
+ efi_warn("Failed to allocate memory for RNG seed.\n");
+ goto err_warn;
+ }
status = efi_call_proto(rng, get_rng, &rng_algo_raw,
- EFI_RANDOM_SEED_SIZE, seed->bits);
+ EFI_RANDOM_SEED_SIZE, seed->bits);
if (status == EFI_UNSUPPORTED)
/*
@@ -100,14 +116,28 @@ 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_size) {
+ /* 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:
+ 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");
+err_warn:
+ if (prev_seed)
+ efi_warn("Retaining bootloader-supplied seed only");
return status;
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7603fc58c47c..4aa1dbc7b064 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1170,8 +1170,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.38.1
Rather than polling every second, use the new notifier to do this at
exactly the right moment.
Cc: Mike Galbraith <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Petr Mladek <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
lib/vsprintf.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 24f37bab8bc1..70aa5de3c330 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -41,6 +41,7 @@
#include <linux/siphash.h>
#include <linux/compiler.h>
#include <linux/property.h>
+#include <linux/notifier.h>
#ifdef CONFIG_BLOCK
#include <linux/blkdev.h>
#endif
@@ -752,26 +753,21 @@ early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
static bool filled_random_ptr_key __read_mostly;
static siphash_key_t ptr_key __read_mostly;
-static void fill_ptr_key_workfn(struct work_struct *work);
-static DECLARE_DELAYED_WORK(fill_ptr_key_work, fill_ptr_key_workfn);
-static void fill_ptr_key_workfn(struct work_struct *work)
+static int fill_ptr_key(struct notifier_block *nb, unsigned long action, void *data)
{
- if (!rng_is_initialized()) {
- queue_delayed_work(system_unbound_wq, &fill_ptr_key_work, HZ * 2);
- return;
- }
-
get_random_bytes(&ptr_key, sizeof(ptr_key));
/* Pairs with smp_rmb() before reading ptr_key. */
smp_wmb();
WRITE_ONCE(filled_random_ptr_key, true);
+ return 0;
}
static int __init vsprintf_init_hashval(void)
{
- fill_ptr_key_workfn(NULL);
+ static struct notifier_block fill_ptr_key_nb = { .notifier_call = fill_ptr_key };
+ notify_on_rng_initialized(&fill_ptr_key_nb);
return 0;
}
subsys_initcall(vsprintf_init_hashval)
--
2.38.1
Variables in the random seed GUID must remain secret, so deny all reads
to them.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
fs/efivarfs/file.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index d57ee15874f9..08996ba3a373 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -76,6 +76,9 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
while (!__ratelimit(&file->f_cred->user->ratelimit))
msleep(50);
+ if (guid_equal(&var->var.VendorGuid, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
+ return -EPERM;
+
err = efivar_entry_size(var, &datasize);
/*
--
2.38.1
This is required by vsprint, because it can't do things synchronously
from hardirq context, and it will be useful for an EFI notifier as well.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/char/random.c | 30 ++++++++++++++++++++++++++++++
include/linux/random.h | 1 +
2 files changed, 31 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 6b7aca683b81..b3cad16ec567 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -84,6 +84,8 @@ static DEFINE_STATIC_KEY_FALSE(crng_is_ready);
/* Various types of waiters for crng_init->CRNG_READY transition. */
static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
static struct fasync_struct *fasync;
+static DEFINE_SPINLOCK(random_ready_chain_lock);
+static RAW_NOTIFIER_HEAD(random_ready_chain);
/* Control how we warn userspace. */
static struct ratelimit_state urandom_warning =
@@ -140,6 +142,33 @@ int wait_for_random_bytes(void)
}
EXPORT_SYMBOL(wait_for_random_bytes);
+/*
+ * Add a callback function that will be invoked when the crng is initialised,
+ * or immediately if it already has been.
+ */
+int __cold notify_on_rng_initialized(struct notifier_block *nb)
+{
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&random_ready_chain_lock, flags);
+ if (crng_ready())
+ nb->notifier_call(nb, 0, NULL);
+ else
+ ret = raw_notifier_chain_register(&random_ready_chain, nb);
+ spin_unlock_irqrestore(&random_ready_chain_lock, flags);
+ return ret;
+}
+
+static void __cold process_random_ready_list(void)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&random_ready_chain_lock, flags);
+ raw_notifier_call_chain(&random_ready_chain, 0, NULL);
+ spin_unlock_irqrestore(&random_ready_chain_lock, flags);
+}
+
#define warn_unseeded_randomness() \
if (IS_ENABLED(CONFIG_WARN_ALL_UNSEEDED_RANDOM) && !crng_ready()) \
printk_deferred(KERN_NOTICE "random: %s called from %pS with crng_init=%d\n", \
@@ -685,6 +714,7 @@ static void __cold _credit_init_bits(size_t bits)
crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */
if (static_key_initialized)
execute_in_process_context(crng_set_ready, &set_ready);
+ process_random_ready_list();
wake_up_interruptible(&crng_init_wait);
kill_fasync(&fasync, SIGIO, POLL_IN);
pr_notice("crng init done\n");
diff --git a/include/linux/random.h b/include/linux/random.h
index acaa328fb34d..566ffc3ab80d 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -119,6 +119,7 @@ void __init random_init_early(const char *command_line);
void __init random_init(void);
bool rng_is_initialized(void);
int wait_for_random_bytes(void);
+int notify_on_rng_initialized(struct notifier_block *nb);
/* Calls wait_for_random_bytes() and then calls get_random_bytes(buf, nbytes).
* Returns the result of the call to wait_for_random_bytes. */
--
2.38.1
On Wed, 16 Nov 2022 at 17:17, Jason A. Donenfeld <[email protected]> wrote:
>
> Variables in the random seed GUID must remain secret, so deny all reads
> to them.
>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
> fs/efivarfs/file.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> index d57ee15874f9..08996ba3a373 100644
> --- a/fs/efivarfs/file.c
> +++ b/fs/efivarfs/file.c
> @@ -76,6 +76,9 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> while (!__ratelimit(&file->f_cred->user->ratelimit))
> msleep(50);
>
> + if (guid_equal(&var->var.VendorGuid, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
> + return -EPERM;
> +
> err = efivar_entry_size(var, &datasize);
>
> /*
I'd prefer it if we could just disregard them entirely, i.e., never
enumerate them so that they don't appear in the file system.
On Mi, 16.11.22 17:16, Jason A. Donenfeld ([email protected]) wrote:
> Commit messages are rather sparse at the moment. I'll fill those out for
> the next non-RFC patchset if this idea isn't immediately demolished.
>
> The biggest consideration is wear leveling on the EFI variable flash
> chips. However, EFI *already* winds up writing to non-volatile memory on
> every single boot anyway, so maybe it's not actually a big deal?
So as mentioned elsewhere: This might (probably more than) double the
wear on the flash chips, since firmware is unlikely to batch these
writes with the monotonic counter write.
I have no idea how realistic these issues are, there's a lot of
handwaving involved, but to sidestep the issue I put sd-boot's seed in
a file on disk (which should not have issues that much with wear)
instead of efi vars.
Lennart
On Wed, Nov 16, 2022 at 6:59 PM Lennart Poettering
<[email protected]> wrote:
>
> On Mi, 16.11.22 17:16, Jason A. Donenfeld ([email protected]) wrote:
>
> > Commit messages are rather sparse at the moment. I'll fill those out for
> > the next non-RFC patchset if this idea isn't immediately demolished.
> >
> > The biggest consideration is wear leveling on the EFI variable flash
> > chips. However, EFI *already* winds up writing to non-volatile memory on
> > every single boot anyway, so maybe it's not actually a big deal?
>
> So as mentioned elsewhere: This might (probably more than) double the
> wear on the flash chips, since firmware is unlikely to batch these
> writes with the monotonic counter write.
>
> I have no idea how realistic these issues are, there's a lot of
> handwaving involved, but to sidestep the issue I put sd-boot's seed in
> a file on disk (which should not have issues that much with wear)
> instead of efi vars.
Therein lies the rub indeed. Does anybody who knows something about
the hardware and historical hardware know for certain that this would
be a bad idea, or does it really not matter at all? Would be useful to
have some definitive advice here.
Jason
On Wed, Nov 16, 2022 at 6:05 PM Ard Biesheuvel <[email protected]> wrote:
>
> On Wed, 16 Nov 2022 at 17:17, Jason A. Donenfeld <[email protected]> wrote:
> >
> > Variables in the random seed GUID must remain secret, so deny all reads
> > to them.
> >
> > Signed-off-by: Jason A. Donenfeld <[email protected]>
> > ---
> > fs/efivarfs/file.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> > index d57ee15874f9..08996ba3a373 100644
> > --- a/fs/efivarfs/file.c
> > +++ b/fs/efivarfs/file.c
> > @@ -76,6 +76,9 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> > while (!__ratelimit(&file->f_cred->user->ratelimit))
> > msleep(50);
> >
> > + if (guid_equal(&var->var.VendorGuid, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
> > + return -EPERM;
> > +
> > err = efivar_entry_size(var, &datasize);
> >
> > /*
>
> I'd prefer it if we could just disregard them entirely, i.e., never
> enumerate them so that they don't appear in the file system.
Okay, sure, I can make that happen I think.
And then I suppose that if you try to create anything under that GUID,
it should just fail.
Jason
On Wed, 2022-11-16 at 18:04 +0100, Ard Biesheuvel wrote:
> On Wed, 16 Nov 2022 at 17:17, Jason A. Donenfeld <[email protected]>
> wrote:
> >
> > Variables in the random seed GUID must remain secret, so deny all
> > reads
> > to them.
> >
> > Signed-off-by: Jason A. Donenfeld <[email protected]>
> > ---
> > fs/efivarfs/file.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
> > index d57ee15874f9..08996ba3a373 100644
> > --- a/fs/efivarfs/file.c
> > +++ b/fs/efivarfs/file.c
> > @@ -76,6 +76,9 @@ static ssize_t efivarfs_file_read(struct file
> > *file, char __user *userbuf,
> > while (!__ratelimit(&file->f_cred->user->ratelimit))
> > msleep(50);
> >
> > + if (guid_equal(&var->var.VendorGuid,
> > &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
> > + return -EPERM;
> > +
> > err = efivar_entry_size(var, &datasize);
> >
> > /*
>
> I'd prefer it if we could just disregard them entirely, i.e., never
> enumerate them so that they don't appear in the file system.
It would be nice if they could be boot services only ... then they
disappear naturally, but that would mean the rng would have to
initialize and save in the EFI stub before ExitBootServices, which
doesn't seem practical.
James
On Wed, Nov 16, 2022 at 8:42 PM James Bottomley
<[email protected]> wrote:
> It would be nice if they could be boot services only ... then they
> disappear naturally, but that would mean the rng would have to
> initialize and save in the EFI stub before ExitBootServices, which
> doesn't seem practical.
That would be nice, but the whole idea is it gets updated by Linux's
RNG, so that won't work. `boot|runtime` it is, then.
Jason
On Wed 2022-11-16 17:16:38, Jason A. Donenfeld wrote:
> Rather than polling every second, use the new notifier to do this at
> exactly the right moment.
Great news!
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -752,26 +753,21 @@ early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
>
> static bool filled_random_ptr_key __read_mostly;
> static siphash_key_t ptr_key __read_mostly;
> -static void fill_ptr_key_workfn(struct work_struct *work);
> -static DECLARE_DELAYED_WORK(fill_ptr_key_work, fill_ptr_key_workfn);
>
> -static void fill_ptr_key_workfn(struct work_struct *work)
> +static int fill_ptr_key(struct notifier_block *nb, unsigned long action, void *data)
> {
> - if (!rng_is_initialized()) {
> - queue_delayed_work(system_unbound_wq, &fill_ptr_key_work, HZ * 2);
> - return;
> - }
> -
> get_random_bytes(&ptr_key, sizeof(ptr_key));
>
> /* Pairs with smp_rmb() before reading ptr_key. */
> smp_wmb();
> WRITE_ONCE(filled_random_ptr_key, true);
> + return 0;
I believe that we should rather return NOTIFY_DONE here.
It is rather a formal change. The value is 0 as well.
That said, I have never really understood the difference between
NOTIFY_OK and NOTIFY_DONE.
> }
>
> static int __init vsprintf_init_hashval(void)
> {
> - fill_ptr_key_workfn(NULL);
> + static struct notifier_block fill_ptr_key_nb = { .notifier_call = fill_ptr_key };
> + notify_on_rng_initialized(&fill_ptr_key_nb);
> return 0;
> }
> subsys_initcall(vsprintf_init_hashval)
Anyway, the code looks good to me:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Fri, Nov 18, 2022 at 3:16 PM Petr Mladek <[email protected]> wrote:
> > + return 0;
>
> I believe that we should rather return NOTIFY_DONE here.
> It is rather a formal change. The value is 0 as well.
>
> That said, I have never really understood the difference between
> NOTIFY_OK and NOTIFY_DONE.
Ah yes, the varying degrees of apathy:
#define NOTIFY_DONE 0x0000 /* Don't care */
#define NOTIFY_OK 0x0001 /* Suits me */
In a sense, the fact that there's this return value at all indicates a
notifier block isn't *quite* the API we want, since this happens only
once and it really should never stop. But it's so convenient and small
to use that I think it's fine. Anyway, I'll use the right constant
here as you suggested.
Jason
On Wed, 2022-11-16 at 21:08 +0100, Jason A. Donenfeld wrote:
> On Wed, Nov 16, 2022 at 8:42 PM James Bottomley
> <[email protected]> wrote:
> > It would be nice if they could be boot services only ... then they
> > disappear naturally, but that would mean the rng would have to
> > initialize and save in the EFI stub before ExitBootServices, which
> > doesn't seem practical.
>
> That would be nice, but the whole idea is it gets updated by Linux's
> RNG, so that won't work. `boot|runtime` it is, then.
But then you can't use the only security mechanism we have in EFI
(keeping sensitive information in BS only variables which can only be
accessed by EFI signed entities). If you can't take advantage of that
then there's no security point in placing the seed in EFI and you might
as well simply write it to a file.
Artificially trying to hide the variables from efivarfs has no real
security value either, as I think you can appreciate if you try the
thought experiment of trying to get a VFS modification to hide the
random seed file past Al ... I'll get the thought experiment popcorn.
James