EFI has a rather unique benefit that it has access to some limited
non-volatile storage, where the kernel can store a random seed. This
series wires that up, with EFISTUB reading the seed and passing it to
the kernel, and with the kernel writing a new seed when the RNG is
initialized.
Patches 1 and 2 are to go through Ard's EFI tree, while patches 3, 4,
and 5 are to go through my RNG tree.
Jason A. Donenfeld (5):
efi: vars: prohibit reading random seed variables
efi: stub: use random seed from EFI variable
random: add back async readiness notifier
vsprintf: initialize siphash key using notifier
efi: random: refresh non-volatile random seed when RNG is initialized
drivers/char/random.c | 22 +++++++++++
drivers/firmware/efi/efi.c | 19 +++++++++
drivers/firmware/efi/libstub/random.c | 55 +++++++++++++++++++++------
fs/efivarfs/inode.c | 4 ++
fs/efivarfs/super.c | 3 ++
include/linux/random.h | 1 +
lib/vsprintf.c | 14 +++----
7 files changed, 97 insertions(+), 21 deletions(-)
--
2.38.1
In anticipation of putting random seeds in EFI variables, it's important
that the random GUID namespace of variables remains hidden from
userspace. We accomplish this by not populating efivarfs with entries
from that GUID, as well as denying the creation of new ones in that
GUID.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
fs/efivarfs/inode.c | 4 ++++
fs/efivarfs/super.c | 3 +++
2 files changed, 7 insertions(+)
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c
index 939e5e242b98..617f3ad2485e 100644
--- a/fs/efivarfs/inode.c
+++ b/fs/efivarfs/inode.c
@@ -91,6 +91,10 @@ static int efivarfs_create(struct user_namespace *mnt_userns, struct inode *dir,
err = guid_parse(dentry->d_name.name + namelen + 1, &var->var.VendorGuid);
if (err)
goto out;
+ if (guid_equal(&var->var.VendorGuid, &LINUX_EFI_RANDOM_SEED_TABLE_GUID)) {
+ err = -EPERM;
+ goto out;
+ }
if (efivar_variable_is_removable(var->var.VendorGuid,
dentry->d_name.name, namelen))
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 6780fc81cc11..07e82e246666 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -116,6 +116,9 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
int err = -ENOMEM;
bool is_removable = false;
+ if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
+ return 0;
+
entry = kzalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
return err;
--
2.38.1
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 <[email protected]>
---
drivers/firmware/efi/libstub/random.c | 55 +++++++++++++++++++++------
1 file changed, 43 insertions(+), 12 deletions(-)
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");
--
2.38.1
Rather than polling every second, use the new notifier to do this at
exactly the right moment.
Reviewed-by: 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..2d11541ee561 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 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 };
+ execute_with_initialized_rng(&fill_ptr_key_nb);
return 0;
}
subsys_initcall(vsprintf_init_hashval)
--
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.
I didn't initially want to do this, but with two potential consumers
now, it seems worth it.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/char/random.c | 22 ++++++++++++++++++++++
include/linux/random.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 65ee69896967..a2a18bd3d7d7 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -84,6 +84,7 @@ 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 ATOMIC_NOTIFIER_HEAD(random_ready_notifier);
/* Control how we warn userspace. */
static struct ratelimit_state urandom_warning =
@@ -140,6 +141,26 @@ 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. Only use this is you are absolutely
+ * sure it is required. Most users should instead be able to test
+ * `rng_is_initialized()` on demand, or make use of `get_random_bytes_wait()`.
+ */
+int __cold execute_with_initialized_rng(struct notifier_block *nb)
+{
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&random_ready_notifier.lock, flags);
+ if (crng_ready())
+ nb->notifier_call(nb, 0, NULL);
+ else
+ ret = raw_notifier_chain_register((struct raw_notifier_head *)&random_ready_notifier.head, nb);
+ spin_unlock_irqrestore(&random_ready_notifier.lock, flags);
+ return ret;
+}
+
#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", \
@@ -697,6 +718,7 @@ static void __cold _credit_init_bits(size_t bits)
crng_reseed(NULL); /* Sets crng_init to CRNG_READY under base_crng.lock. */
if (static_key_initialized)
execute_in_process_context(crng_set_ready, &set_ready);
+ atomic_notifier_call_chain(&random_ready_notifier, 0, NULL);
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 579117d83eb8..b1a34181eed6 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -120,6 +120,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 execute_with_initialized_rng(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
EFI has a rather unique benefit that it has access to some limited
non-volatile storage, where the kernel can store a random seed. Register
a notification for when the RNG is initialized, and at that point, store
a new random seed.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/firmware/efi/efi.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index f12cc29bd4b8..b23ec97d68ea 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -337,6 +337,24 @@ static void __init efi_debugfs_init(void)
static inline void efi_debugfs_init(void) {}
#endif
+static void refresh_nv_rng_seed(struct work_struct *work)
+{
+ u8 seed[EFI_RANDOM_SEED_SIZE];
+
+ get_random_bytes(seed, sizeof(seed));
+ efi.set_variable(L"RandomSeed", &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));
+}
+static int refresh_nv_rng_seed_notification(struct notifier_block *nb, unsigned long action, void *data)
+{
+ static DECLARE_WORK(work, refresh_nv_rng_seed);
+ schedule_work(&work);
+ return NOTIFY_DONE;
+}
+static struct notifier_block refresh_nv_rng_seed_nb = { .notifier_call = refresh_nv_rng_seed_notification };
+
/*
* 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 +431,7 @@ static int __init efisubsys_init(void)
platform_device_register_simple("efi_secret", 0, NULL, 0);
#endif
+ execute_with_initialized_rng(&refresh_nv_rng_seed_nb);
return 0;
err_remove_group:
--
2.38.1
On Tue, Nov 22, 2022 at 03:04:00AM +0100, Jason A. Donenfeld wrote:
> In anticipation of putting random seeds in EFI variables, it's important
> that the random GUID namespace of variables remains hidden from
> userspace. We accomplish this by not populating efivarfs with entries
> from that GUID, as well as denying the creation of new ones in that
> GUID.
What's the concern here? Booting an older kernel would allow a malicious
actor to either read the seed variable or set it to a value under their
control, so we can't guarantee that the information is secret.
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.
Hi,
On Sun, Nov 27, 2022 at 09:00:40PM +0000, Matthew Garrett wrote:
> On Tue, Nov 22, 2022 at 03:04:00AM +0100, Jason A. Donenfeld wrote:
> > In anticipation of putting random seeds in EFI variables, it's important
> > that the random GUID namespace of variables remains hidden from
> > userspace. We accomplish this by not populating efivarfs with entries
> > from that GUID, as well as denying the creation of new ones in that
> > GUID.
>
> What's the concern here? Booting an older kernel would allow a malicious
> actor to either read the seed variable or set it to a value under their
> control, so we can't guarantee that the information is secret.
The security model is the same as that of random seed files, on, say,
BSD. If you remove the hard drive or change the operating system or what
have you, then sure, you can fiddle with the seed and read it. But the
running operating system shouldn't show it to you if it can help it.
Consider, for example, systemd's use of EFI variables for the
SystemToken. There, they have PID 1 take care of chmod'ing it before
other processes start. But of course a different OS or even EFI shell
could just read it. So, think of this as just basic runtime safety --
like what people do when they set the umask before writing a random seed
file -- rather than some type of ultimate secrecy.
(And either way, the larger picture is that it's much more important to
get as much random data from as many sources as possible as soon as
possible, rather than being overly paranoid about every one single
source that we start excluding sources. A plethora of sources is better
off here.)
Jason
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
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.