Message-Id: <26E1A34A-CC24-4D6B-9C12-782C837A91E1@jrtc27.com>
Date: Wed, 16 Oct 2024 16:30:20 +0100
From: Jessica Clarke <jrtc27@...c27.com>
To: Alexandre Ghiti <alex@...ti.fr>
Cc: Alexandre Ghiti <alexghiti@...osinc.com>,
 Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>,
 Albert Ou <aou@...s.berkeley.edu>,
 Heiko Stuebner <heiko@...ech.de>,
 Björn Töpel <bjorn@...osinc.com>,
 linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org,
 Jason Montleon <jmontleo@...hat.com>,
 stable@...r.kernel.org,
 Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH -fixes] riscv: Do not use fortify in early code

On 16 Oct 2024, at 12:26, Alexandre Ghiti <alex@...ti.fr> wrote:
> 
> Hi Jessica,
> 
> On 16/10/2024 00:04, Jessica Clarke wrote:
>> On 9 Oct 2024, at 08:27, Alexandre Ghiti <alexghiti@...osinc.com> wrote:
>>> Early code designates the code executed when the MMU is not yet enabled,
>>> and this comes with some limitations (see
>>> Documentation/arch/riscv/boot.rst, section "Pre-MMU execution").
>>> 
>>> FORTIFY_SOURCE must be disabled then since it can trigger kernel panics
>>> as reported in [1].
>>> 
>>> Reported-by: Jason Montleon <jmontleo@...hat.com>
>>> Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1]
>>> Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head")
>>> Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line")
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
>> Is the problem in [1] not just that the early boot path uses memcpy on
>> the result of ALT_OLD_PTR, which is a wildly out-of-bounds pointer from
>> the compiler’s perspective? If so, it would seem better to use
>> unsafe_memcpy for that one call site rather than use the big
>> __NO_FORTIFY hammer, surely?
> 
> 
> Not sure why fortify complains here, and I have just seen that I forgot to cc Kees (done now).
> 
> 
>> 
>> Presumably the non-early path is just as bad to the compiler, but works
>> because patch_text_nosync isn’t instrumented, so that would just align
>> the two.
>> 
>> Getting the implementation to not be silent on failure during early
>> boot would also be a good idea, but it’s surely better to have
>> FORTIFY_SOURCE enabled with no output for positives than disable the
>> checking in the first place and risk uncaught corruption.
> 
> 
> I'm not sure to follow: you propose to use unsafe_memcpy() instead of disabling fortify entirely, so we would not get any warning in case of failure anyway right?

Yes, but no. The point is to disable it only for the problematic
function call, not the entire file, so any other fortifiable function
calls that exist now or in the future in that file don’t get it
unnecessarily disabled too.

> Or do you propose to modify the fortify code to somehow print a warning? If the latter, it's hard this soon in the boot process (where the mmu is disabled) to make sure that the printing warning path does not try to access any virtual address (which is why the boot failed in the first place) but maybe Kees has an idea.

Not for this patch, just observing it would be nice to have.

> And I believe that enabling fortify and using the unsafe_*() variants is error-prone since we'd have to make sure that all the "fortified" functions used in that code use the unsafe_*() variants.

I mean, that’s how all these things work, normally?

Jess

> So to me, it's way easier in terms of maintenance to just disabling fortify.
> 
> Thanks,
> 
> Alex
> 
> 
>> Jess
>> 
>> 
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists