[PATCH] random: defer use of bootloader randomness to random_init()

[PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Stephen reported that a static key warning splat appears during early
boot on systems that credit randomness from device trees that contain an
"rng-seed" property, because because setup_machine_fdt() is called
before jump_label_init() during setup_arch():

 static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
 WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
 pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : static_key_enable_cpuslocked+0xb0/0xb8
 lr : static_key_enable_cpuslocked+0xb0/0xb8
 sp : ffffffe51c393cf0
 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
 Call trace:
  static_key_enable_cpuslocked+0xb0/0xb8
  static_key_enable+0x2c/0x40
  crng_set_ready+0x24/0x30
  execute_in_process_context+0x80/0x90
  _credit_init_bits+0x100/0x154
  add_bootloader_randomness+0x64/0x78
  early_init_dt_scan_chosen+0x140/0x184
  early_init_dt_scan_nodes+0x28/0x4c
  early_init_dt_scan+0x40/0x44
  setup_machine_fdt+0x7c/0x120
  setup_arch+0x74/0x1d8
  start_kernel+0x84/0x44c
  __primary_switched+0xc0/0xc8
 ---[ end trace 0000000000000000 ]---
 random: crng init done
 Machine model: Google Lazor (rev1 - 2) with LTE

A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
Initialize jump labels before setup_machine_fdt()"). But it appears that
fixing it on other platforms might not be so trivial. And in the past
there have been problems related to add_bootloader_randomness() being
called too early in boot for what it needed.

This patch defers all entropy crediting until random_init(), where we
can be sure that all facilities we need are up and running. It changes
the name add_bootloader_randomness() to register_bootloader_seed(), and
insists that its buffer remain alive during all of __init.

A potential downside is that EFI runs a little bit before random_init(),
which means we were getting that seed a lot earlier before.

Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Ard - I guess we've got two things to figure out here:

1) How much we gain by the situation before this patch of EFI supplying
   its seed before random_init().
2) Whether those buffers are guaranteed to live throughout __init, or if
   this patch introduces a UaF.

-Jason

 drivers/char/random.c      | 38 ++++++++++++++++++--------------------
 drivers/firmware/efi/efi.c |  2 +-
 drivers/of/fdt.c           |  2 +-
 include/linux/random.h     |  2 +-
 4 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..d9d00143c7c5 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -678,7 +678,6 @@ static void __cold _credit_init_bits(size_t bits)
  *
  *	void add_device_randomness(const void *buf, size_t len);
  *	void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy);
- *	void add_bootloader_randomness(const void *buf, size_t len);
  *	void add_vmfork_randomness(const void *unique_vm_id, size_t len);
  *	void add_interrupt_randomness(int irq);
  *	void add_input_randomness(unsigned int type, unsigned int code, unsigned int value);
@@ -696,10 +695,6 @@ static void __cold _credit_init_bits(size_t bits)
  * entropy as specified by the caller. If the entropy pool is full it will
  * block until more entropy is needed.
  *
- * add_bootloader_randomness() is called by bootloader drivers, such as EFI
- * and device tree, and credits its input depending on whether or not the
- * configuration option CONFIG_RANDOM_TRUST_BOOTLOADER is set.
- *
  * add_vmfork_randomness() adds a unique (but not necessarily secret) ID
  * representing the current instance of a VM to the pool, without crediting,
  * and then force-reseeds the crng so that it takes effect immediately.
@@ -765,6 +760,18 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
 
 static struct notifier_block pm_notifier = { .notifier_call = random_pm_notification };
 
+/*
+ * Handle random seed passed by bootloader. The buf pointer
+ * must remain alive during the kernel's init sequence.
+ */
+static __initdata const void *bootloader_seed;
+static __initdata size_t bootloader_seed_bytes;
+void __init register_bootloader_seed(const void *buf, size_t len)
+{
+	bootloader_seed = buf;
+	bootloader_seed_bytes = len;
+}
+
 /*
  * The first collection of entropy occurs at system boot while interrupts
  * are still turned off. Here we push in latent entropy, RDSEED, a timestamp,
@@ -793,6 +800,8 @@ int __init random_init(const char *command_line)
 		}
 		_mix_pool_bytes(&entropy, sizeof(entropy));
 	}
+	if (bootloader_seed && bootloader_seed_bytes)
+		_mix_pool_bytes(bootloader_seed, bootloader_seed_bytes);
 	_mix_pool_bytes(&now, sizeof(now));
 	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
 	_mix_pool_bytes(command_line, strlen(command_line));
@@ -800,9 +809,10 @@ int __init random_init(const char *command_line)
 
 	if (crng_ready())
 		crng_reseed();
-	else if (trust_cpu)
-		_credit_init_bits(arch_bytes * 8);
-	used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
+	else
+		_credit_init_bits((trust_cpu ? arch_bytes * 8 : 0) +
+				  (trust_bootloader ? bootloader_seed_bytes * 8 : 0));
+	used_arch_random = arch_bytes * 8 + bootloader_seed_bytes * 8 >= POOL_READY_BITS;
 
 	WARN_ON(register_pm_notifier(&pm_notifier));
 
@@ -861,18 +871,6 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
 }
 EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
 
-/*
- * Handle random seed passed by bootloader, and credit it if
- * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
- */
-void __cold add_bootloader_randomness(const void *buf, size_t len)
-{
-	mix_pool_bytes(buf, len);
-	if (trust_bootloader)
-		credit_init_bits(len * 8);
-}
-EXPORT_SYMBOL_GPL(add_bootloader_randomness);
-
 #if IS_ENABLED(CONFIG_VMGENID)
 static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
 
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 860534bcfdac..daf374983012 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -614,7 +614,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 					      sizeof(*seed) + size);
 			if (seed != NULL) {
 				pr_notice("seeding entropy pool\n");
-				add_bootloader_randomness(seed->bits, size);
+				register_bootloader_seed(seed->bits, size);
 				early_memunmap(seed, sizeof(*seed) + size);
 			} else {
 				pr_err("Could not map UEFI random seed!\n");
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a8f5b6532165..389ef0f781c2 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1201,7 +1201,7 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 
 	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
 	if (rng_seed && l > 0) {
-		add_bootloader_randomness(rng_seed, l);
+		register_bootloader_seed(rng_seed, l);
 
 		/* try to clear seed so it won't be found. */
 		fdt_nop_property(initial_boot_params, node, "rng-seed");
diff --git a/include/linux/random.h b/include/linux/random.h
index fae0c84027fd..c4fa7ca008df 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -13,7 +13,6 @@
 struct notifier_block;
 
 void add_device_randomness(const void *buf, size_t len);
-void add_bootloader_randomness(const void *buf, size_t len);
 void add_input_randomness(unsigned int type, unsigned int code,
 			  unsigned int value) __latent_entropy;
 void add_interrupt_randomness(int irq) __latent_entropy;
@@ -72,6 +71,7 @@ static inline unsigned long get_random_canary(void)
 	return get_random_long() & CANARY_MASK;
 }
 
+void __init register_bootloader_seed(const void *buf, size_t len);
 int __init random_init(const char *command_line);
 bool rng_is_initialized(void);
 bool rng_has_arch_random(void);
-- 
2.35.1

[PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Stephen reported that a static key warning splat appears during early
boot on systems that credit randomness from device trees that contain an
"rng-seed" property, because because setup_machine_fdt() is called
before jump_label_init() during setup_arch():

 static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
 WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
 pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : static_key_enable_cpuslocked+0xb0/0xb8
 lr : static_key_enable_cpuslocked+0xb0/0xb8
 sp : ffffffe51c393cf0
 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
 Call trace:
  static_key_enable_cpuslocked+0xb0/0xb8
  static_key_enable+0x2c/0x40
  crng_set_ready+0x24/0x30
  execute_in_process_context+0x80/0x90
  _credit_init_bits+0x100/0x154
  add_bootloader_randomness+0x64/0x78
  early_init_dt_scan_chosen+0x140/0x184
  early_init_dt_scan_nodes+0x28/0x4c
  early_init_dt_scan+0x40/0x44
  setup_machine_fdt+0x7c/0x120
  setup_arch+0x74/0x1d8
  start_kernel+0x84/0x44c
  __primary_switched+0xc0/0xc8
 ---[ end trace 0000000000000000 ]---
 random: crng init done
 Machine model: Google Lazor (rev1 - 2) with LTE

A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
Initialize jump labels before setup_machine_fdt()"). But it appears that
fixing it on other platforms might not be so trivial. And in the past
there have been problems related to add_bootloader_randomness() being
called too early in boot for what it needed.

This patch defers all entropy crediting until random_init(), where we
can be sure that all facilities we need are up and running. It changes
the name add_bootloader_randomness() to register_bootloader_seed(), and
insists that its buffer remain alive during all of __init.

A potential downside is that EFI runs a little bit before random_init(),
which means we were getting that seed a lot earlier before.

Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Ard - I guess we've got two things to figure out here:

1) How much we gain by the situation before this patch of EFI supplying
   its seed before random_init().
2) Whether those buffers are guaranteed to live throughout __init, or if
   this patch introduces a UaF.

-Jason

 drivers/char/random.c      | 38 ++++++++++++++++++--------------------
 drivers/firmware/efi/efi.c |  2 +-
 drivers/of/fdt.c           |  2 +-
 include/linux/random.h     |  2 +-
 4 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..d9d00143c7c5 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -678,7 +678,6 @@ static void __cold _credit_init_bits(size_t bits)
  *
  *	void add_device_randomness(const void *buf, size_t len);
  *	void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy);
- *	void add_bootloader_randomness(const void *buf, size_t len);
  *	void add_vmfork_randomness(const void *unique_vm_id, size_t len);
  *	void add_interrupt_randomness(int irq);
  *	void add_input_randomness(unsigned int type, unsigned int code, unsigned int value);
@@ -696,10 +695,6 @@ static void __cold _credit_init_bits(size_t bits)
  * entropy as specified by the caller. If the entropy pool is full it will
  * block until more entropy is needed.
  *
- * add_bootloader_randomness() is called by bootloader drivers, such as EFI
- * and device tree, and credits its input depending on whether or not the
- * configuration option CONFIG_RANDOM_TRUST_BOOTLOADER is set.
- *
  * add_vmfork_randomness() adds a unique (but not necessarily secret) ID
  * representing the current instance of a VM to the pool, without crediting,
  * and then force-reseeds the crng so that it takes effect immediately.
@@ -765,6 +760,18 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
 
 static struct notifier_block pm_notifier = { .notifier_call = random_pm_notification };
 
+/*
+ * Handle random seed passed by bootloader. The buf pointer
+ * must remain alive during the kernel's init sequence.
+ */
+static __initdata const void *bootloader_seed;
+static __initdata size_t bootloader_seed_bytes;
+void __init register_bootloader_seed(const void *buf, size_t len)
+{
+	bootloader_seed = buf;
+	bootloader_seed_bytes = len;
+}
+
 /*
  * The first collection of entropy occurs at system boot while interrupts
  * are still turned off. Here we push in latent entropy, RDSEED, a timestamp,
@@ -793,6 +800,8 @@ int __init random_init(const char *command_line)
 		}
 		_mix_pool_bytes(&entropy, sizeof(entropy));
 	}
+	if (bootloader_seed && bootloader_seed_bytes)
+		_mix_pool_bytes(bootloader_seed, bootloader_seed_bytes);
 	_mix_pool_bytes(&now, sizeof(now));
 	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
 	_mix_pool_bytes(command_line, strlen(command_line));
@@ -800,9 +809,10 @@ int __init random_init(const char *command_line)
 
 	if (crng_ready())
 		crng_reseed();
-	else if (trust_cpu)
-		_credit_init_bits(arch_bytes * 8);
-	used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
+	else
+		_credit_init_bits((trust_cpu ? arch_bytes * 8 : 0) +
+				  (trust_bootloader ? bootloader_seed_bytes * 8 : 0));
+	used_arch_random = arch_bytes * 8 + bootloader_seed_bytes * 8 >= POOL_READY_BITS;
 
 	WARN_ON(register_pm_notifier(&pm_notifier));
 
@@ -861,18 +871,6 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
 }
 EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
 
-/*
- * Handle random seed passed by bootloader, and credit it if
- * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
- */
-void __cold add_bootloader_randomness(const void *buf, size_t len)
-{
-	mix_pool_bytes(buf, len);
-	if (trust_bootloader)
-		credit_init_bits(len * 8);
-}
-EXPORT_SYMBOL_GPL(add_bootloader_randomness);
-
 #if IS_ENABLED(CONFIG_VMGENID)
 static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
 
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 860534bcfdac..daf374983012 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -614,7 +614,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 					      sizeof(*seed) + size);
 			if (seed != NULL) {
 				pr_notice("seeding entropy pool\n");
-				add_bootloader_randomness(seed->bits, size);
+				register_bootloader_seed(seed->bits, size);
 				early_memunmap(seed, sizeof(*seed) + size);
 			} else {
 				pr_err("Could not map UEFI random seed!\n");
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a8f5b6532165..389ef0f781c2 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1201,7 +1201,7 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 
 	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
 	if (rng_seed && l > 0) {
-		add_bootloader_randomness(rng_seed, l);
+		register_bootloader_seed(rng_seed, l);
 
 		/* try to clear seed so it won't be found. */
 		fdt_nop_property(initial_boot_params, node, "rng-seed");
diff --git a/include/linux/random.h b/include/linux/random.h
index fae0c84027fd..c4fa7ca008df 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -13,7 +13,6 @@
 struct notifier_block;
 
 void add_device_randomness(const void *buf, size_t len);
-void add_bootloader_randomness(const void *buf, size_t len);
 void add_input_randomness(unsigned int type, unsigned int code,
 			  unsigned int value) __latent_entropy;
 void add_interrupt_randomness(int irq) __latent_entropy;
@@ -72,6 +71,7 @@ static inline unsigned long get_random_canary(void)
 	return get_random_long() & CANARY_MASK;
 }
 
+void __init register_bootloader_seed(const void *buf, size_t len);
 int __init random_init(const char *command_line);
 bool rng_is_initialized(void);
 bool rng_has_arch_random(void);
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Yet another approach would be to mix immediately, but defer crediting
until random_init().

Jason

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Yet another approach would be to mix immediately, but defer crediting
until random_init().

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Stephen reported that a static key warning splat appears during early
boot on systems that credit randomness from device trees that contain an
"rng-seed" property, because because setup_machine_fdt() is called
before jump_label_init() during setup_arch():

 static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
 WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
 pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : static_key_enable_cpuslocked+0xb0/0xb8
 lr : static_key_enable_cpuslocked+0xb0/0xb8
 sp : ffffffe51c393cf0
 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
 Call trace:
  static_key_enable_cpuslocked+0xb0/0xb8
  static_key_enable+0x2c/0x40
  crng_set_ready+0x24/0x30
  execute_in_process_context+0x80/0x90
  _credit_init_bits+0x100/0x154
  add_bootloader_randomness+0x64/0x78
  early_init_dt_scan_chosen+0x140/0x184
  early_init_dt_scan_nodes+0x28/0x4c
  early_init_dt_scan+0x40/0x44
  setup_machine_fdt+0x7c/0x120
  setup_arch+0x74/0x1d8
  start_kernel+0x84/0x44c
  __primary_switched+0xc0/0xc8
 ---[ end trace 0000000000000000 ]---
 random: crng init done
 Machine model: Google Lazor (rev1 - 2) with LTE

A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
Initialize jump labels before setup_machine_fdt()"). But it appears that
fixing it on other platforms might not be so trivial. And in the past
there have been problems related to add_bootloader_randomness() being
called too early in boot for what it needed.

This patch defers all entropy crediting until random_init(), where we
can be sure that all facilities we need are up and running. It still
mixes the actual seed immediately, so that it's maximally useful, but
the crediting doesn't happen until later.

This also has the positive effect of allowing rng_has_arch_random() to
reflect bootloader randomness.

Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 11 ++++++-----
 include/linux/random.h |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..34399e4bad19 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
  **********************************************************************/
 
 static bool used_arch_random;
-static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
-static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
+static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static size_t bootloader_seed_bytes __initdata;
 static int __init parse_trust_cpu(char *arg)
 {
 	return kstrtobool(arg, &trust_cpu);
@@ -793,6 +794,7 @@ int __init random_init(const char *command_line)
 		}
 		_mix_pool_bytes(&entropy, sizeof(entropy));
 	}
+	arch_bytes += bootloader_seed_bytes;
 	_mix_pool_bytes(&now, sizeof(now));
 	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
 	_mix_pool_bytes(command_line, strlen(command_line));
@@ -865,13 +867,12 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
  * Handle random seed passed by bootloader, and credit it if
  * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
  */
-void __cold add_bootloader_randomness(const void *buf, size_t len)
+void __init add_bootloader_randomness(const void *buf, size_t len)
 {
 	mix_pool_bytes(buf, len);
 	if (trust_bootloader)
-		credit_init_bits(len * 8);
+		bootloader_seed_bytes = len;
 }
-EXPORT_SYMBOL_GPL(add_bootloader_randomness);
 
 #if IS_ENABLED(CONFIG_VMGENID)
 static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
diff --git a/include/linux/random.h b/include/linux/random.h
index fae0c84027fd..223b4bd584e7 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -13,7 +13,7 @@
 struct notifier_block;
 
 void add_device_randomness(const void *buf, size_t len);
-void add_bootloader_randomness(const void *buf, size_t len);
+void __init add_bootloader_randomness(const void *buf, size_t len);
 void add_input_randomness(unsigned int type, unsigned int code,
 			  unsigned int value) __latent_entropy;
 void add_interrupt_randomness(int irq) __latent_entropy;
-- 
2.35.1

[PATCH v2] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Stephen reported that a static key warning splat appears during early
boot on systems that credit randomness from device trees that contain an
"rng-seed" property, because because setup_machine_fdt() is called
before jump_label_init() during setup_arch():

 static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
 WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
 pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : static_key_enable_cpuslocked+0xb0/0xb8
 lr : static_key_enable_cpuslocked+0xb0/0xb8
 sp : ffffffe51c393cf0
 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
 Call trace:
  static_key_enable_cpuslocked+0xb0/0xb8
  static_key_enable+0x2c/0x40
  crng_set_ready+0x24/0x30
  execute_in_process_context+0x80/0x90
  _credit_init_bits+0x100/0x154
  add_bootloader_randomness+0x64/0x78
  early_init_dt_scan_chosen+0x140/0x184
  early_init_dt_scan_nodes+0x28/0x4c
  early_init_dt_scan+0x40/0x44
  setup_machine_fdt+0x7c/0x120
  setup_arch+0x74/0x1d8
  start_kernel+0x84/0x44c
  __primary_switched+0xc0/0xc8
 ---[ end trace 0000000000000000 ]---
 random: crng init done
 Machine model: Google Lazor (rev1 - 2) with LTE

A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
Initialize jump labels before setup_machine_fdt()"). But it appears that
fixing it on other platforms might not be so trivial. And in the past
there have been problems related to add_bootloader_randomness() being
called too early in boot for what it needed.

This patch defers all entropy crediting until random_init(), where we
can be sure that all facilities we need are up and running. It still
mixes the actual seed immediately, so that it's maximally useful, but
the crediting doesn't happen until later.

This also has the positive effect of allowing rng_has_arch_random() to
reflect bootloader randomness.

Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 11 ++++++-----
 include/linux/random.h |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..34399e4bad19 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
  **********************************************************************/
 
 static bool used_arch_random;
-static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
-static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
+static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static size_t bootloader_seed_bytes __initdata;
 static int __init parse_trust_cpu(char *arg)
 {
 	return kstrtobool(arg, &trust_cpu);
@@ -793,6 +794,7 @@ int __init random_init(const char *command_line)
 		}
 		_mix_pool_bytes(&entropy, sizeof(entropy));
 	}
+	arch_bytes += bootloader_seed_bytes;
 	_mix_pool_bytes(&now, sizeof(now));
 	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
 	_mix_pool_bytes(command_line, strlen(command_line));
@@ -865,13 +867,12 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
  * Handle random seed passed by bootloader, and credit it if
  * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
  */
-void __cold add_bootloader_randomness(const void *buf, size_t len)
+void __init add_bootloader_randomness(const void *buf, size_t len)
 {
 	mix_pool_bytes(buf, len);
 	if (trust_bootloader)
-		credit_init_bits(len * 8);
+		bootloader_seed_bytes = len;
 }
-EXPORT_SYMBOL_GPL(add_bootloader_randomness);
 
 #if IS_ENABLED(CONFIG_VMGENID)
 static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
diff --git a/include/linux/random.h b/include/linux/random.h
index fae0c84027fd..223b4bd584e7 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -13,7 +13,7 @@
 struct notifier_block;
 
 void add_device_randomness(const void *buf, size_t len);
-void add_bootloader_randomness(const void *buf, size_t len);
+void __init add_bootloader_randomness(const void *buf, size_t len);
 void add_input_randomness(unsigned int type, unsigned int code,
 			  unsigned int value) __latent_entropy;
 void add_interrupt_randomness(int irq) __latent_entropy;
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

Hi Jason,

On Tue, 7 Jun 2022 at 13:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Stephen reported that a static key warning splat appears during early
> boot on systems that credit randomness from device trees that contain an
> "rng-seed" property, because because setup_machine_fdt() is called
> before jump_label_init() during setup_arch():
>
>  static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
>  WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
>  Modules linked in:
>  CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
>  pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : static_key_enable_cpuslocked+0xb0/0xb8
>  lr : static_key_enable_cpuslocked+0xb0/0xb8
>  sp : ffffffe51c393cf0
>  x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
>  x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
>  x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
>  x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
>  x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
>  x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
>  x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>  x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
>  x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
>  x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
>  Call trace:
>   static_key_enable_cpuslocked+0xb0/0xb8
>   static_key_enable+0x2c/0x40
>   crng_set_ready+0x24/0x30
>   execute_in_process_context+0x80/0x90
>   _credit_init_bits+0x100/0x154
>   add_bootloader_randomness+0x64/0x78
>   early_init_dt_scan_chosen+0x140/0x184
>   early_init_dt_scan_nodes+0x28/0x4c
>   early_init_dt_scan+0x40/0x44
>   setup_machine_fdt+0x7c/0x120
>   setup_arch+0x74/0x1d8
>   start_kernel+0x84/0x44c
>   __primary_switched+0xc0/0xc8
>  ---[ end trace 0000000000000000 ]---
>  random: crng init done
>  Machine model: Google Lazor (rev1 - 2) with LTE
>
> A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
> Initialize jump labels before setup_machine_fdt()"). But it appears that
> fixing it on other platforms might not be so trivial. And in the past
> there have been problems related to add_bootloader_randomness() being
> called too early in boot for what it needed.
>
> This patch defers all entropy crediting until random_init(), where we
> can be sure that all facilities we need are up and running. It changes
> the name add_bootloader_randomness() to register_bootloader_seed(), and
> insists that its buffer remain alive during all of __init.
>
> A potential downside is that EFI runs a little bit before random_init(),
> which means we were getting that seed a lot earlier before.
>
> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> Reported-by: Stephen Boyd <swboyd@chromium.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Ard - I guess we've got two things to figure out here:
>
> 1) How much we gain by the situation before this patch of EFI supplying
>    its seed before random_init().
> 2) Whether those buffers are guaranteed to live throughout __init, or if
>    this patch introduces a UaF.
>

This is not going to work, I'm afraid - please see below.


