Message-ID: <Yinwq3Z9l0selLLS@shell.armlinux.org.uk>
Date:   Thu, 10 Mar 2022 12:35:55 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Naresh Kamboju <naresh.kamboju@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        Linux-Next Mailing List <linux-next@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Corentin Labbe <clabbe.montjoie@...il.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>
Subject: Re: [next] arm: Internal error: Oops: 5 PC is at
 __read_once_word_nocheck

On Wed, Mar 09, 2022 at 09:42:29PM +0100, Ard Biesheuvel wrote:
> On Wed, 9 Mar 2022 at 20:39, Russell King (Oracle)
> <linux@...linux.org.uk> wrote:
> >
> > On Wed, Mar 09, 2022 at 08:14:30PM +0100, Ard Biesheuvel wrote:
> > > The backtrace dumped by __die() uses the pt_regs from the exception
> > > context as the starting point, so the exception entry code that deals
> > > with the condition that triggered the oops is omitted, and does not
> > > have to be unwound.
> >
> > That is true, but that's not really the case I was thinking about.
> > I was thinking more about cases such as RCU stalls, soft lockups,
> > etc.
> >
> > For example:
> >
> > https://www.linuxquestions.org/questions/linux-kernel-70/kenel-v4-4-60-panic-in-igmp6_send-and-and-__neigh_create-4175704721/
> >
> > In that stack trace, the interesting bits are not the beginning of
> > the stack trace down to __irq_svc, but everything beyond __irq_svc,
> > since the lockup is probably caused by being stuck in
> > _raw_write_lock_bh().
> >
> > It's these situations that we will totally destroy debuggability for,
> > and the only way around that would be to force frame pointers and
> > ARM builds (not Thumb-2 as that requires the unwinder... which means
> > a Thumb-2 kernel soft lockup would be undebuggable.
> >
> 
> Indeed.
> 
> But that means that the only other choice we have is to retain the
> imprecise nature of the current solution (which usually works fine
> btw), and simply deal with the faulting double dereference of vsp in
> the unwinder code. We simply don't know whether the exception was
> taken at a point where the stack frame is consistent with the unwind
> data.

Okay, further analysis (for the record, since I've said much of this on
IRC):

What we have currently is a robust unwinder that will cope when things
go wrong, such as an interrupt taken during the prologue of a function.
The way it copes is by two mechanisms:

        /* store the highest address on the stack to avoid crossing it*/
        low = frame->sp;
        ctrl.sp_high = ALIGN(low, THREAD_SIZE);

These two represent the allowable bounds of the kernel stack. When we
run the unwinder, before each unwind instruction we check whether the
current SP value is getting close to the top of the kernel stack, and
if so, turn on additional checking:

                if ((ctrl.sp_high - ctrl.vrs[SP]) < sizeof(ctrl.vrs))
                        ctrl.check_each_pop = 1;

that will ensure if we go off the top of the kernel stack, the
unwinder will report failure, and not access those addresses.

After each instruction, we check whether the SP value is within the
above bounds:

                if (ctrl.vrs[SP] < low || ctrl.vrs[SP] >= ctrl.sp_high)
                        return -URC_FAILURE;

This means that the unwinder can never modify SP to point outside of
the kernel stack region identified by low..ctrl.sp_high, thereby
protecting the load inside unwind_pop_register() from ever
dereferencing something outside of the kernel stack. Moreover, it also
prevents the unwinder modifying SP to point below the current stack
frame.

The problem has been introduced by trying to make the unwinder cope
with IRQ stacks in b6506981f880 ("ARM: unwind: support unwinding across
multiple stacks"):

-       if (!load_sp)
+       if (!load_sp) {
                ctrl->vrs[SP] = (unsigned long)vsp;
+       } else {
+               ctrl->sp_low = ctrl->vrs[SP];
+               ctrl->sp_high = ALIGN(ctrl->sp_low, THREAD_SIZE);
+       }

Now, whenever SP is loaded, we reset the allowable range for the SP
value, and this completely defeats the protections we previously had
which were ensuring that:

1) the SP value doesn't jump back _down_ the kernel stack resulting
   in an infinite unwind loop.
2) the SP value doesn't end up outside the kernel stack.

We need those protections to prevent these problems that are being
reported - and the most efficient way I can think of doing that is to
somehow valudate the new SP value _before_ we modify sp_low and
sp_high, so these two limits are always valid.

Merely changing the READ_ONCE_NOCHECK() to be get_kernel_nocheck()
will only partly fix this problem - it will stop the unwinder oopsing
the kernel, but definitely doesn't protect against (1) and doesn't
protect against SP pointing at some thing that is accessible (e.g.
a device or other kernel memory.)

We're yet again at Thursday, with the last linux-next prior to the
merge window being created this evening, which really doesn't leave
much time to get this sorted... and we can't say "this code should
have been in earlier in the cycle" this time around, because these
changes to the unwinder have been present in linux-next prior to
5.17-rc2. Annoyingly, it seems merging stuff earlier in the cycle
doesn't actually solve the problem of these last minute debugging
panics.

Any suggestions for what we should do? Can we come up with some way
to validate the new SP value before 6pm UTC this evening?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists