Re: [PATCH] random: split initialization into early arch step and later non-arch step

Re: [PATCH] random: split initialization into early arch step and later non-arch step

[Kees Cook]

On Mon, Sep 26, 2022 at 06:03:32PM +0200, Jason A. Donenfeld wrote:
> The full RNG initialization relies on some timestamps, made possible
> with general functions like time_init() and timekeeping_init(). However,
> these are only available rather late in initialization. Meanwhile, other
> things, such as memory allocator functions, make use of the RNG much
> earlier.
> 
> So split RNG initialization into two phases. We can give arch randomness
> very early on, and then later, after timekeeping and such are available,
> initialize the rest.
> 
> This ensures that, for example, slabs are properly randomized if RDRAND
> is available. Another positive consequence is that on systems with
> RDRAND, running with CONFIG_WARN_ALL_UNSEEDED_RANDOM=y results in no
> warnings at all.

Nice! I like it. Notes below...

> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> I intend to take this through the random.git tree, but reviews/acks
> would be appreciated, given that I'm touching init/main.c.
> 
>  drivers/char/random.c  | 47 ++++++++++++++++++++++++------------------
>  include/linux/random.h |  3 ++-
>  init/main.c            | 17 +++++++--------
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index a90d96f4b3bb..1cb53495e8f7 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -772,18 +772,13 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
>  static struct notifier_block pm_notifier = { .notifier_call = random_pm_notification };
>  
>  /*
> - * The first collection of entropy occurs at system boot while interrupts
> - * are still turned off. Here we push in latent entropy, RDSEED, a timestamp,
> - * utsname(), and the command line. Depending on the above configuration knob,
> - * RDSEED may be considered sufficient for initialization. Note that much
> - * earlier setup may already have pushed entropy into the input pool by the
> - * time we get here.
> + * This is called extremely early, before time keeping functionality is
> + * available, but arch randomness is. Interrupts are not yet enabled.
>   */
> -int __init random_init(const char *command_line)
> +void __init random_init_early(const char *command_line)
>  {
> -	ktime_t now = ktime_get_real();
> -	size_t i, longs, arch_bits;
>  	unsigned long entropy[BLAKE2S_BLOCK_SIZE / sizeof(long)];
> +	size_t i, longs, arch_bits;
>  
>  #if defined(LATENT_ENTROPY_PLUGIN)
>  	static const u8 compiletime_seed[BLAKE2S_BLOCK_SIZE] __initconst __latent_entropy;
> @@ -803,34 +798,46 @@ int __init random_init(const char *command_line)
>  			i += longs;
>  			continue;
>  		}

Can find a way to get efi_get_random_bytes() in here too? (As a separate
patch.) I don't see where that actually happens anywhere currently,
and we should have it available at this point in the boot, yes?

> -		entropy[0] = random_get_entropy();
> -		_mix_pool_bytes(entropy, sizeof(*entropy));
>  		arch_bits -= sizeof(*entropy) * 8;
>  		++i;
>  	}
> -	_mix_pool_bytes(&now, sizeof(now));
> -	_mix_pool_bytes(utsname(), sizeof(*(utsname())));

Hm, can't we keep utsname in the early half by using init_utsname() ?

