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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202209262017.D751DDC38F@keescook>
Date:   Mon, 26 Sep 2022 20:23:57 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Alexander Potapenko <glider@...gle.com>,
        Marco Elver <elver@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] random: split initialization into early arch step and
 later non-arch step

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@...omium.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