random: Move rand_initialize() earlier

PATCH In review Kees Cook 3 participants 11 messages Oct 11, 2018 – Apr 24, 2019

Subsystems (2)

Real time clock (RTC) Random number driver

lore

Share & tools

lore

Kees Cook

author

2018-10-11 22:54 UTC #0 lore raw

PATCH 1/1 random: Move rand_initialize() earlier

Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:

random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0

Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.

Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools during setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).

Signed-off-by Kees Cook <[email protected]>

---
Alternatively, ktime_get_real() could get mixed into the pools after timekeeping_init(), and rand_initialize() could be run MUCH early, like after setup_arch()...

---
 drivers/char/random.c  |  5 ++---
 include/linux/random.h |  1 +
 init/main.c            | 21 ++++++++++++++-------
 3 files changed, 17 insertions(+), 10 deletions(-)

drivers/char/random.c +2 -3

index c75b6cdf0053..deff1aa4d000 100644
@@ -1784,7 +1784,7 @@ EXPORT_SYMBOL(get_random_bytes_arch);
  * data into the pool to prepare it for use. The pool is not cleared
  * as that can only decrease the entropy in the pool.
  */
-static void init_std_data(struct entropy_store *r)
+static void __init init_std_data(struct entropy_store *r)
 {
 	int i;
 	ktime_t now = ktime_get_real();

@@ -1811,7 +1811,7 @@ static void init_std_data(struct entropy_store *r)
  * take care not to overwrite the precious per platform data
  * we were given.
  */
-static int rand_initialize(void)
+int __init rand_initialize(void)
 {
 	init_std_data(&input_pool);
 	init_std_data(&blocking_pool);

@@ -1823,7 +1823,6 @@ static int rand_initialize(void)
 	}
 	return 0;
 }
-early_initcall(rand_initialize);
 
 #ifdef CONFIG_BLOCK
 void rand_initialize_disk(struct gendisk *disk)

include/linux/random.h +1 -0

index 445a0ea4ff49..13aeaf5a4bd4 100644
@@ -36,6 +36,7 @@ extern void add_interrupt_randomness(int irq, int irq_flags) __latent_entropy;
 
 extern void get_random_bytes(void *buf, int nbytes);
 extern int wait_for_random_bytes(void);
+extern int __init rand_initialize(void);
 extern bool rng_is_initialized(void);
 extern int add_random_ready_callback(struct random_ready_callback *rdy);
 extern void del_random_ready_callback(struct random_ready_callback *rdy);

init/main.c +14 -7

index 18f8f0140fa0..e2b073bf846f 100644
@@ -550,13 +550,6 @@ asmlinkage __visible void __init start_kernel(void)
 	page_address_init();
 	pr_notice("%s", linux_banner);
 	setup_arch(&command_line);
-	/*
-	 * Set up the the initial canary and entropy after arch
-	 * and after adding latent and command line entropy.
-	 */
-	add_latent_entropy();
-	add_device_randomness(command_line, strlen(command_line));
-	boot_init_stack_canary();
 	mm_init_cpumask(&init_mm);
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();

@@ -641,6 +634,20 @@ asmlinkage __visible void __init start_kernel(void)
 	hrtimers_init();
 	softirq_init();
 	timekeeping_init();
+
+	/*
+	 * For best initial stack canary entropy, prepare it after:
+	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
+	 * - timekeeping_init() for ktime entropy used in rand_initialize()
+	 * - rand_initialize() to get any arch-specific entropy like RDRAND
+	 * - add_latent_entropy() to get any latent entropy
+	 * - adding command line entropy
+	 */
+	rand_initialize();
+	add_latent_entropy();
+	add_device_randomness(command_line, strlen(command_line));
+	boot_init_stack_canary();
+
 	time_init();
 	printk_safe_init();
 	perf_event_init();

Signature · 6 lines

2.17.1


-- 
Kees Cook
Pixel Security

Arnd Bergmann

2018-10-12 08:42 UTC #1 lore raw

Quoted 16 lines · Kees Cook

Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.
Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools during setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).

Quoted 5 lines

Signed-off-by Kees Cook <[email protected]>
---
Alternatively, ktime_get_real() could get mixed into the pools after timekeeping_init(), and rand_initialize() could be run MUCH early, like after setup_arch()...

I wonder if mixing in ktime_get_real() is flawed to start with:
This depends on read_persistent_clock64() actually returning a meaningful time, but in many cases it does not; x86 being a notable exception.

We have three ways of setting the initial time:

* At early boot using read_persistent_clock64(). This may read the time from the hypervisor (if any), and on some older architectures that have a custom RTC abstraction, it reads from the RTC directly. We generally move away from the RTC method in favor of the proper rtc class interfaces (see below)

* At late_initcall time, in rtc_hctosys(). If an RTC driver has been loaded successfully at this time, it will be read from there. We also move away from this.

* From user space, which reads the RTC time or NTP, and then sets the system time from that.

The latter two end up in do_settimeofday64(), but there is no mixing in of the time into the random seed that I can see, and it's not clear whether there should be (it can be called with arbitrary times from user space, provided CAP_SYS_TIME permissions).

Arnd

Theodore Y. Ts'o

maintainer

2018-10-12 14:44 UTC #2 lore raw

Quoted 18 lines · Arnd Bergmann

I wonder if mixing in ktime_get_real() is flawed to start with:
This depends on read_persistent_clock64() actually returning a meaningful time, but in many cases it does not; x86 being a notable exception.
We have three ways of setting the initial time:
* At early boot using read_persistent_clock64(). This may read the time from the hypervisor (if any), and on some older architectures that have a custom RTC abstraction, it reads from the RTC directly. We generally move away from the RTC method in favor of the proper rtc class interfaces (see below)
* At late_initcall time, in rtc_hctosys(). If an RTC driver has been loaded successfully at this time, it will be read from there. We also move away from this.

Quoted 8 lines

* From user space, which reads the RTC time or NTP, and then sets the system time from that.
The latter two end up in do_settimeofday64(), but there is no mixing in of the time into the random seed that I can see, and it's not clear whether there should be (it can be called with arbitrary times from user space, provided CAP_SYS_TIME permissions).

I think it adds some real value (although perhaps small) in the first two cases. In all of the cases, though, since we don't add any entropy credit, it doesn't hurt to mix it in --- this is the same argument why it's fine that /dev/[u]random can be world-writeable. Mixing in user-controlled data is harmless, and if it's not visible to a remote attacker, it can be helpful.

- Ted

Theodore Y. Ts'o

maintainer

2018-10-12 14:29 UTC #3 lore raw

In reply to Kees Cook

Quoted 10 lines · Kees Cook

Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.

Quoted 6 lines

Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools durting setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).
Signed-off-by Kees Cook <[email protected]>

This seems reasonable to me. Were you hoping to get this in for -rc8? It looks sane, and I don't see any _obvious_ unintended consequences of such a change, but it's rather late in the development cycle, and it isn't regression fix.

My druthers would be wait until -rc2, as a minor low-risk improvement that goes in after the merge window, although some people are of the belief that -rc2 should be a strict feature freeze delimiter. My personal opinion is no major feature changes except during the merge window, and we go into bug-fix only mode after -rc4, and by -rc6 it's regression and major security fixes only. (This is however not a universally held opinion; see the "bug-introducing patches" thread which Sasha started on the ksummit-discuss thread.)

- Ted

Kees Cook

author

2018-10-12 14:45 UTC #4 lore raw

Quoted 16 lines · Theodore Y. Ts'o

Quoted 15 lines · Kees Cook
Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.
Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools durting setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).

Quoted 6 lines

Quoted 1 line
Signed-off-by Kees Cook <[email protected]>
This seems reasonable to me. Were you hoping to get this in for -rc8? It looks sane, and I don't see any _obvious_ unintended consequences of such a change, but it's rather late in the development cycle, and it isn't regression fix.

Yeah, for sure. I didn't mean this for 4.19. I assumed -next, and likely further changes based on discussion, etc etc.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Kees Cook

author

2019-04-16 03:46 UTC #5 lore raw

Quoted 20 lines · Kees Cook

Quoted 18 lines · Theodore Y. Ts'o
Quoted 16 lines · Kees Cook
Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.
Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools durting setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).
Signed-off-by Kees Cook <[email protected]>

Quoted 7 lines

Quoted 4 lines
This seems reasonable to me. Were you hoping to get this in for -rc8? It looks sane, and I don't see any _obvious_ unintended consequences of such a change, but it's rather late in the development cycle, and it isn't regression fix.
Yeah, for sure. I didn't mean this for 4.19. I assumed -next, and likely further changes based on discussion, etc etc.

Hi Ted! Did this get lost? This has come up again, and I don't see it having landed anywhere yet. Can you take this?

Signature · 1 line

Kees Cook

Theodore Ts'o

maintainer

2019-04-16 16:11 UTC #6 lore raw

Quoted 19 lines · Kees Cook

Quoted 18 lines · Kees Cook
Quoted 16 lines · Theodore Y. Ts'o
Quoted 15 lines · Kees Cook
Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.
Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools durting setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).

Quoted 4 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Signed-off-by Kees Cook <[email protected]>
Hi Ted! Did this get lost? This has come up again, and I don't see it having landed anywhere yet. Can you take this?

Yeah, sorry, it got lost; my bad. I'll take it and push it out at the next merge window.

- Ted

Kees Cook

author

2019-04-16 23:00 UTC #7 lore raw

Quoted 21 lines · Theodore Ts'o

Quoted 19 lines · Kees Cook
Quoted 18 lines · Kees Cook
Quoted 16 lines · Theodore Y. Ts'o
Quoted 15 lines · Kees Cook
Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.
Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools durting setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Signed-off-by Kees Cook <[email protected]>
Hi Ted! Did this get lost? This has come up again, and I don't see it having landed anywhere yet. Can you take this?
Yeah, sorry, it got lost; my bad. I'll take it and push it out at the next merge window.

Awesome; thanks!

Signature · 1 line

Kees Cook

Kees Cook

author

2019-04-23 22:21 UTC #8 lore raw

In reply to Theodore Ts'o

Quoted 21 lines · Theodore Ts'o

Quoted 19 lines · Kees Cook
Quoted 18 lines · Kees Cook
Quoted 16 lines · Theodore Y. Ts'o
Quoted 15 lines · Kees Cook
Right now rand_initialize() is run as an early_initcall(), but it only depends on timekeeping_init() (for mixing ktime_get_real() into the pools). However, the call to boot_init_stack_canary() for stack canary initialization runs earlier, which triggers a warning at boot:
random: get_random_bytes called from start_kernel+0x357/0x548 with crng_init=0
Instead, this moves rand_initialize() to after timekeeping_init(), and moves canary initialization here as well.
Note that this warning may still remain for machines that do not have UEFI RNG support (which initializes the RNG pools durting setup_arch()), or for x86 machines without RDRAND (or booting without "random.trust=on" or CONFIG_RANDOM_TRUST_CPU=y).

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Signed-off-by Kees Cook <[email protected]>
Hi Ted! Did this get lost? This has come up again, and I don't see it having landed anywhere yet. Can you take this?
Yeah, sorry, it got lost; my bad. I'll take it and push it out at the next merge window.

Gentle poke. :) I still don't see this in -next...

Signature · 1 line

Kees Cook

Theodore Ts'o

maintainer

2019-04-24 22:11 UTC #9 lore raw

Quoted 4 lines · Kees Cook

Quoted 2 lines
Yeah, sorry, it got lost; my bad. I'll take it and push it out at the next merge window.
Gentle poke. :) I still don't see this in -next...

Oops, sorry, I pushed it out to the master branch, and I forgot that
the -next branch was pulling from the dev branch. Fixed.

- Ted

Kees Cook

author

2019-04-24 23:15 UTC #10 lore raw

Quoted 9 lines · Theodore Ts'o

Quoted 4 lines · Kees Cook
Quoted 2 lines
Yeah, sorry, it got lost; my bad. I'll take it and push it out at the next merge window.
Gentle poke. :) I still don't see this in -next...
Oops, sorry, I pushed it out to the master branch, and I forgot that
the -next branch was pulling from the dev branch. Fixed.

Ah-ha! Thanks. :)

Signature · 1 line

Kees Cook