linux-kernel - Re: [PATCH] ARM: initialize jump labels before setup_machine

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: <CAMj1kXGo=Jr2mZJ2ryh7Z7FgoXSBttAyX=yMhBnikK6vCXnRGg@mail.gmail.com>
Date:   Fri, 3 Jun 2022 10:06:02 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Russell King <linux@...linux.org.uk>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Stephen Boyd <swboyd@...omium.org>,
        "# 3.4.x" <stable@...r.kernel.org>
Subject: Re: [PATCH] ARM: initialize jump labels before setup_machine_fdt()

On Fri, 3 Jun 2022 at 09:51, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> (+ Greg)
>
> On Fri, 3 Jun 2022 at 09:37, Jason A. Donenfeld <Jason@...c4.com> wrote:
> >
> > Hi Ard,
> >
> > On 6/3/22, Ard Biesheuvel <ardb@...nel.org> wrote:
> > > On Thu, 2 Jun 2022 at 23:22, Jason A. Donenfeld <Jason@...c4.com> wrote:
> > >>
> > >> Stephen reported that a static key warning splat appears during early
> > >> boot on arm64 systems that credit randomness from device trees that
> > >> contain an "rng-seed" property, because setup_machine_fdt() is called
> > >> before jump_label_init() during setup_arch(), which was fixed by
> > >> 73e2d827a501 ("arm64: Initialize jump labels before
> > >> setup_machine_fdt()").
> > >>
> > >> Upon cursory inspection, the same basic issue appears to apply to arm32
> > >> as well. In this case, we reorder setup_arch() to do things in the same
> > >> order as is now the case on arm64.
> > >>
> > >> Reported-by: Stephen Boyd <swboyd@...omium.org>
> > >> Cc: Catalin Marinas <catalin.marinas@....com>
> > >> Cc: Ard Biesheuvel <ardb@...nel.org>
> > >> Cc: stable@...r.kernel.org
> > >> Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
> > >
> > > Wouldn't it be better to defer the
> > > static_branch_enable(&crng_is_ready) call to later in the boot (e.g.,
> > > using an initcall()), rather than going around 'fixing' fragile,
> > > working early boot code across multiple architectures?
> >
> > Yes, maybe. It's just more book keeping that's potentially
> > unnecessary, which would be nice to avoid. I wrote a patch for this
> > before, but it wasn't beautiful. And Catalin got a pretty easy arm64
> > patch queued up sufficiently fast that I figured this was better.
> >
>
> The problem is that your original patch was already backported as far
> back as 5.10, and so this fix will need to be as well.
>
> Playing with the code that runs before the call to setup_machine_fdt()
> is risky because it implies that issues that are introduced are likely
> to limit the ability of the system to generate diagnostic output of
> any kind, given that the device tree is what describes the topology of
> the system to the kernel. Before that, there is no serial or graphical
> console, and the only way to figure out what goes on is to connect a
> JTAG debugger and single step through the code or dump the contents of
> __log_buf[].
>
> I like the /dev/random work you have been doing but as you know, I was
> skeptical about the need to backport all of that work to -stable, and
> it appears my skepticism may have been justified.
>
> The patch in question is an unquantified performance optimization,
> which means it does not meet the stable-kernel-rules.rst criteria, but
> it was backported nonetheless. Now, we are in a situation where we
> must refactor very early boot code to address a regression introduced
> by that backport.
>
> > >
> > >> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> > >> ---
> > >>  arch/arm/kernel/setup.c | 12 ++++++------
> > >>  1 file changed, 6 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> > >> index 1e8a50a97edf..ef40d9f5d5a7 100644
> > >> --- a/arch/arm/kernel/setup.c
> > >> +++ b/arch/arm/kernel/setup.c
> > >> @@ -1097,10 +1097,15 @@ void __init setup_arch(char **cmdline_p)
> > >>         const struct machine_desc *mdesc = NULL;
> > >>         void *atags_vaddr = NULL;
> > >>
> > >> +       setup_initial_init_mm(_text, _etext, _edata, _end);
> > >> +       setup_processor();
> > >> +       early_fixmap_init();
> > >> +       early_ioremap_init();
> > >> +       jump_label_init();
> > >> +
> > >
> > > Is it really necessary to reorder all these calls? What does
> > > jump_label_init() actually need?
> >
> > I'm not quite sure, but it matched how arm64 does things now. Was
> > hoping somebody with deep arm32 knowledge (e.g. you or rmk) would be
> > able to eyeball that to confirm.
> >
>
> As far as I can tell, the early patching code on ARM does not rely on
> the early fixmap code. Did you try just moving jump_label_init()
> earlier in the function?
>

The below seems to work too:

--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -1101,6 +1101,7 @@ void __init setup_arch(char **cmdline_p)
                atags_vaddr = FDT_VIRT_BASE(__atags_pointer);

        setup_processor();
+       jump_label_init();
        if (atags_vaddr) {
                mdesc = setup_machine_fdt(atags_vaddr);
                if (mdesc)