>  drivers/char/random.c      | 38 ++++++++++++++++++--------------------
>  drivers/firmware/efi/efi.c |  2 +-
>  drivers/of/fdt.c           |  2 +-
>  include/linux/random.h     |  2 +-
>  4 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 4862d4d3ec49..d9d00143c7c5 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -678,7 +678,6 @@ static void __cold _credit_init_bits(size_t bits)
>   *
>   *     void add_device_randomness(const void *buf, size_t len);
>   *     void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy);
> - *     void add_bootloader_randomness(const void *buf, size_t len);
>   *     void add_vmfork_randomness(const void *unique_vm_id, size_t len);
>   *     void add_interrupt_randomness(int irq);
>   *     void add_input_randomness(unsigned int type, unsigned int code, unsigned int value);
> @@ -696,10 +695,6 @@ static void __cold _credit_init_bits(size_t bits)
>   * entropy as specified by the caller. If the entropy pool is full it will
>   * block until more entropy is needed.
>   *
> - * add_bootloader_randomness() is called by bootloader drivers, such as EFI
> - * and device tree, and credits its input depending on whether or not the
> - * configuration option CONFIG_RANDOM_TRUST_BOOTLOADER is set.
> - *
>   * add_vmfork_randomness() adds a unique (but not necessarily secret) ID
>   * representing the current instance of a VM to the pool, without crediting,
>   * and then force-reseeds the crng so that it takes effect immediately.
> @@ -765,6 +760,18 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
>
>  static struct notifier_block pm_notifier = { .notifier_call = random_pm_notification };
>
> +/*
> + * Handle random seed passed by bootloader. The buf pointer
> + * must remain alive during the kernel's init sequence.
> + */
> +static __initdata const void *bootloader_seed;
> +static __initdata size_t bootloader_seed_bytes;
> +void __init register_bootloader_seed(const void *buf, size_t len)
> +{
> +       bootloader_seed = buf;
> +       bootloader_seed_bytes = len;
> +}
> +
>  /*
>   * The first collection of entropy occurs at system boot while interrupts
>   * are still turned off. Here we push in latent entropy, RDSEED, a timestamp,
> @@ -793,6 +800,8 @@ int __init random_init(const char *command_line)
>                 }
>                 _mix_pool_bytes(&entropy, sizeof(entropy));
>         }
> +       if (bootloader_seed && bootloader_seed_bytes)
> +               _mix_pool_bytes(bootloader_seed, bootloader_seed_bytes);
>         _mix_pool_bytes(&now, sizeof(now));
>         _mix_pool_bytes(utsname(), sizeof(*(utsname())));
>         _mix_pool_bytes(command_line, strlen(command_line));
> @@ -800,9 +809,10 @@ int __init random_init(const char *command_line)
>
>         if (crng_ready())
>                 crng_reseed();
> -       else if (trust_cpu)
> -               _credit_init_bits(arch_bytes * 8);
> -       used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
> +       else
> +               _credit_init_bits((trust_cpu ? arch_bytes * 8 : 0) +
> +                                 (trust_bootloader ? bootloader_seed_bytes * 8 : 0));
> +       used_arch_random = arch_bytes * 8 + bootloader_seed_bytes * 8 >= POOL_READY_BITS;
>
>         WARN_ON(register_pm_notifier(&pm_notifier));
>
> @@ -861,18 +871,6 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
>  }
>  EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>
> -/*
> - * Handle random seed passed by bootloader, and credit it if
> - * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
> - */
> -void __cold add_bootloader_randomness(const void *buf, size_t len)
> -{
> -       mix_pool_bytes(buf, len);
> -       if (trust_bootloader)
> -               credit_init_bits(len * 8);
> -}
> -EXPORT_SYMBOL_GPL(add_bootloader_randomness);
> -
>  #if IS_ENABLED(CONFIG_VMGENID)
>  static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 860534bcfdac..daf374983012 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -614,7 +614,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                                               sizeof(*seed) + size);
>                         if (seed != NULL) {
>                                 pr_notice("seeding entropy pool\n");
> -                               add_bootloader_randomness(seed->bits, size);
> +                               register_bootloader_seed(seed->bits, size);

The next line says it all, really: the seed is in a firmware table
somewhere, and only gets mapped temporarily here. Note that we cannot
copy it either, as we are running way before we have discovered where
RAM is to begin with.



>                                 early_memunmap(seed, sizeof(*seed) + size);
>                         } else {
>                                 pr_err("Could not map UEFI random seed!\n");
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index a8f5b6532165..389ef0f781c2 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1201,7 +1201,7 @@ int __init early_init_dt_scan_chosen(char *cmdline)
>
>         rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
>         if (rng_seed && l > 0) {
> -               add_bootloader_randomness(rng_seed, l);
> +               register_bootloader_seed(rng_seed, l);
>
>                 /* try to clear seed so it won't be found. */
>                 fdt_nop_property(initial_boot_params, node, "rng-seed");
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fae0c84027fd..c4fa7ca008df 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -13,7 +13,6 @@
>  struct notifier_block;
>
>  void add_device_randomness(const void *buf, size_t len);
> -void add_bootloader_randomness(const void *buf, size_t len);
>  void add_input_randomness(unsigned int type, unsigned int code,
>                           unsigned int value) __latent_entropy;
>  void add_interrupt_randomness(int irq) __latent_entropy;
> @@ -72,6 +71,7 @@ static inline unsigned long get_random_canary(void)
>         return get_random_long() & CANARY_MASK;
>  }
>
> +void __init register_bootloader_seed(const void *buf, size_t len);
>  int __init random_init(const char *command_line);
>  bool rng_is_initialized(void);
>  bool rng_has_arch_random(void);

More fundamentally, I feel we are losing sight of the actual issue.
This is another patch that makes potentially risky functional changes
to very early boot code, only so that we can manipulate the static key
as early as we think we need to.

So could we please go back to some basic questions here;
- Why do we need/want a static key here to begin with? Is is for performance?
- Why do we need to enable this static key so early?
- Even if very convincing replies can be given to the previous two
points, wouldn't it be betterr to simply revert the -stable backport
that introduces the use of the static key, and find a robust and
portable solution for after v5.19?

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

Hi Jason,

On Tue, 7 Jun 2022 at 13:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Stephen reported that a static key warning splat appears during early
> boot on systems that credit randomness from device trees that contain an
> "rng-seed" property, because because setup_machine_fdt() is called
> before jump_label_init() during setup_arch():
>
>  static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
>  WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
>  Modules linked in:
>  CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
>  pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : static_key_enable_cpuslocked+0xb0/0xb8
>  lr : static_key_enable_cpuslocked+0xb0/0xb8
>  sp : ffffffe51c393cf0
>  x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
>  x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
>  x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
>  x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
>  x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
>  x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
>  x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>  x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
>  x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
>  x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
>  Call trace:
>   static_key_enable_cpuslocked+0xb0/0xb8
>   static_key_enable+0x2c/0x40
>   crng_set_ready+0x24/0x30
>   execute_in_process_context+0x80/0x90
>   _credit_init_bits+0x100/0x154
>   add_bootloader_randomness+0x64/0x78
>   early_init_dt_scan_chosen+0x140/0x184
>   early_init_dt_scan_nodes+0x28/0x4c
>   early_init_dt_scan+0x40/0x44
>   setup_machine_fdt+0x7c/0x120
>   setup_arch+0x74/0x1d8
>   start_kernel+0x84/0x44c
>   __primary_switched+0xc0/0xc8
>  ---[ end trace 0000000000000000 ]---
>  random: crng init done
>  Machine model: Google Lazor (rev1 - 2) with LTE
>
> A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
> Initialize jump labels before setup_machine_fdt()"). But it appears that
> fixing it on other platforms might not be so trivial. And in the past
> there have been problems related to add_bootloader_randomness() being
> called too early in boot for what it needed.
>
> This patch defers all entropy crediting until random_init(), where we
> can be sure that all facilities we need are up and running. It changes
> the name add_bootloader_randomness() to register_bootloader_seed(), and
> insists that its buffer remain alive during all of __init.
>
> A potential downside is that EFI runs a little bit before random_init(),
> which means we were getting that seed a lot earlier before.
>
> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> Reported-by: Stephen Boyd <swboyd@chromium.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Ard - I guess we've got two things to figure out here:
>
> 1) How much we gain by the situation before this patch of EFI supplying
>    its seed before random_init().
> 2) Whether those buffers are guaranteed to live throughout __init, or if
>    this patch introduces a UaF.
>

This is not going to work, I'm afraid - please see below.


>  drivers/char/random.c      | 38 ++++++++++++++++++--------------------
>  drivers/firmware/efi/efi.c |  2 +-
>  drivers/of/fdt.c           |  2 +-
>  include/linux/random.h     |  2 +-
>  4 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 4862d4d3ec49..d9d00143c7c5 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -678,7 +678,6 @@ static void __cold _credit_init_bits(size_t bits)
>   *
>   *     void add_device_randomness(const void *buf, size_t len);
>   *     void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy);
> - *     void add_bootloader_randomness(const void *buf, size_t len);
>   *     void add_vmfork_randomness(const void *unique_vm_id, size_t len);
>   *     void add_interrupt_randomness(int irq);
>   *     void add_input_randomness(unsigned int type, unsigned int code, unsigned int value);
> @@ -696,10 +695,6 @@ static void __cold _credit_init_bits(size_t bits)
>   * entropy as specified by the caller. If the entropy pool is full it will
>   * block until more entropy is needed.
>   *
> - * add_bootloader_randomness() is called by bootloader drivers, such as EFI
> - * and device tree, and credits its input depending on whether or not the
> - * configuration option CONFIG_RANDOM_TRUST_BOOTLOADER is set.
> - *
>   * add_vmfork_randomness() adds a unique (but not necessarily secret) ID
>   * representing the current instance of a VM to the pool, without crediting,
>   * and then force-reseeds the crng so that it takes effect immediately.
> @@ -765,6 +760,18 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
>
>  static struct notifier_block pm_notifier = { .notifier_call = random_pm_notification };
>
> +/*
> + * Handle random seed passed by bootloader. The buf pointer
> + * must remain alive during the kernel's init sequence.
> + */
> +static __initdata const void *bootloader_seed;
> +static __initdata size_t bootloader_seed_bytes;
> +void __init register_bootloader_seed(const void *buf, size_t len)
> +{
> +       bootloader_seed = buf;
> +       bootloader_seed_bytes = len;
> +}
> +
>  /*
>   * The first collection of entropy occurs at system boot while interrupts
>   * are still turned off. Here we push in latent entropy, RDSEED, a timestamp,
> @@ -793,6 +800,8 @@ int __init random_init(const char *command_line)
>                 }
>                 _mix_pool_bytes(&entropy, sizeof(entropy));
>         }
> +       if (bootloader_seed && bootloader_seed_bytes)
> +               _mix_pool_bytes(bootloader_seed, bootloader_seed_bytes);
>         _mix_pool_bytes(&now, sizeof(now));
>         _mix_pool_bytes(utsname(), sizeof(*(utsname())));
>         _mix_pool_bytes(command_line, strlen(command_line));
> @@ -800,9 +809,10 @@ int __init random_init(const char *command_line)
>
>         if (crng_ready())
>                 crng_reseed();
> -       else if (trust_cpu)
> -               _credit_init_bits(arch_bytes * 8);
> -       used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
> +       else
> +               _credit_init_bits((trust_cpu ? arch_bytes * 8 : 0) +
> +                                 (trust_bootloader ? bootloader_seed_bytes * 8 : 0));
> +       used_arch_random = arch_bytes * 8 + bootloader_seed_bytes * 8 >= POOL_READY_BITS;
>
>         WARN_ON(register_pm_notifier(&pm_notifier));
>
> @@ -861,18 +871,6 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
>  }
>  EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>
> -/*
> - * Handle random seed passed by bootloader, and credit it if
> - * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
> - */
> -void __cold add_bootloader_randomness(const void *buf, size_t len)
> -{
> -       mix_pool_bytes(buf, len);
> -       if (trust_bootloader)
> -               credit_init_bits(len * 8);
> -}
> -EXPORT_SYMBOL_GPL(add_bootloader_randomness);
> -
>  #if IS_ENABLED(CONFIG_VMGENID)
>  static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 860534bcfdac..daf374983012 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -614,7 +614,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                                               sizeof(*seed) + size);
>                         if (seed != NULL) {
>                                 pr_notice("seeding entropy pool\n");
> -                               add_bootloader_randomness(seed->bits, size);
> +                               register_bootloader_seed(seed->bits, size);

The next line says it all, really: the seed is in a firmware table
somewhere, and only gets mapped temporarily here. Note that we cannot
copy it either, as we are running way before we have discovered where
RAM is to begin with.



>                                 early_memunmap(seed, sizeof(*seed) + size);
>                         } else {
>                                 pr_err("Could not map UEFI random seed!\n");
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index a8f5b6532165..389ef0f781c2 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1201,7 +1201,7 @@ int __init early_init_dt_scan_chosen(char *cmdline)
>
>         rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
>         if (rng_seed && l > 0) {
> -               add_bootloader_randomness(rng_seed, l);
> +               register_bootloader_seed(rng_seed, l);
>
>                 /* try to clear seed so it won't be found. */
>                 fdt_nop_property(initial_boot_params, node, "rng-seed");
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fae0c84027fd..c4fa7ca008df 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -13,7 +13,6 @@
>  struct notifier_block;
>
>  void add_device_randomness(const void *buf, size_t len);
> -void add_bootloader_randomness(const void *buf, size_t len);
>  void add_input_randomness(unsigned int type, unsigned int code,
>                           unsigned int value) __latent_entropy;
>  void add_interrupt_randomness(int irq) __latent_entropy;
> @@ -72,6 +71,7 @@ static inline unsigned long get_random_canary(void)
>         return get_random_long() & CANARY_MASK;
>  }
>
> +void __init register_bootloader_seed(const void *buf, size_t len);
>  int __init random_init(const char *command_line);
>  bool rng_is_initialized(void);
>  bool rng_has_arch_random(void);

More fundamentally, I feel we are losing sight of the actual issue.
This is another patch that makes potentially risky functional changes
to very early boot code, only so that we can manipulate the static key
as early as we think we need to.

So could we please go back to some basic questions here;
- Why do we need/want a static key here to begin with? Is is for performance?
- Why do we need to enable this static key so early?
- Even if very convincing replies can be given to the previous two
points, wouldn't it be betterr to simply revert the -stable backport
that introduces the use of the static key, and find a robust and
portable solution for after v5.19?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Ard,

On Tue, Jun 07, 2022 at 01:55:37PM +0200, Ard Biesheuvel wrote:
> This is not going to work, I'm afraid - please see below.
>
> The next line says it all, really: the seed is in a firmware table
> somewhere, and only gets mapped temporarily here. Note that we cannot
> copy it either, as we are running way before we have discovered where
> RAM is to begin with.

Oh, darn. Okay. The v2 might be a bit more palpable then. It mixes
immediately, but defers crediting:

[1] https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/

> - Even if very convincing replies can be given to the previous two
> points, wouldn't it be betterr to simply revert the -stable backport
> that introduces the use of the static key, and find a robust and
> portable solution for after v5.19?

This has already been done:

[2] https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/

And mentioned here:

[3] https://lore.kernel.org/lkml/CAHmME9rJif3ydZuFJcSjPxkGMofZkbu2PXcHBF23OWVgGQ4c+A@mail.gmail.com/

You're on this thread.

> So could we please go back to some basic questions here;
> - Why do we need/want a static key here to begin with? Is is for performance?
> - Why do we need to enable this static key so early?

We don't need to enable it especially early. I've now sent three
different approaches for deferring it until later and you suggested one.
The first of mine is kind of ugly (checking static_key_initialized and
such at different points).  Your suggested one after that did the same
but deferred into crng_reseed(), which I'm not a fan of. My second one
is this patch, which is flawed for the reason you pointed out. But
perhaps my third one is the right amount of simple and okay? That's the
one I linked up top, [1]. Let me know what you think of that.

My motivation for not wanting to defer it is that if the arch solution
winds up being easy and straight forward (as it was for arm64), then it
would be nice to not need to clutter up random.c as a result. But if
the arch solution winds up looking fragile or problematic somehow, then
one of these solutions should do the trick. In particular [1] seems nice
enough that it doesn't really even clutter things up too much as feared.

Jason

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Ard,

On Tue, Jun 07, 2022 at 01:55:37PM +0200, Ard Biesheuvel wrote:
> This is not going to work, I'm afraid - please see below.
>
> The next line says it all, really: the seed is in a firmware table
> somewhere, and only gets mapped temporarily here. Note that we cannot
> copy it either, as we are running way before we have discovered where
> RAM is to begin with.

Oh, darn. Okay. The v2 might be a bit more palpable then. It mixes
immediately, but defers crediting:

[1] https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/

> - Even if very convincing replies can be given to the previous two
> points, wouldn't it be betterr to simply revert the -stable backport
> that introduces the use of the static key, and find a robust and
> portable solution for after v5.19?

This has already been done:

[2] https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/

And mentioned here:

[3] https://lore.kernel.org/lkml/CAHmME9rJif3ydZuFJcSjPxkGMofZkbu2PXcHBF23OWVgGQ4c+A@mail.gmail.com/

You're on this thread.

> So could we please go back to some basic questions here;
> - Why do we need/want a static key here to begin with? Is is for performance?
> - Why do we need to enable this static key so early?

We don't need to enable it especially early. I've now sent three
different approaches for deferring it until later and you suggested one.
The first of mine is kind of ugly (checking static_key_initialized and
such at different points).  Your suggested one after that did the same
but deferred into crng_reseed(), which I'm not a fan of. My second one
is this patch, which is flawed for the reason you pointed out. But
perhaps my third one is the right amount of simple and okay? That's the
one I linked up top, [1]. Let me know what you think of that.

My motivation for not wanting to defer it is that if the arch solution
winds up being easy and straight forward (as it was for arm64), then it
would be nice to not need to clutter up random.c as a result. But if
the arch solution winds up looking fragile or problematic somehow, then
one of these solutions should do the trick. In particular [1] seems nice
enough that it doesn't really even clutter things up too much as feared.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 14:05, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Tue, Jun 07, 2022 at 01:55:37PM +0200, Ard Biesheuvel wrote:
> > This is not going to work, I'm afraid - please see below.
> >
> > The next line says it all, really: the seed is in a firmware table
> > somewhere, and only gets mapped temporarily here. Note that we cannot
> > copy it either, as we are running way before we have discovered where
> > RAM is to begin with.
>
> Oh, darn. Okay. The v2 might be a bit more palpable then. It mixes
> immediately, but defers crediting:
>
> [1] https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/
>
> > - Even if very convincing replies can be given to the previous two
> > points, wouldn't it be betterr to simply revert the -stable backport
> > that introduces the use of the static key, and find a robust and
> > portable solution for after v5.19?
>
> This has already been done:
>
> [2] https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/
>
> And mentioned here:
>
> [3] https://lore.kernel.org/lkml/CAHmME9rJif3ydZuFJcSjPxkGMofZkbu2PXcHBF23OWVgGQ4c+A@mail.gmail.com/
>
> You're on this thread.
>

OK, excellent, thanks for confirming. There are so many threads now
going on at the same time that I lost track.

> > So could we please go back to some basic questions here;
> > - Why do we need/want a static key here to begin with? Is is for performance?

You still haven't answered this question, f5bda35fba61 only reasons
about how we can use a static branch but not why we should.

Note that jump labels use asm() blocks, which are opaque to the
compiler, and so it is not guaranteed that codegen will be better than
a single conditional branch that will always be predicted correctly at
runtime. And it clutters up the code, given that you need to use the
execute_in_process_context() helper to perform the static key
manipulation outside of the calling context.

> > - Why do we need to enable this static key so early?
>
> We don't need to enable it especially early. I've now sent three
> different approaches for deferring it until later and you suggested one.
> The first of mine is kind of ugly (checking static_key_initialized and
> such at different points).  Your suggested one after that did the same
> but deferred into crng_reseed(), which I'm not a fan of. My second one
> is this patch, which is flawed for the reason you pointed out. But
> perhaps my third one is the right amount of simple and okay? That's the
> one I linked up top, [1]. Let me know what you think of that.
>
> My motivation for not wanting to defer it is that if the arch solution
> winds up being easy and straight forward (as it was for arm64), then it
> would be nice to not need to clutter up random.c as a result.

If clutter is a concern, how about getting rid of the
execute_in_process_context() dance, and just use a late_initcall()
instead?

> But if
> the arch solution winds up looking fragile or problematic somehow, then
> one of these solutions should do the trick. In particular [1] seems nice
> enough that it doesn't really even clutter things up too much as feared.
>

As I replied there, I think that patch is not as bad as the other ones
that have been suggested (including mine). But I still feel we are
going through a lot of trouble to enable something that we really
don't need here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 14:05, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Tue, Jun 07, 2022 at 01:55:37PM +0200, Ard Biesheuvel wrote:
> > This is not going to work, I'm afraid - please see below.
> >
> > The next line says it all, really: the seed is in a firmware table
> > somewhere, and only gets mapped temporarily here. Note that we cannot
> > copy it either, as we are running way before we have discovered where
> > RAM is to begin with.
>
> Oh, darn. Okay. The v2 might be a bit more palpable then. It mixes
> immediately, but defers crediting:
>
> [1] https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/
>
> > - Even if very convincing replies can be given to the previous two
> > points, wouldn't it be betterr to simply revert the -stable backport
> > that introduces the use of the static key, and find a robust and
> > portable solution for after v5.19?
>
> This has already been done:
>
> [2] https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/
>
> And mentioned here:
>
> [3] https://lore.kernel.org/lkml/CAHmME9rJif3ydZuFJcSjPxkGMofZkbu2PXcHBF23OWVgGQ4c+A@mail.gmail.com/
>
> You're on this thread.
>

OK, excellent, thanks for confirming. There are so many threads now
going on at the same time that I lost track.

> > So could we please go back to some basic questions here;
> > - Why do we need/want a static key here to begin with? Is is for performance?

You still haven't answered this question, f5bda35fba61 only reasons
about how we can use a static branch but not why we should.

Note that jump labels use asm() blocks, which are opaque to the
compiler, and so it is not guaranteed that codegen will be better than
a single conditional branch that will always be predicted correctly at
runtime. And it clutters up the code, given that you need to use the
execute_in_process_context() helper to perform the static key
manipulation outside of the calling context.

> > - Why do we need to enable this static key so early?
>
> We don't need to enable it especially early. I've now sent three
> different approaches for deferring it until later and you suggested one.
> The first of mine is kind of ugly (checking static_key_initialized and
> such at different points).  Your suggested one after that did the same
> but deferred into crng_reseed(), which I'm not a fan of. My second one
> is this patch, which is flawed for the reason you pointed out. But
> perhaps my third one is the right amount of simple and okay? That's the
> one I linked up top, [1]. Let me know what you think of that.
>
> My motivation for not wanting to defer it is that if the arch solution
> winds up being easy and straight forward (as it was for arm64), then it
> would be nice to not need to clutter up random.c as a result.

If clutter is a concern, how about getting rid of the
execute_in_process_context() dance, and just use a late_initcall()
instead?

> But if
> the arch solution winds up looking fragile or problematic somehow, then
> one of these solutions should do the trick. In particular [1] seems nice
> enough that it doesn't really even clutter things up too much as feared.
>

As I replied there, I think that patch is not as bad as the other ones
that have been suggested (including mine). But I still feel we are
going through a lot of trouble to enable something that we really
don't need here.

Re: [PATCH v2] random: defer crediting bootloader randomness to random_init()

[Phil Elwell]

Hi Jason,

On 07/06/2022 12:32, Jason A. Donenfeld wrote:
> Stephen reported that a static key warning splat appears during early
> boot on systems that credit randomness from device trees that contain an
> "rng-seed" property, because because setup_machine_fdt() is called
> before jump_label_init() during setup_arch():
> 
>   static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
>   WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
>   pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : static_key_enable_cpuslocked+0xb0/0xb8
>   lr : static_key_enable_cpuslocked+0xb0/0xb8
>   sp : ffffffe51c393cf0
>   x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
>   x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
>   x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
>   x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
>   x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
>   x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
>   x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>   x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
>   x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
>   x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
>   Call trace:
>    static_key_enable_cpuslocked+0xb0/0xb8
>    static_key_enable+0x2c/0x40
>    crng_set_ready+0x24/0x30
>    execute_in_process_context+0x80/0x90
>    _credit_init_bits+0x100/0x154
>    add_bootloader_randomness+0x64/0x78
>    early_init_dt_scan_chosen+0x140/0x184
>    early_init_dt_scan_nodes+0x28/0x4c
>    early_init_dt_scan+0x40/0x44
>    setup_machine_fdt+0x7c/0x120
>    setup_arch+0x74/0x1d8
>    start_kernel+0x84/0x44c
>    __primary_switched+0xc0/0xc8
>   ---[ end trace 0000000000000000 ]---
>   random: crng init done
>   Machine model: Google Lazor (rev1 - 2) with LTE
> 
> A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
> Initialize jump labels before setup_machine_fdt()"). But it appears that
> fixing it on other platforms might not be so trivial. And in the past
> there have been problems related to add_bootloader_randomness() being
> called too early in boot for what it needed.
> 
> This patch defers all entropy crediting until random_init(), where we
> can be sure that all facilities we need are up and running. It still
> mixes the actual seed immediately, so that it's maximally useful, but
> the crediting doesn't happen until later.
> 
> This also has the positive effect of allowing rng_has_arch_random() to
> reflect bootloader randomness.
> 
> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> Reported-by: Stephen Boyd <swboyd@chromium.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   drivers/char/random.c  | 11 ++++++-----
>   include/linux/random.h |  2 +-
>   2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 4862d4d3ec49..34399e4bad19 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
>    **********************************************************************/
>   
>   static bool used_arch_random;
> -static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> -static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> +static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static size_t bootloader_seed_bytes __initdata;
>   static int __init parse_trust_cpu(char *arg)
>   {
>   	return kstrtobool(arg, &trust_cpu);
> @@ -793,6 +794,7 @@ int __init random_init(const char *command_line)
>   		}
>   		_mix_pool_bytes(&entropy, sizeof(entropy));
>   	}
> +	arch_bytes += bootloader_seed_bytes;
>   	_mix_pool_bytes(&now, sizeof(now));
>   	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
>   	_mix_pool_bytes(command_line, strlen(command_line));
> @@ -865,13 +867,12 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>    * Handle random seed passed by bootloader, and credit it if
>    * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
>    */
> -void __cold add_bootloader_randomness(const void *buf, size_t len)
> +void __init add_bootloader_randomness(const void *buf, size_t len)
>   {
>   	mix_pool_bytes(buf, len);
>   	if (trust_bootloader)
> -		credit_init_bits(len * 8);
> +		bootloader_seed_bytes = len;
>   }
> -EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>   
>   #if IS_ENABLED(CONFIG_VMGENID)
>   static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fae0c84027fd..223b4bd584e7 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -13,7 +13,7 @@
>   struct notifier_block;
>   
>   void add_device_randomness(const void *buf, size_t len);
> -void add_bootloader_randomness(const void *buf, size_t len);
> +void __init add_bootloader_randomness(const void *buf, size_t len);
>   void add_input_randomness(unsigned int type, unsigned int code,
>   			  unsigned int value) __latent_entropy;
>   void add_interrupt_randomness(int irq) __latent_entropy;

After a trivial merge conflict (into 5.15) was resolved, this patch allows the 
downstream kernel with CONFIG_RANDOM_TRUST_BOOTLOADER=y to boot cleanly. 
However, it does delay the initialisation of crng significantly compared to my
hack.

With v2:
[    1.981493] bcm2835-rng 3f104000.rng: hwrng registered
[   12.549190] random: crng init done

With the hack:
[    0.000000] random: crng init done
[    1.970662] bcm2835-rng 3f104000.rng: hwrng registered

I'll leave it to others to decide whether or not this is acceptable.

Phil

Re: [PATCH v2] random: defer crediting bootloader randomness to random_init()

[Phil Elwell]

Hi Jason,

On 07/06/2022 12:32, Jason A. Donenfeld wrote:
> Stephen reported that a static key warning splat appears during early
> boot on systems that credit randomness from device trees that contain an
> "rng-seed" property, because because setup_machine_fdt() is called
> before jump_label_init() during setup_arch():
> 
>   static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
>   WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
>   pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : static_key_enable_cpuslocked+0xb0/0xb8
>   lr : static_key_enable_cpuslocked+0xb0/0xb8
>   sp : ffffffe51c393cf0
>   x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
>   x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
>   x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
>   x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
>   x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
>   x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
>   x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>   x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
>   x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
>   x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
>   Call trace:
>    static_key_enable_cpuslocked+0xb0/0xb8
>    static_key_enable+0x2c/0x40
>    crng_set_ready+0x24/0x30
>    execute_in_process_context+0x80/0x90
>    _credit_init_bits+0x100/0x154
>    add_bootloader_randomness+0x64/0x78
>    early_init_dt_scan_chosen+0x140/0x184
>    early_init_dt_scan_nodes+0x28/0x4c
>    early_init_dt_scan+0x40/0x44
>    setup_machine_fdt+0x7c/0x120
>    setup_arch+0x74/0x1d8
>    start_kernel+0x84/0x44c
>    __primary_switched+0xc0/0xc8
>   ---[ end trace 0000000000000000 ]---
>   random: crng init done
>   Machine model: Google Lazor (rev1 - 2) with LTE
> 
> A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
> Initialize jump labels before setup_machine_fdt()"). But it appears that
> fixing it on other platforms might not be so trivial. And in the past
> there have been problems related to add_bootloader_randomness() being
> called too early in boot for what it needed.
> 
> This patch defers all entropy crediting until random_init(), where we
> can be sure that all facilities we need are up and running. It still
> mixes the actual seed immediately, so that it's maximally useful, but
> the crediting doesn't happen until later.
> 
> This also has the positive effect of allowing rng_has_arch_random() to
> reflect bootloader randomness.
> 
> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> Reported-by: Stephen Boyd <swboyd@chromium.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   drivers/char/random.c  | 11 ++++++-----
>   include/linux/random.h |  2 +-
>   2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 4862d4d3ec49..34399e4bad19 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
>    **********************************************************************/
>   
>   static bool used_arch_random;
> -static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> -static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> +static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static size_t bootloader_seed_bytes __initdata;
>   static int __init parse_trust_cpu(char *arg)
>   {
>   	return kstrtobool(arg, &trust_cpu);
> @@ -793,6 +794,7 @@ int __init random_init(const char *command_line)
>   		}
>   		_mix_pool_bytes(&entropy, sizeof(entropy));
>   	}
> +	arch_bytes += bootloader_seed_bytes;
>   	_mix_pool_bytes(&now, sizeof(now));
>   	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
>   	_mix_pool_bytes(command_line, strlen(command_line));
> @@ -865,13 +867,12 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>    * Handle random seed passed by bootloader, and credit it if
>    * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
>    */
> -void __cold add_bootloader_randomness(const void *buf, size_t len)
> +void __init add_bootloader_randomness(const void *buf, size_t len)
>   {
>   	mix_pool_bytes(buf, len);
>   	if (trust_bootloader)
> -		credit_init_bits(len * 8);
> +		bootloader_seed_bytes = len;
>   }
> -EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>   
>   #if IS_ENABLED(CONFIG_VMGENID)
>   static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fae0c84027fd..223b4bd584e7 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -13,7 +13,7 @@
>   struct notifier_block;
>   
>   void add_device_randomness(const void *buf, size_t len);
> -void add_bootloader_randomness(const void *buf, size_t len);
> +void __init add_bootloader_randomness(const void *buf, size_t len);
>   void add_input_randomness(unsigned int type, unsigned int code,
>   			  unsigned int value) __latent_entropy;
>   void add_interrupt_randomness(int irq) __latent_entropy;

After a trivial merge conflict (into 5.15) was resolved, this patch allows the 
downstream kernel with CONFIG_RANDOM_TRUST_BOOTLOADER=y to boot cleanly. 
However, it does delay the initialisation of crng significantly compared to my
hack.

With v2:
[    1.981493] bcm2835-rng 3f104000.rng: hwrng registered
[   12.549190] random: crng init done

With the hack:
[    0.000000] random: crng init done
[    1.970662] bcm2835-rng 3f104000.rng: hwrng registered

I'll leave it to others to decide whether or not this is acceptable.

Phil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Ard,

On Tue, Jun 07, 2022 at 02:15:27PM +0200, Ard Biesheuvel wrote:
> Note that jump labels use asm() blocks, which are opaque to the
> compiler, and so it is not guaranteed that codegen will be better than

I actually spent a lot of time looking at the codegen on a few
platforms.

> > > - Why do we need to enable this static key so early?
> >
> > We don't need to enable it especially early. I've now sent three
> > different approaches for deferring it until later and you suggested one.
> > The first of mine is kind of ugly (checking static_key_initialized and
> > such at different points).  Your suggested one after that did the same
> > but deferred into crng_reseed(), which I'm not a fan of. My second one
> > is this patch, which is flawed for the reason you pointed out. But
> > perhaps my third one is the right amount of simple and okay? That's the
> > one I linked up top, [1]. Let me know what you think of that.
> >
> > My motivation for not wanting to defer it is that if the arch solution
> > winds up being easy and straight forward (as it was for arm64), then it
> > would be nice to not need to clutter up random.c as a result.
> 
> If clutter is a concern, how about getting rid of the
> execute_in_process_context() dance, and just use a late_initcall()
> instead?

As I already explained in [1], this does not work. If the order is
(A)(B), then all this will happen *after* the late init call.

[1] https://lore.kernel.org/lkml/Yp8oOH+9V336LrLk@zx2c4.com/

Jason

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Ard,

On Tue, Jun 07, 2022 at 02:15:27PM +0200, Ard Biesheuvel wrote:
> Note that jump labels use asm() blocks, which are opaque to the
> compiler, and so it is not guaranteed that codegen will be better than

I actually spent a lot of time looking at the codegen on a few
platforms.

> > > - Why do we need to enable this static key so early?
> >
> > We don't need to enable it especially early. I've now sent three
> > different approaches for deferring it until later and you suggested one.
> > The first of mine is kind of ugly (checking static_key_initialized and
> > such at different points).  Your suggested one after that did the same
> > but deferred into crng_reseed(), which I'm not a fan of. My second one
> > is this patch, which is flawed for the reason you pointed out. But
> > perhaps my third one is the right amount of simple and okay? That's the
> > one I linked up top, [1]. Let me know what you think of that.
> >
> > My motivation for not wanting to defer it is that if the arch solution
> > winds up being easy and straight forward (as it was for arm64), then it
> > would be nice to not need to clutter up random.c as a result.
> 
> If clutter is a concern, how about getting rid of the
> execute_in_process_context() dance, and just use a late_initcall()
> instead?

As I already explained in [1], this does not work. If the order is
(A)(B), then all this will happen *after* the late init call.

[1] https://lore.kernel.org/lkml/Yp8oOH+9V336LrLk@zx2c4.com/

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Phil,

On Tue, Jun 07, 2022 at 01:19:41PM +0100, Phil Elwell wrote:
> After a trivial merge conflict (into 5.15) was resolved, this patch allows the 
> downstream kernel with CONFIG_RANDOM_TRUST_BOOTLOADER=y to boot cleanly. 
> However, it does delay the initialisation of crng significantly compared to my
> hack.
> 
> With v2:
> [    1.981493] bcm2835-rng 3f104000.rng: hwrng registered
> [   12.549190] random: crng init done

Thanks. It's because the patch is buggy :)

v3 coming right up.

Jason

Re: [PATCH v2] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Phil,

On Tue, Jun 07, 2022 at 01:19:41PM +0100, Phil Elwell wrote:
> After a trivial merge conflict (into 5.15) was resolved, this patch allows the 
> downstream kernel with CONFIG_RANDOM_TRUST_BOOTLOADER=y to boot cleanly. 
> However, it does delay the initialisation of crng significantly compared to my
> hack.
> 
> With v2:
> [    1.981493] bcm2835-rng 3f104000.rng: hwrng registered
> [   12.549190] random: crng init done

Thanks. It's because the patch is buggy :)

v3 coming right up.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Stephen reported that a static key warning splat appears during early
boot on systems that credit randomness from device trees that contain an
"rng-seed" property, because because setup_machine_fdt() is called
before jump_label_init() during setup_arch():

 static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
 WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
 pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : static_key_enable_cpuslocked+0xb0/0xb8
 lr : static_key_enable_cpuslocked+0xb0/0xb8
 sp : ffffffe51c393cf0
 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
 Call trace:
  static_key_enable_cpuslocked+0xb0/0xb8
  static_key_enable+0x2c/0x40
  crng_set_ready+0x24/0x30
  execute_in_process_context+0x80/0x90
  _credit_init_bits+0x100/0x154
  add_bootloader_randomness+0x64/0x78
  early_init_dt_scan_chosen+0x140/0x184
  early_init_dt_scan_nodes+0x28/0x4c
  early_init_dt_scan+0x40/0x44
  setup_machine_fdt+0x7c/0x120
  setup_arch+0x74/0x1d8
  start_kernel+0x84/0x44c
  __primary_switched+0xc0/0xc8
 ---[ end trace 0000000000000000 ]---
 random: crng init done
 Machine model: Google Lazor (rev1 - 2) with LTE

A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
Initialize jump labels before setup_machine_fdt()"). But it appears that
fixing it on other platforms might not be so trivial. And in the past
there have been problems related to add_bootloader_randomness() being
called too early in boot for what it needed.

This patch defers all entropy crediting until random_init(), where we
can be sure that all facilities we need are up and running. It still
mixes the actual seed immediately, so that it's maximally useful, but
the crediting doesn't happen until later.

This also has the positive effect of allowing rng_has_arch_random() to
reflect bootloader randomness.

Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 29 +++++++++++++----------------
 include/linux/random.h |  2 +-
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..ebfec7cbd5e1 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
  **********************************************************************/
 
 static bool used_arch_random;
-static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
-static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
+static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static size_t bootloader_seed_bits __initdata;
 static int __init parse_trust_cpu(char *arg)
 {
 	return kstrtobool(arg, &trust_cpu);
@@ -776,7 +777,7 @@ static struct notifier_block pm_notifier = { .notifier_call = random_pm_notifica
 int __init random_init(const char *command_line)
 {
 	ktime_t now = ktime_get_real();
-	unsigned int i, arch_bytes;
+	unsigned int i, arch_bits;
 	unsigned long entropy;
 
 #if defined(LATENT_ENTROPY_PLUGIN)
@@ -784,12 +785,12 @@ int __init random_init(const char *command_line)
 	_mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed));
 #endif
 
-	for (i = 0, arch_bytes = BLAKE2S_BLOCK_SIZE;
+	for (i = 0, arch_bits = BLAKE2S_BLOCK_SIZE * 8;
 	     i < BLAKE2S_BLOCK_SIZE; i += sizeof(entropy)) {
 		if (!arch_get_random_seed_long_early(&entropy) &&
 		    !arch_get_random_long_early(&entropy)) {
 			entropy = random_get_entropy();
-			arch_bytes -= sizeof(entropy);
+			arch_bits -= sizeof(entropy) * 8;
 		}
 		_mix_pool_bytes(&entropy, sizeof(entropy));
 	}
@@ -800,9 +801,10 @@ int __init random_init(const char *command_line)
 
 	if (crng_ready())
 		crng_reseed();
-	else if (trust_cpu)
-		_credit_init_bits(arch_bytes * 8);
-	used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
+	else
+		_credit_init_bits((trust_cpu ? arch_bits : 0) +
+				  (trust_bootloader ? bootloader_seed_bits : 0));
+	used_arch_random = arch_bits + bootloader_seed_bits >= POOL_READY_BITS;
 
 	WARN_ON(register_pm_notifier(&pm_notifier));
 
@@ -861,17 +863,12 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
 }
 EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
 
-/*
- * Handle random seed passed by bootloader, and credit it if
- * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
- */
-void __cold add_bootloader_randomness(const void *buf, size_t len)
+/*  Handle random seed passed by bootloader during init. */
+void __init add_bootloader_randomness(const void *buf, size_t len)
 {
 	mix_pool_bytes(buf, len);
-	if (trust_bootloader)
-		credit_init_bits(len * 8);
+	bootloader_seed_bits = len * 8;
 }
-EXPORT_SYMBOL_GPL(add_bootloader_randomness);
 
 #if IS_ENABLED(CONFIG_VMGENID)
 static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
diff --git a/include/linux/random.h b/include/linux/random.h
index fae0c84027fd..223b4bd584e7 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -13,7 +13,7 @@
 struct notifier_block;
 
 void add_device_randomness(const void *buf, size_t len);
-void add_bootloader_randomness(const void *buf, size_t len);
+void __init add_bootloader_randomness(const void *buf, size_t len);
 void add_input_randomness(unsigned int type, unsigned int code,
 			  unsigned int value) __latent_entropy;
 void add_interrupt_randomness(int irq) __latent_entropy;
-- 
2.35.1

[PATCH v3] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Stephen reported that a static key warning splat appears during early
boot on systems that credit randomness from device trees that contain an
"rng-seed" property, because because setup_machine_fdt() is called
before jump_label_init() during setup_arch():

 static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
 WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
 pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : static_key_enable_cpuslocked+0xb0/0xb8
 lr : static_key_enable_cpuslocked+0xb0/0xb8
 sp : ffffffe51c393cf0
 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
 Call trace:
  static_key_enable_cpuslocked+0xb0/0xb8
  static_key_enable+0x2c/0x40
  crng_set_ready+0x24/0x30
  execute_in_process_context+0x80/0x90
  _credit_init_bits+0x100/0x154
  add_bootloader_randomness+0x64/0x78
  early_init_dt_scan_chosen+0x140/0x184
  early_init_dt_scan_nodes+0x28/0x4c
  early_init_dt_scan+0x40/0x44
  setup_machine_fdt+0x7c/0x120
  setup_arch+0x74/0x1d8
  start_kernel+0x84/0x44c
  __primary_switched+0xc0/0xc8
 ---[ end trace 0000000000000000 ]---
 random: crng init done
 Machine model: Google Lazor (rev1 - 2) with LTE

A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
Initialize jump labels before setup_machine_fdt()"). But it appears that
fixing it on other platforms might not be so trivial. And in the past
there have been problems related to add_bootloader_randomness() being
called too early in boot for what it needed.

This patch defers all entropy crediting until random_init(), where we
can be sure that all facilities we need are up and running. It still
mixes the actual seed immediately, so that it's maximally useful, but
the crediting doesn't happen until later.

This also has the positive effect of allowing rng_has_arch_random() to
reflect bootloader randomness.

Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Reported-by: Stephen Boyd <swboyd@chromium.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Elwell <phil@raspberrypi.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 29 +++++++++++++----------------
 include/linux/random.h |  2 +-
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4862d4d3ec49..ebfec7cbd5e1 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
  **********************************************************************/
 
 static bool used_arch_random;
-static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
-static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
+static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
+static size_t bootloader_seed_bits __initdata;
 static int __init parse_trust_cpu(char *arg)
 {
 	return kstrtobool(arg, &trust_cpu);
@@ -776,7 +777,7 @@ static struct notifier_block pm_notifier = { .notifier_call = random_pm_notifica
 int __init random_init(const char *command_line)
 {
 	ktime_t now = ktime_get_real();
-	unsigned int i, arch_bytes;
+	unsigned int i, arch_bits;
 	unsigned long entropy;
 
 #if defined(LATENT_ENTROPY_PLUGIN)
@@ -784,12 +785,12 @@ int __init random_init(const char *command_line)
 	_mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed));
 #endif
 
-	for (i = 0, arch_bytes = BLAKE2S_BLOCK_SIZE;
+	for (i = 0, arch_bits = BLAKE2S_BLOCK_SIZE * 8;
 	     i < BLAKE2S_BLOCK_SIZE; i += sizeof(entropy)) {
 		if (!arch_get_random_seed_long_early(&entropy) &&
 		    !arch_get_random_long_early(&entropy)) {
 			entropy = random_get_entropy();
-			arch_bytes -= sizeof(entropy);
+			arch_bits -= sizeof(entropy) * 8;
 		}
 		_mix_pool_bytes(&entropy, sizeof(entropy));
 	}
@@ -800,9 +801,10 @@ int __init random_init(const char *command_line)
 
 	if (crng_ready())
 		crng_reseed();
-	else if (trust_cpu)
-		_credit_init_bits(arch_bytes * 8);
-	used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
+	else
+		_credit_init_bits((trust_cpu ? arch_bits : 0) +
+				  (trust_bootloader ? bootloader_seed_bits : 0));
+	used_arch_random = arch_bits + bootloader_seed_bits >= POOL_READY_BITS;
 
 	WARN_ON(register_pm_notifier(&pm_notifier));
 
@@ -861,17 +863,12 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
 }
 EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
 
-/*
- * Handle random seed passed by bootloader, and credit it if
- * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
- */
-void __cold add_bootloader_randomness(const void *buf, size_t len)
+/*  Handle random seed passed by bootloader during init. */
+void __init add_bootloader_randomness(const void *buf, size_t len)
 {
 	mix_pool_bytes(buf, len);
-	if (trust_bootloader)
-		credit_init_bits(len * 8);
+	bootloader_seed_bits = len * 8;
 }
-EXPORT_SYMBOL_GPL(add_bootloader_randomness);
 
 #if IS_ENABLED(CONFIG_VMGENID)
 static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
diff --git a/include/linux/random.h b/include/linux/random.h
index fae0c84027fd..223b4bd584e7 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -13,7 +13,7 @@
 struct notifier_block;
 
 void add_device_randomness(const void *buf, size_t len);
-void add_bootloader_randomness(const void *buf, size_t len);
+void __init add_bootloader_randomness(const void *buf, size_t len);
 void add_input_randomness(unsigned int type, unsigned int code,
 			  unsigned int value) __latent_entropy;
 void add_interrupt_randomness(int irq) __latent_entropy;
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] random: defer crediting bootloader randomness to random_init()

[Phil Elwell]

Hi Jason,

On 07/06/2022 13:44, Jason A. Donenfeld wrote:
> Stephen reported that a static key warning splat appears during early
> boot on systems that credit randomness from device trees that contain an
> "rng-seed" property, because because setup_machine_fdt() is called
> before jump_label_init() during setup_arch():
> 
>   static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
>   WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
>   pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : static_key_enable_cpuslocked+0xb0/0xb8
>   lr : static_key_enable_cpuslocked+0xb0/0xb8
>   sp : ffffffe51c393cf0
>   x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
>   x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
>   x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
>   x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
>   x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
>   x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
>   x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>   x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
>   x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
>   x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
>   Call trace:
>    static_key_enable_cpuslocked+0xb0/0xb8
>    static_key_enable+0x2c/0x40
>    crng_set_ready+0x24/0x30
>    execute_in_process_context+0x80/0x90
>    _credit_init_bits+0x100/0x154
>    add_bootloader_randomness+0x64/0x78
>    early_init_dt_scan_chosen+0x140/0x184
>    early_init_dt_scan_nodes+0x28/0x4c
>    early_init_dt_scan+0x40/0x44
>    setup_machine_fdt+0x7c/0x120
>    setup_arch+0x74/0x1d8
>    start_kernel+0x84/0x44c
>    __primary_switched+0xc0/0xc8
>   ---[ end trace 0000000000000000 ]---
>   random: crng init done
>   Machine model: Google Lazor (rev1 - 2) with LTE
> 
> A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
> Initialize jump labels before setup_machine_fdt()"). But it appears that
> fixing it on other platforms might not be so trivial. And in the past
> there have been problems related to add_bootloader_randomness() being
> called too early in boot for what it needed.
> 
> This patch defers all entropy crediting until random_init(), where we
> can be sure that all facilities we need are up and running. It still
> mixes the actual seed immediately, so that it's maximally useful, but
> the crediting doesn't happen until later.
> 
> This also has the positive effect of allowing rng_has_arch_random() to
> reflect bootloader randomness.
> 
> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> Reported-by: Stephen Boyd <swboyd@chromium.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   drivers/char/random.c  | 29 +++++++++++++----------------
>   include/linux/random.h |  2 +-
>   2 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 4862d4d3ec49..ebfec7cbd5e1 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
>    **********************************************************************/
>   
>   static bool used_arch_random;
> -static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> -static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> +static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static size_t bootloader_seed_bits __initdata;
>   static int __init parse_trust_cpu(char *arg)
>   {
>   	return kstrtobool(arg, &trust_cpu);
> @@ -776,7 +777,7 @@ static struct notifier_block pm_notifier = { .notifier_call = random_pm_notifica
>   int __init random_init(const char *command_line)
>   {
>   	ktime_t now = ktime_get_real();
> -	unsigned int i, arch_bytes;
> +	unsigned int i, arch_bits;
>   	unsigned long entropy;
>   
>   #if defined(LATENT_ENTROPY_PLUGIN)
> @@ -784,12 +785,12 @@ int __init random_init(const char *command_line)
>   	_mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed));
>   #endif
>   
> -	for (i = 0, arch_bytes = BLAKE2S_BLOCK_SIZE;
> +	for (i = 0, arch_bits = BLAKE2S_BLOCK_SIZE * 8;
>   	     i < BLAKE2S_BLOCK_SIZE; i += sizeof(entropy)) {
>   		if (!arch_get_random_seed_long_early(&entropy) &&
>   		    !arch_get_random_long_early(&entropy)) {
>   			entropy = random_get_entropy();
> -			arch_bytes -= sizeof(entropy);
> +			arch_bits -= sizeof(entropy) * 8;
>   		}
>   		_mix_pool_bytes(&entropy, sizeof(entropy));
>   	}
> @@ -800,9 +801,10 @@ int __init random_init(const char *command_line)
>   
>   	if (crng_ready())
>   		crng_reseed();
> -	else if (trust_cpu)
> -		_credit_init_bits(arch_bytes * 8);
> -	used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
> +	else
> +		_credit_init_bits((trust_cpu ? arch_bits : 0) +
> +				  (trust_bootloader ? bootloader_seed_bits : 0));
> +	used_arch_random = arch_bits + bootloader_seed_bits >= POOL_READY_BITS;
>   
>   	WARN_ON(register_pm_notifier(&pm_notifier));
>   
> @@ -861,17 +863,12 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
>   }
>   EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>   
> -/*
> - * Handle random seed passed by bootloader, and credit it if
> - * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
> - */
> -void __cold add_bootloader_randomness(const void *buf, size_t len)
> +/*  Handle random seed passed by bootloader during init. */
> +void __init add_bootloader_randomness(const void *buf, size_t len)
>   {
>   	mix_pool_bytes(buf, len);
> -	if (trust_bootloader)
> -		credit_init_bits(len * 8);
> +	bootloader_seed_bits = len * 8;
>   }
> -EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>   
>   #if IS_ENABLED(CONFIG_VMGENID)
>   static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fae0c84027fd..223b4bd584e7 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -13,7 +13,7 @@
>   struct notifier_block;
>   
>   void add_device_randomness(const void *buf, size_t len);
> -void add_bootloader_randomness(const void *buf, size_t len);
> +void __init add_bootloader_randomness(const void *buf, size_t len);
>   void add_input_randomness(unsigned int type, unsigned int code,
>   			  unsigned int value) __latent_entropy;
>   void add_interrupt_randomness(int irq) __latent_entropy;

v3 boots cleanly and with the firmware-provided entropy available from much earlier:

[    0.056431] random: crng init done
[    1.991312] bcm2835-rng 3f104000.rng: hwrng registered

For what it's worth, on rpi-5.15.y and 5.15.45:

     Tested-by: Phil Elwell <phil@raspberrypi.com>

Note that the patch needs some adjustment for 5.15.

Phil

Re: [PATCH v3] random: defer crediting bootloader randomness to random_init()

[Phil Elwell]

Hi Jason,

On 07/06/2022 13:44, Jason A. Donenfeld wrote:
> Stephen reported that a static key warning splat appears during early
> boot on systems that credit randomness from device trees that contain an
> "rng-seed" property, because because setup_machine_fdt() is called
> before jump_label_init() during setup_arch():
> 
>   static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init()
>   WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff
>   pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : static_key_enable_cpuslocked+0xb0/0xb8
>   lr : static_key_enable_cpuslocked+0xb0/0xb8
>   sp : ffffffe51c393cf0
>   x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10
>   x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000
>   x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000
>   x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020
>   x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708
>   x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000
>   x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
>   x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027
>   x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05
>   x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065
>   Call trace:
>    static_key_enable_cpuslocked+0xb0/0xb8
>    static_key_enable+0x2c/0x40
>    crng_set_ready+0x24/0x30
>    execute_in_process_context+0x80/0x90
>    _credit_init_bits+0x100/0x154
>    add_bootloader_randomness+0x64/0x78
>    early_init_dt_scan_chosen+0x140/0x184
>    early_init_dt_scan_nodes+0x28/0x4c
>    early_init_dt_scan+0x40/0x44
>    setup_machine_fdt+0x7c/0x120
>    setup_arch+0x74/0x1d8
>    start_kernel+0x84/0x44c
>    __primary_switched+0xc0/0xc8
>   ---[ end trace 0000000000000000 ]---
>   random: crng init done
>   Machine model: Google Lazor (rev1 - 2) with LTE
> 
> A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64:
> Initialize jump labels before setup_machine_fdt()"). But it appears that
> fixing it on other platforms might not be so trivial. And in the past
> there have been problems related to add_bootloader_randomness() being
> called too early in boot for what it needed.
> 
> This patch defers all entropy crediting until random_init(), where we
> can be sure that all facilities we need are up and running. It still
> mixes the actual seed immediately, so that it's maximally useful, but
> the crediting doesn't happen until later.
> 
> This also has the positive effect of allowing rng_has_arch_random() to
> reflect bootloader randomness.
> 
> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> Reported-by: Stephen Boyd <swboyd@chromium.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   drivers/char/random.c  | 29 +++++++++++++----------------
>   include/linux/random.h |  2 +-
>   2 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 4862d4d3ec49..ebfec7cbd5e1 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -725,8 +725,9 @@ static void __cold _credit_init_bits(size_t bits)
>    **********************************************************************/
>   
>   static bool used_arch_random;
> -static bool trust_cpu __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> -static bool trust_bootloader __ro_after_init = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static bool trust_cpu __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_CPU);
> +static bool trust_bootloader __initdata = IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER);
> +static size_t bootloader_seed_bits __initdata;
>   static int __init parse_trust_cpu(char *arg)
>   {
>   	return kstrtobool(arg, &trust_cpu);
> @@ -776,7 +777,7 @@ static struct notifier_block pm_notifier = { .notifier_call = random_pm_notifica
>   int __init random_init(const char *command_line)
>   {
>   	ktime_t now = ktime_get_real();
> -	unsigned int i, arch_bytes;
> +	unsigned int i, arch_bits;
>   	unsigned long entropy;
>   
>   #if defined(LATENT_ENTROPY_PLUGIN)
> @@ -784,12 +785,12 @@ int __init random_init(const char *command_line)
>   	_mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed));
>   #endif
>   
> -	for (i = 0, arch_bytes = BLAKE2S_BLOCK_SIZE;
> +	for (i = 0, arch_bits = BLAKE2S_BLOCK_SIZE * 8;
>   	     i < BLAKE2S_BLOCK_SIZE; i += sizeof(entropy)) {
>   		if (!arch_get_random_seed_long_early(&entropy) &&
>   		    !arch_get_random_long_early(&entropy)) {
>   			entropy = random_get_entropy();
> -			arch_bytes -= sizeof(entropy);
> +			arch_bits -= sizeof(entropy) * 8;
>   		}
>   		_mix_pool_bytes(&entropy, sizeof(entropy));
>   	}
> @@ -800,9 +801,10 @@ int __init random_init(const char *command_line)
>   
>   	if (crng_ready())
>   		crng_reseed();
> -	else if (trust_cpu)
> -		_credit_init_bits(arch_bytes * 8);
> -	used_arch_random = arch_bytes * 8 >= POOL_READY_BITS;
> +	else
> +		_credit_init_bits((trust_cpu ? arch_bits : 0) +
> +				  (trust_bootloader ? bootloader_seed_bits : 0));
> +	used_arch_random = arch_bits + bootloader_seed_bits >= POOL_READY_BITS;
>   
>   	WARN_ON(register_pm_notifier(&pm_notifier));
>   
> @@ -861,17 +863,12 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy)
>   }
>   EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
>   
> -/*
> - * Handle random seed passed by bootloader, and credit it if
> - * CONFIG_RANDOM_TRUST_BOOTLOADER is set.
> - */
> -void __cold add_bootloader_randomness(const void *buf, size_t len)
> +/*  Handle random seed passed by bootloader during init. */
> +void __init add_bootloader_randomness(const void *buf, size_t len)
>   {
>   	mix_pool_bytes(buf, len);
> -	if (trust_bootloader)
> -		credit_init_bits(len * 8);
> +	bootloader_seed_bits = len * 8;
>   }
> -EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>   
>   #if IS_ENABLED(CONFIG_VMGENID)
>   static BLOCKING_NOTIFIER_HEAD(vmfork_chain);
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fae0c84027fd..223b4bd584e7 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -13,7 +13,7 @@
>   struct notifier_block;
>   
>   void add_device_randomness(const void *buf, size_t len);
> -void add_bootloader_randomness(const void *buf, size_t len);
> +void __init add_bootloader_randomness(const void *buf, size_t len);
>   void add_input_randomness(unsigned int type, unsigned int code,
>   			  unsigned int value) __latent_entropy;
>   void add_interrupt_randomness(int irq) __latent_entropy;

