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]

Date:   Fri, 3 Jun 2022 09:37:13 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Ard Biesheuvel <ardb@...nel.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()

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.

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

Jason