> +
>  	_mix_pool_bytes(command_line, strlen(command_line));
> +
> +	if (trust_cpu)
> +		credit_init_bits(arch_bits);
> +}
> +
> +/*
> + * This is called a little bit after the prior function, and now there is
> + * access to timestamps counters. Interrupts are not yet enabled.
> + */
> +void __init random_init(void)
> +{
> +	unsigned long entropy = random_get_entropy();
> +	ktime_t now = ktime_get_real();
> +
> +	_mix_pool_bytes(utsname(), sizeof(*(utsname())));

(...and then obviously don't repeat it here.)

> +	_mix_pool_bytes(&now, sizeof(now));
> +	_mix_pool_bytes(&entropy, sizeof(entropy));
>  	add_latent_entropy();
>  
>  	/*
> -	 * If we were initialized by the bootloader before jump labels are
> -	 * initialized, then we should enable the static branch here, where
> +	 * If we were initialized by the cpu or bootloader before jump labels
> +	 * are initialized, then we should enable the static branch here, where
>  	 * it's guaranteed that jump labels have been initialized.
>  	 */
>  	if (!static_branch_likely(&crng_is_ready) && crng_init >= CRNG_READY)
>  		crng_set_ready(NULL);
>  
> +	/* Reseed if already seeded by earlier phases. */
>  	if (crng_ready())
>  		crng_reseed();
> -	else if (trust_cpu)
> -		_credit_init_bits(arch_bits);
>  
>  	WARN_ON(register_pm_notifier(&pm_notifier));
>  
> -	WARN(!random_get_entropy(), "Missing cycle counter and fallback timer; RNG "
> -				    "entropy collection will consequently suffer.");
> -	return 0;
> +	WARN(!entropy, "Missing cycle counter and fallback timer; RNG "
> +		       "entropy collection will consequently suffer.");
>  }
>  
>  /*
> diff --git a/include/linux/random.h b/include/linux/random.h
> index 3fec206487f6..a9e6e16f9774 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -72,7 +72,8 @@ static inline unsigned long get_random_canary(void)
>  	return get_random_long() & CANARY_MASK;
>  }
>  
> -int __init random_init(const char *command_line);
> +void __init random_init_early(const char *command_line);
> +void __init random_init(void);
>  bool rng_is_initialized(void);
>  int wait_for_random_bytes(void);
>  
> diff --git a/init/main.c b/init/main.c
> index 1fe7942f5d4a..611886430e28 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -976,6 +976,9 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>  		parse_args("Setting extra init args", extra_init_args,
>  			   NULL, 0, -1, -1, NULL, set_init_arg);
>  
> +	/* Call before any memory or allocators are initialized */

Maybe for greater clarity:

	/* Pre-time-keeping entropy collection before allocator init. */

> +	random_init_early(command_line);
> +
>  	/*
>  	 * These use large bootmem allocations and must precede
>  	 * kmem_cache_init()
> @@ -1035,17 +1038,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>  	hrtimers_init();
>  	softirq_init();
>  	timekeeping_init();
> -	kfence_init();
>  	time_init();

Was there a reason kfence_init() was happening before time_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 random_init()
> -	 * - time_init() for making random_get_entropy() work on some platforms
> -	 * - random_init() to initialize the RNG from from early entropy sources
> -	 */
> -	random_init(command_line);
> +	/* This must be after timekeeping is initialized */
> +	random_init();
> +
> +	/* These make use of the initialized randomness */

I'd clarify this more:

	/* These make use of the fully initialized randomness entropy. */

> +	kfence_init();
>  	boot_init_stack_canary();
>  
>  	perf_event_init();
> -- 
> 2.37.3
> 

-- 
Kees Cook

Re: [PATCH] random: split initialization into early arch step and later non-arch step

[Jason A. Donenfeld]

On Mon, Sep 26, 2022 at 8:22 PM Kees Cook <keescook@chromium.org> wrote:
> Can find a way to get efi_get_random_bytes() in here too? (As a separate
> patch.) I don't see where that actually happens anywhere currently,
> and we should have it available at this point in the boot, yes?

No, absolutely not. That is not how EFI works. EFI gets its seed to
random.c much earlier by way of add_bootloader_randomness().

> > -             entropy[0] = random_get_entropy();
> > -             _mix_pool_bytes(entropy, sizeof(*entropy));
> >               arch_bits -= sizeof(*entropy) * 8;
> >               ++i;
> >       }
> > -     _mix_pool_bytes(&now, sizeof(now));
> > -     _mix_pool_bytes(utsname(), sizeof(*(utsname())));
>
> Hm, can't we keep utsname in the early half by using init_utsname() ?

Yes, we could maybe *change* to using init_utsname if we wanted. That
seems kind of different though. So I'd prefer that to be a different
patch, which would require looking at the interaction with early
hostname setting and such. If you want to do that work, I'd certainly
welcome the patch.

> > @@ -976,6 +976,9 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> >               parse_args("Setting extra init args", extra_init_args,
> >                          NULL, 0, -1, -1, NULL, set_init_arg);
> >
> > +     /* Call before any memory or allocators are initialized */
>
> Maybe for greater clarity:
>
>         /* Pre-time-keeping entropy collection before allocator init. */

Will do.

>
> > +     random_init_early(command_line);
> > +
> >       /*
> >        * These use large bootmem allocations and must precede
> >        * kmem_cache_init()
> > @@ -1035,17 +1038,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> >       hrtimers_init();
> >       softirq_init();
> >       timekeeping_init();
> > -     kfence_init();
> >       time_init();
>
> Was there a reason kfence_init() was happening before time_init()?

Historically there was, I think, because random_init() used to make
weird allocations. But that's been gone for a while. At this point
it's a mistake, and removing it allows me to do this:

https://groups.google.com/g/kasan-dev/c/jhExcSv_Pj4

>
> >
> > -     /*
> > -      * 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 random_init()
> > -      * - time_init() for making random_get_entropy() work on some platforms
> > -      * - random_init() to initialize the RNG from from early entropy sources
> > -      */
> > -     random_init(command_line);
> > +     /* This must be after timekeeping is initialized */
> > +     random_init();
> > +
> > +     /* These make use of the initialized randomness */
>
> I'd clarify this more:
>
>         /* These make use of the fully initialized randomness entropy. */

Okay will do.

Jason

Re: [PATCH] random: split initialization into early arch step and later non-arch step

[Kees Cook]

On Mon, Sep 26, 2022 at 08:52:39PM +0200, Jason A. Donenfeld wrote:
> On Mon, Sep 26, 2022 at 8:22 PM Kees Cook <keescook@chromium.org> wrote:
> > Can find a way to get efi_get_random_bytes() in here too? (As a separate
> > patch.) I don't see where that actually happens anywhere currently,
> > and we should have it available at this point in the boot, yes?
> 
> No, absolutely not. That is not how EFI works. EFI gets its seed to
> random.c much earlier by way of add_bootloader_randomness().

Ah! Okay, so, yes, it _does_ get entropy in there, just via a path I
didn't see?

> 
> > > -             entropy[0] = random_get_entropy();
> > > -             _mix_pool_bytes(entropy, sizeof(*entropy));
> > >               arch_bits -= sizeof(*entropy) * 8;
> > >               ++i;
> > >       }
> > > -     _mix_pool_bytes(&now, sizeof(now));
> > > -     _mix_pool_bytes(utsname(), sizeof(*(utsname())));
> >
> > Hm, can't we keep utsname in the early half by using init_utsname() ?
> 
> Yes, we could maybe *change* to using init_utsname if we wanted. That
> seems kind of different though. So I'd prefer that to be a different
> patch, which would require looking at the interaction with early
> hostname setting and such. If you want to do that work, I'd certainly
> welcome the patch.

Er, isn't that _WAY_ later? Like, hostname isn't set until sysctls up
and running, etc. I haven't actually verified 100% but it looks like
current->utsname is exactly init_utsname currently.

But if not, I guess it could just get added in both places. I'd be nice
to keep kernel version as part of the pre-time-keeping entropy stuffing.

> > Was there a reason kfence_init() was happening before time_init()?
> 
> Historically there was, I think, because random_init() used to make
> weird allocations. But that's been gone for a while. At this point
> it's a mistake, and removing it allows me to do this:
> 
> https://groups.google.com/g/kasan-dev/c/jhExcSv_Pj4

Cool. Is that true for all the -stable releases this is aimed at?

Anyway, just to repeat before: yay! I really like seeing this split up.
:)

-- 
Kees Cook

Re: [PATCH] random: split initialization into early arch step and later non-arch step

[Jason A. Donenfeld]

On Tue, Sep 27, 2022 at 5:23 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Sep 26, 2022 at 08:52:39PM +0200, Jason A. Donenfeld wrote:
> > On Mon, Sep 26, 2022 at 8:22 PM Kees Cook <keescook@chromium.org> wrote:
> > > Can find a way to get efi_get_random_bytes() in here too? (As a separate
> > > patch.) I don't see where that actually happens anywhere currently,
> > > and we should have it available at this point in the boot, yes?
> >
> > No, absolutely not. That is not how EFI works. EFI gets its seed to
> > random.c much earlier by way of add_bootloader_randomness().
>
> Ah! Okay, so, yes, it _does_ get entropy in there, just via a path I
> didn't see?

Yes.

> > Yes, we could maybe *change* to using init_utsname if we wanted. That
> > seems kind of different though. So I'd prefer that to be a different
> > patch, which would require looking at the interaction with early
> > hostname setting and such. If you want to do that work, I'd certainly
> > welcome the patch.
>
> Er, isn't that _WAY_ later? Like, hostname isn't set until sysctls up
> and running, etc. I haven't actually verified 100% but it looks like
> current->utsname is exactly init_utsname currently.

If init_utsname()==utsname() and all is fine, can you please send a
patch atop random.git adjusting that and explaining why? I would
happily take such a patch. If your suspicion is correct, it would make
a most welcome improvement.

> > > Was there a reason kfence_init() was happening before time_init()?
> >
> > Historically there was, I think, because random_init() used to make
> > weird allocations. But that's been gone for a while. At this point
> > it's a mistake, and removing it allows me to do this:
> >
> > https://groups.google.com/g/kasan-dev/c/jhExcSv_Pj4
>
> Cool. Is that true for all the -stable releases this is aimed at?

Yes.

Though I'll likely drop the stable@ tag for this, and instead visit
backporting it later in 6.1's cycle, or even after. There's no need to
rush it, and this is an area that has been historically temperamental.

Jason

Re: [PATCH] random: split initialization into early arch step and later non-arch step

[Jason A. Donenfeld]

On Tue, Sep 27, 2022 at 10:34:16AM +0200, Jason A. Donenfeld wrote:
> > > Yes, we could maybe *change* to using init_utsname if we wanted. That
> > > seems kind of different though. So I'd prefer that to be a different
> > > patch, which would require looking at the interaction with early
> > > hostname setting and such. If you want to do that work, I'd certainly
> > > welcome the patch.
> >
> > Er, isn't that _WAY_ later? Like, hostname isn't set until sysctls up
> > and running, etc. I haven't actually verified 100% but it looks like
> > current->utsname is exactly init_utsname currently.
> 
> If init_utsname()==utsname() and all is fine, can you please send a
> patch atop random.git adjusting that and explaining why? I would
> happily take such a patch. If your suspicion is correct, it would make
> a most welcome improvement.

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