v3 boots cleanly and with the firmware-provided entropy available from much earlier:

[    0.056431] random: crng init done
[    1.991312] bcm2835-rng 3f104000.rng: hwrng registered

For what it's worth, on rpi-5.15.y and 5.15.45:

     Tested-by: Phil Elwell <phil@raspberrypi.com>

Note that the patch needs some adjustment for 5.15.

Phil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Phil,

And this one also works, I assume?

https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/

Jason

Re: [PATCH v3] random: defer crediting bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Phil,

And this one also works, I assume?

https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] random: defer crediting bootloader randomness to random_init()

[Phil Elwell]

Jason,

On 07/06/2022 14:51, Jason A. Donenfeld wrote:
> Hi Phil,
> 
> And this one also works, I assume?
> 
> https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/

Using the metrics "earlier is better2 and "shorter is better", that patch wins 
on both counts:

With 5.15.45 + CONFIG_RANDOM_TRUST_BOOTLOADER=y:
[    0.000000] random: crng init done
[    1.985290] bcm2835-rng 3f104000.rng: hwrng registered

With rpi-5.15.y:
[    0.000000] random: crng init done
[    1.969552] bcm2835-rng 3f104000.rng: hwrng registered

No splats in either, of course.

Phil

Re: [PATCH v3] random: defer crediting bootloader randomness to random_init()

[Phil Elwell]

Jason,

On 07/06/2022 14:51, Jason A. Donenfeld wrote:
> Hi Phil,
> 
> And this one also works, I assume?
> 
> https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/

Using the metrics "earlier is better2 and "shorter is better", that patch wins 
on both counts:

With 5.15.45 + CONFIG_RANDOM_TRUST_BOOTLOADER=y:
[    0.000000] random: crng init done
[    1.985290] bcm2835-rng 3f104000.rng: hwrng registered

With rpi-5.15.y:
[    0.000000] random: crng init done
[    1.969552] bcm2835-rng 3f104000.rng: hwrng registered

No splats in either, of course.

Phil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 14:22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> On Tue, Jun 07, 2022 at 02:15:27PM +0200, Ard Biesheuvel wrote:
> > Note that jump labels use asm() blocks, which are opaque to the
> > compiler, and so it is not guaranteed that codegen will be better than
>
> I actually spent a lot of time looking at the codegen on a few
> platforms.
>

I did a quick experiment on ThunderX2 with the following program

#include <stdio.h>
#include <stdlib.h>
#include <sys/random.h>

static unsigned char buf[16];

int main(void)
{
  for (int i = 0; i < 1000000; i++) {
    if (getrandom(buf, sizeof(buf),
        GRND_RANDOM | GRND_NONBLOCK) < sizeof(buf)) {
          fprintf(stderr, "getrandom() error!\n");
          exit(-1);
    }
  }
  return 0;
}

both with and without your revert patch of f5bda35fba615ac applied
onto v5.19-rc1, the results are below (Cortex-A57 @ 2 GHz):

