Re: [next] arm: Internal error: Oops: 5 PC is at __read_once_word_nocheck

[Ard Biesheuvel]

On Wed, 9 Mar 2022 at 18:11, Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, Mar 09, 2022 at 10:08:25PM +0530, Naresh Kamboju wrote:
> > Hi Russell,
> >
> > On Wed, 9 Mar 2022 at 20:37, Russell King (Oracle)
> > <linux@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Mar 09, 2022 at 03:57:32PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 9 Mar 2022 at 15:44, Naresh Kamboju <naresh.kamboju@xxxxxxxxxx> wrote:
> > > > >
> > <trim>
> > > Well, we unwound until:
> > >
> > >  __irq_svc from migrate_disable+0x0/0x70
> > >
> > > and then crashed - and the key thing there is that we're at the start
> > > of migrate_disable() when we took an interrupt.
> > >
> > > For some reason, this triggers an access to address 0x10, which faults.
> > > We then try unwinding again, and successfully unwind all the way back
> > > to the same point (the line above) which then causes the unwinder to
> > > again access address 0x10, and the cycle repeats with the stack
> > > growing bigger and bigger.
> > >
> > > I'd suggest also testing without the revert but with my patch.
> >
> > I have tested your patch on top of linux next-20220309 and still see kernel
> > crash as below [1]. build link [2].
> >
> > [   26.812060] 8<--- cut here ---
> > [   26.813459] Unhandled fault: page domain fault (0x01b) at 0xb6a3ab70
> > [   26.816139] [b6a3ab70] *pgd=fb28a835
> > [   26.817770] Internal error: : 1b [#1] SMP ARM
> > [   26.819636] Modules linked in:
> > [   26.820956] CPU: 0 PID: 211 Comm: haveged Not tainted
> > 5.17.0-rc7-next-20220309 #1
> > [   26.824519] Hardware name: Generic DT based system
> > [   26.827148] PC is at __read_once_word_nocheck+0x0/0x8
> > [   26.829856] LR is at unwind_frame+0x7dc/0xab4
> >
> > - Naresh
> >
> > [1] https://lkft.validation.linaro.org/scheduler/job/4688599#L596
> > [2] https://builds.tuxbuild.com/269gYLGuAdmltuLhIUDAjS2fg1Q/
>
> I think the problem has just moved:
>
> [   27.113085]  __irq_svc from __copy_to_user_std+0x24/0x378
>
> The code at the start of __copy_to_user_std is:
>
>    0:   e3a034bf        mov     r3, #-1090519040        ; 0xbf000000
>    4:   e243c001        sub     ip, r3, #1
>    8:   e05cc000        subs    ip, ip, r0
>    c:   228cc001        addcs   ip, ip, #1
>   10:   205cc002        subscs  ip, ip, r2
>   14:   33a00000        movcc   r0, #0
>   18:   e320f014        csdb
>   1c:   e3a03000        mov     r3, #0
>   20:   e92d481d        push    {r0, r2, r3, r4, fp, lr}
>   24:   e1a0b00d        mov     fp, sp
>
> and the unwind information will be:
>
> 0xc056f14c <arm_copy_to_user+0x1c>: @0xc0b89b84
>   Compact model index: 1
>   0x9b      vsp = r11
>   0xb1 0x0d pop {r0, r2, r3}
>   0x84 0x81 pop {r4, r11, r14}
>   0xb0      finish
>
> The problem is that the unwind information says "starting at offset
> 0x1c, to unwind do the following operations". The first of which is
> to move r11 (fp) to the stack pointer. However, r11 isn't setup
> until function offset 0x24. You've hit that instruction, which hasn't
> executed yet, but the stack has been modified by pushing r0, r2-r4,
> fp and lr onto it.
>
> Given this, there is no way that the unwinder (as it currently stands)
> can do its job properly between 0x1c and 0x24.
>
> I don't think this is specifically caused by Ard's patches, but by
> the addition of KASAN, which has the effect of calling the unwinder
> at random points in the kernel (when an interrupt happens) and it's
> clear from the above that there are windows in the code where, if
> we attempt to unwind using the unwind information, we faill fail
> because the program state is not consistent with the unwind
> information.
>
> Ard's patch that changes:
>
>         ctrl->vrs[reg] = READ_ONCE_NOCHECK(*(*vsp));
>
> to use get_kernel_nofault() should have the effect of protecting
> against the oops, but the side effect is that it is fundamentally not
> possible with the way these things are to unwind at these points -
> which means its not possible to get a stacktrace there.
>
> So, I don't think this is a "new" problem, but a weakness of using
> the unwinder to get a backtrace for KASAN.
>

It essentially means that we cannot unwind through asynchronous
exceptions, and so we should probably make the svc_entry macro
.nounwind, instead of pretending that we can reliably unwind through
it.

For these annotation purposes, where the interrupt was taken is not
terribly interesting anyway, terminating the stacktrace earlier might
even recover some performance lost to KASAN overhead.

Naresh, please try the patch below.

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 5609ca8ae46a..0d8ae1a14643 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -184,12 +184,12 @@ ENDPROC(__und_invalid)

        .macro  svc_entry, stack_hole=0, trace=1, uaccess=1, overflow_check=1
  UNWIND(.fnstart               )
+ UNWIND(.cantunwind            )
        sub     sp, sp, #(SVC_REGS_SIZE + \stack_hole)
  THUMB(        add     sp, r1          )       @ get SP in a GPR without
  THUMB(        sub     r1, sp, r1      )       @ using a temp register

        .if     \overflow_check
- UNWIND(.save  {r0 - pc}       )
        do_overflow_check (SVC_REGS_SIZE + \stack_hole)
        .endif