Message-ID: <CAJD_bPKHA5+Ta1vzokN-TXJU4bk4OA5QTXEEvAwmzLzmW8YtTQ@mail.gmail.com>
Date: Wed, 2 Oct 2024 11:14:33 -0400
From: Jason Montleon <jmontleo@...hat.com>
To: Alexandre Ghiti <alex@...ti.fr>
Cc: Kees Cook <kees@...nel.org>, 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

On Tue, Oct 1, 2024 at 10:53 AM Alexandre Ghiti <alex@...ti.fr> wrote:
>
> 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?
>

Hi Alex, Thank you for having a look.

Although I narrowed it down to starting with 6.11-rc1, the first time
I encountered this was actually with an rc7 Fedora build.
http://riscv.rocks/koji/buildinfo?buildID=338433

I also did a build from upstream 6.11.1 last night and still see the
problem trying to boot with it this morning.

> 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?
>
What you are describing here does not seem unreasonable to me, but I
simply do not know enough to say one way or the other.
Is there something I can do to collect more information to determine
if this is the cause?

Thank you again for having a look,
Jason

> 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