############## WITH STATIC BRANCH

ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            906.01 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                40      page-faults               #    0.044 K/sec
     1,812,010,034      cycles                    #    2.000 GHz
     1,944,549,733      instructions              #    1.07  insn per cycle
   <not supported>      branches
         2,014,408      branch-misses

       0.906695576 seconds time elapsed

       0.140334000 seconds user
       0.765871000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            918.37 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                37      page-faults               #    0.040 K/sec
     1,836,733,451      cycles                    #    2.000 GHz
     1,944,572,442      instructions              #    1.06  insn per cycle
   <not supported>      branches
         3,012,020      branch-misses

       0.919027080 seconds time elapsed

       0.159721000 seconds user
       0.758718000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            902.06 msec task-clock                #    0.999 CPUs utilized
                 1      context-switches          #    0.001 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                39      page-faults               #    0.043 K/sec
     1,804,103,600      cycles                    #    2.000 GHz
     1,944,563,889      instructions              #    1.08  insn per cycle
   <not supported>      branches
         1,956,027      branch-misses

       0.902793520 seconds time elapsed

       0.172464000 seconds user
       0.729822000 seconds sys

############## WITHOUT STATIC BRANCH

ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            924.79 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                39      page-faults               #    0.042 K/sec
     1,849,568,681      cycles                    #    2.000 GHz
     1,950,584,115      instructions              #    1.05  insn per cycle
   <not supported>      branches
         4,012,227      branch-misses

       0.925500832 seconds time elapsed

       0.204227000 seconds user
       0.720739000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            933.06 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                39      page-faults               #    0.042 K/sec
     1,866,097,571      cycles                    #    2.000 GHz
     1,950,574,944      instructions              #    1.05  insn per cycle
   <not supported>      branches
         3,984,008      branch-misses

       0.933798032 seconds time elapsed

       0.323067000 seconds user
       0.610271000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            913.16 msec task-clock                #    0.999 CPUs utilized
                 1      context-switches          #    0.001 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                37      page-faults               #    0.041 K/sec
     1,826,308,530      cycles                    #    2.000 GHz
     1,950,559,902      instructions              #    1.07  insn per cycle
   <not supported>      branches
         2,231,050      branch-misses

       0.913874672 seconds time elapsed

       0.164228000 seconds user
       0.749157000 seconds sys

So that's a 0.3% reduction (in terms of actual instructions executed)
in a synthetic benchmark that does nothing but call getrandom() in a
tight loop.

In other words, I think the static branch solves a problem that does not exist.

> > > > - Why do we need to enable this static key so early?
> > >
> > > We don't need to enable it especially early. I've now sent three
> > > different approaches for deferring it until later and you suggested one.
> > > The first of mine is kind of ugly (checking static_key_initialized and
> > > such at different points).  Your suggested one after that did the same
> > > but deferred into crng_reseed(), which I'm not a fan of. My second one
> > > is this patch, which is flawed for the reason you pointed out. But
> > > perhaps my third one is the right amount of simple and okay? That's the
> > > one I linked up top, [1]. Let me know what you think of that.
> > >
> > > My motivation for not wanting to defer it is that if the arch solution
> > > winds up being easy and straight forward (as it was for arm64), then it
> > > would be nice to not need to clutter up random.c as a result.
> >
> > If clutter is a concern, how about getting rid of the
> > execute_in_process_context() dance, and just use a late_initcall()
> > instead?
>
> As I already explained in [1], this does not work. If the order is
> (A)(B), then all this will happen *after* the late init call.
>
> [1] https://lore.kernel.org/lkml/Yp8oOH+9V336LrLk@zx2c4.com/
>

Yeah, I guess finding the right spot is tricky. The more reason just
to drop it altogether.

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 14:22, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> On Tue, Jun 07, 2022 at 02:15:27PM +0200, Ard Biesheuvel wrote:
> > Note that jump labels use asm() blocks, which are opaque to the
> > compiler, and so it is not guaranteed that codegen will be better than
>
> I actually spent a lot of time looking at the codegen on a few
> platforms.
>

I did a quick experiment on ThunderX2 with the following program

#include <stdio.h>
#include <stdlib.h>
#include <sys/random.h>

static unsigned char buf[16];

int main(void)
{
  for (int i = 0; i < 1000000; i++) {
    if (getrandom(buf, sizeof(buf),
        GRND_RANDOM | GRND_NONBLOCK) < sizeof(buf)) {
          fprintf(stderr, "getrandom() error!\n");
          exit(-1);
    }
  }
  return 0;
}

both with and without your revert patch of f5bda35fba615ac applied
onto v5.19-rc1, the results are below (Cortex-A57 @ 2 GHz):

############## WITH STATIC BRANCH

ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            906.01 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                40      page-faults               #    0.044 K/sec
     1,812,010,034      cycles                    #    2.000 GHz
     1,944,549,733      instructions              #    1.07  insn per cycle
   <not supported>      branches
         2,014,408      branch-misses

       0.906695576 seconds time elapsed

       0.140334000 seconds user
       0.765871000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            918.37 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                37      page-faults               #    0.040 K/sec
     1,836,733,451      cycles                    #    2.000 GHz
     1,944,572,442      instructions              #    1.06  insn per cycle
   <not supported>      branches
         3,012,020      branch-misses

       0.919027080 seconds time elapsed

       0.159721000 seconds user
       0.758718000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            902.06 msec task-clock                #    0.999 CPUs utilized
                 1      context-switches          #    0.001 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                39      page-faults               #    0.043 K/sec
     1,804,103,600      cycles                    #    2.000 GHz
     1,944,563,889      instructions              #    1.08  insn per cycle
   <not supported>      branches
         1,956,027      branch-misses

       0.902793520 seconds time elapsed

       0.172464000 seconds user
       0.729822000 seconds sys

############## WITHOUT STATIC BRANCH

ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            924.79 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                39      page-faults               #    0.042 K/sec
     1,849,568,681      cycles                    #    2.000 GHz
     1,950,584,115      instructions              #    1.05  insn per cycle
   <not supported>      branches
         4,012,227      branch-misses

       0.925500832 seconds time elapsed

       0.204227000 seconds user
       0.720739000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            933.06 msec task-clock                #    0.999 CPUs utilized
                 2      context-switches          #    0.002 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                39      page-faults               #    0.042 K/sec
     1,866,097,571      cycles                    #    2.000 GHz
     1,950,574,944      instructions              #    1.05  insn per cycle
   <not supported>      branches
         3,984,008      branch-misses

       0.933798032 seconds time elapsed

       0.323067000 seconds user
       0.610271000 seconds sys


ard@dogfood:~$ perf_5.10 stat ./rnd

 Performance counter stats for './rnd':

            913.16 msec task-clock                #    0.999 CPUs utilized
                 1      context-switches          #    0.001 K/sec
                 0      cpu-migrations            #    0.000 K/sec
                37      page-faults               #    0.041 K/sec
     1,826,308,530      cycles                    #    2.000 GHz
     1,950,559,902      instructions              #    1.07  insn per cycle
   <not supported>      branches
         2,231,050      branch-misses

       0.913874672 seconds time elapsed

       0.164228000 seconds user
       0.749157000 seconds sys

So that's a 0.3% reduction (in terms of actual instructions executed)
in a synthetic benchmark that does nothing but call getrandom() in a
tight loop.

In other words, I think the static branch solves a problem that does not exist.

> > > > - Why do we need to enable this static key so early?
> > >
> > > We don't need to enable it especially early. I've now sent three
> > > different approaches for deferring it until later and you suggested one.
> > > The first of mine is kind of ugly (checking static_key_initialized and
> > > such at different points).  Your suggested one after that did the same
> > > but deferred into crng_reseed(), which I'm not a fan of. My second one
> > > is this patch, which is flawed for the reason you pointed out. But
> > > perhaps my third one is the right amount of simple and okay? That's the
> > > one I linked up top, [1]. Let me know what you think of that.
> > >
> > > My motivation for not wanting to defer it is that if the arch solution
> > > winds up being easy and straight forward (as it was for arm64), then it
> > > would be nice to not need to clutter up random.c as a result.
> >
> > If clutter is a concern, how about getting rid of the
> > execute_in_process_context() dance, and just use a late_initcall()
> > instead?
>
> As I already explained in [1], this does not work. If the order is
> (A)(B), then all this will happen *after* the late init call.
>
> [1] https://lore.kernel.org/lkml/Yp8oOH+9V336LrLk@zx2c4.com/
>

Yeah, I guess finding the right spot is tricky. The more reason just
to drop it altogether.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Ard,

On Tue, Jun 07, 2022 at 04:19:26PM +0200, Ard Biesheuvel wrote:
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/random.h>
> 
> static unsigned char buf[16];
> 
> int main(void)
> {
>   for (int i = 0; i < 1000000; i++) {
>     if (getrandom(buf, sizeof(buf),
>         GRND_RANDOM | GRND_NONBLOCK) < sizeof(buf)) {
>           fprintf(stderr, "getrandom() error!\n");
>           exit(-1);
>     }
>   }
>   return 0;
> }

I'm actually more worried about the random input flow than the random
output flow and branch misprediction. But more generally, I'd just like
to keep that code as cold as possible after crng init. It's code that's
only used in that one phase and then never again. It can be entirely
disabled.

Anyway, we've got a few solutions now to pick from on the random.c side
of things. I'm going to investigate the arm32 situation next. And then
we'll see what it all looks like.

Jason

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Jason A. Donenfeld]

Hi Ard,

On Tue, Jun 07, 2022 at 04:19:26PM +0200, Ard Biesheuvel wrote:
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/random.h>
> 
> static unsigned char buf[16];
> 
> int main(void)
> {
>   for (int i = 0; i < 1000000; i++) {
>     if (getrandom(buf, sizeof(buf),
>         GRND_RANDOM | GRND_NONBLOCK) < sizeof(buf)) {
>           fprintf(stderr, "getrandom() error!\n");
>           exit(-1);
>     }
>   }
>   return 0;
> }

I'm actually more worried about the random input flow than the random
output flow and branch misprediction. But more generally, I'd just like
to keep that code as cold as possible after crng init. It's code that's
only used in that one phase and then never again. It can be entirely
disabled.

Anyway, we've got a few solutions now to pick from on the random.c side
of things. I'm going to investigate the arm32 situation next. And then
we'll see what it all looks like.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 16:48, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> On Tue, Jun 07, 2022 at 04:19:26PM +0200, Ard Biesheuvel wrote:
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <sys/random.h>
> >
> > static unsigned char buf[16];
> >
> > int main(void)
> > {
> >   for (int i = 0; i < 1000000; i++) {
> >     if (getrandom(buf, sizeof(buf),
> >         GRND_RANDOM | GRND_NONBLOCK) < sizeof(buf)) {
> >           fprintf(stderr, "getrandom() error!\n");
> >           exit(-1);
> >     }
> >   }
> >   return 0;
> > }
>
> I'm actually more worried about the random input flow than the random
> output flow and branch misprediction. But more generally, I'd just like
> to keep that code as cold as possible after crng init. It's code that's
> only used in that one phase and then never again. It can be entirely
> disabled.
>
> Anyway, we've got a few solutions now to pick from on the random.c side
> of things. I'm going to investigate the arm32 situation next. And then
> we'll see what it all looks like.
>

Sure.

It would be helpful if some other folks could chime in as well?

Re: [PATCH] random: defer use of bootloader randomness to random_init()

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 16:48, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> On Tue, Jun 07, 2022 at 04:19:26PM +0200, Ard Biesheuvel wrote:
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <sys/random.h>
> >
> > static unsigned char buf[16];
> >
> > int main(void)
> > {
> >   for (int i = 0; i < 1000000; i++) {
> >     if (getrandom(buf, sizeof(buf),
> >         GRND_RANDOM | GRND_NONBLOCK) < sizeof(buf)) {
> >           fprintf(stderr, "getrandom() error!\n");
> >           exit(-1);
> >     }
> >   }
> >   return 0;
> > }
>
> I'm actually more worried about the random input flow than the random
> output flow and branch misprediction. But more generally, I'd just like
> to keep that code as cold as possible after crng init. It's code that's
> only used in that one phase and then never again. It can be entirely
> disabled.
>
> Anyway, we've got a few solutions now to pick from on the random.c side
> of things. I'm going to investigate the arm32 situation next. And then
> we'll see what it all looks like.
>

Sure.

It would be helpful if some other folks could chime in as well?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel