Message-ID: <eb7354f6-98bd-45c6-a616-733637848a14@ghiti.fr>
Date: Tue, 1 Oct 2024 16:28:43 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Jason Montleon <jmontleo@...hat.com>, Kees Cook <kees@...nel.org>
Cc: linux-hardening@...r.kernel.org,
 Linux regressions mailing list <regressions@...ts.linux.dev>,
 linux-riscv@...ts.infradead.org
Subject: Re: [REGRESSION][BISECTED] Cannot boot Lichee Pi 4A with
 FORTIFY_SOURCE enabled

Hi Jason,

On 25/09/2024 16:32, Jason Montleon wrote:
> On Tue, Sep 24, 2024 at 1:36 PM Kees Cook <kees@...nel.org> wrote:
>>
>>
>> On September 24, 2024 8:58:57 AM PDT, Jason Montleon <jmontleo@...hat.com> wrote:
>>> On Sun, Sep 22, 2024 at 6:38 PM Kees Cook <kees@...nel.org> wrote:
>>>> Can you try this patch? It should avoid using the "WARN" infrastructure
>>>> (if that is the source of blocking boot), but should still provide some
>>>> detail about what tripped it up (via the "regular" pr_*() logging). And
>>>> if it boots, can you look at the log to find what it reports for the
>>>> "Wanted to write ..." line(s)?
>>>>
>>>> Thanks!
>>>>
>>>> -Kees
>>>>
>>>>
>>>> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
>>>> index 0d99bf11d260..469a3439959d 100644
>>>> --- a/include/linux/fortify-string.h
>>>> +++ b/include/linux/fortify-string.h
>>>> @@ -549,7 +549,8 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
>>>>                                           const size_t q_size,
>>>>                                           const size_t p_size_field,
>>>>                                           const size_t q_size_field,
>>>> -                                        const u8 func)
>>>> +                                        const u8 func,
>>>> +                                        const char *field)
>>>>   {
>>>>          if (__builtin_constant_p(size)) {
>>>>                  /*
>>>> @@ -610,8 +611,13 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
>>>>           * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
>>>>           */
>>>>          if (p_size_field != SIZE_MAX &&
>>>> -           p_size != p_size_field && p_size_field < size)
>>>> -               return true;
>>>> +           p_size != p_size_field && p_size_field < size) {
>>>> +               if (p_size_field == 0)
>>>> +                       pr_warn("Wanted to write %zu to a 0-sized destination: %s\n",
>>>> +                               size, field);
>>>> +               else
>>>> +                       return true;
>>>> +       }
>>>>
>>>>          return false;
>>>>   }
>>>> @@ -625,7 +631,8 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
>>>>          const size_t __q_size_field = (q_size_field);                   \
>>>>          fortify_warn_once(fortify_memcpy_chk(__fortify_size, __p_size,  \
>>>>                                       __q_size, __p_size_field,          \
>>>> -                                    __q_size_field, FORTIFY_FUNC_ ##op), \
>>>> +                                    __q_size_field, FORTIFY_FUNC_ ##op,\
>>>> +                                    #p " at " FILE_LINE), \
>>>>                    #op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \
>>>>                    __fortify_size,                                       \
>>>>                    "field \"" #p "\" at " FILE_LINE,                     \
>>>>
>>>>
>>>> --
>>>> Kees Cook
>>>>
>>> Hi Kees,
>>> Sorry for the delay in responding. Your patch also caused the system
>>> to hang booting, but I took it as inspiration to try some things.
>>> TL;DR I see these two print several times:
>>>
>>> Wanted to write 8 to a 0-sized destination: oldptr at
>>> arch/riscv/errata/thead/errata.c:185
>>> Wanted to write 28 to a 0-sized destination: oldptr at
>>> arch/riscv/errata/thead/errata.c:185
>>>
>>> Since I suspected CONFIG_ERRATA_THEAD_MAE which relies on
>>> CONFIG_RISCV_ALTERNATIVE_EARLY I suspected it is just too early for
>>> pr_*, etc. I tried trace_printk but that didn't produce any traces,
>>> and then I found a comment which makes me think we're too early for
>>> that too.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/kernel/alternative.c?h=v6.11#n214
>>>
>>> I didn't want to give up so I wrote a simple module to figure out how
>>> to use sbi_ecall and then incorporated that idea into your patch,
>>> although even that was not straightforward:
>>> https://gist.github.com/jmontleon/f3536ad70dc92bed992139ad5b266d5c
>> Oh nice work! Probably something riscv-specific needs to be wired up to the printk routines to do what you have there until printk is fully available  But that's a separate issue, I guess.
>>
>>> Full output: https://gist.github.com/jmontleon/823facff38e28b717f153e38adcfd7fe
>> Does the system boot with your patch?
> Yes. It boots fully. It also booted with trace_printk, it just didn't
> produce a trace, which again I think is expected at that stage. It
> seems as long as there are no 'normal' print attempts, pr_()*, etc. it
> works.


Hmmm I don't see what in your patch would fix your issue.

I sent a patch 
(https://lore.kernel.org/linux-riscv/20240829165048.49756-1-alexghiti@rivosinc.com/) 
that was merged in v6.11-rc7 which fixes something similar, could that 
be what fixed your problem?

The problem with this early code is that the MMU is not enabled yet and 
if your kernel is relocatable, the relocations are set "virtually" and 
then without the MMU, a call to an external function results in a 
trap...Which happens way too often, I have a few ideas to fix that, I 
need to find time to tackle this though.

I don't know FORTIFY_SOURCE, but I guess this could cause a call to an 
external function and then cause the issue I describe above right? We 
already remove all instrumentations from this early code so maybe we 
should remove FORTIFY_SOURCE too?

Thanks for digging into this,

Alex


>
>> I would guess that this line at arch/riscv/errata/thead/errata.c:168:
>>
>> void *oldptr, *altptr;
>>
>> Should be:
>>
>> u8 *oldptr, *altptr;
> I tried this, but it didn't appear to make a difference, I will try
> some more today, though I admit to being out of my element.
>
>
>
>> But maybe __ALT_PTR needs adjustment too? Hmm. I would have expected void * to have a size of SIZE_MAX here, though, so something else might be confusing the check.
>>
>>
>> --
>> Kees Cook
>>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists