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

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	Alexander Potapenko <glider@google.com>,
	Marco Elver <elver@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	kasan-dev@googlegroups.com, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] random: split initialization into early arch step and later non-arch step
Date: Mon, 26 Sep 2022 20:52:39 +0200	[thread overview]
Message-ID: <CAHmME9pFDzyKJd5ixyB9E05jkZvHShFimbiQsGTcdQO1E5R0QQ@mail.gmail.com> (raw)
In-Reply-To: <202209261105.9C6AEEEE1@keescook>

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