# \[PATCH\] powerpc: make show\_stack's stack walking KASAN-safe - Subsystems: LINUX FOR POWERPC (32-BIT AND 64-BIT), THE REST - Maintainers: Madhavan Srinivasan, Linus Torvalds - List: linuxppc-dev - 4 messages, 3 authors - Page: https://ratatoskr.run/linuxppc-dev/2021/05/9522903/t ## \[PATCH\] powerpc: make show\_stack's stack walking KASAN-safe - From: Daniel Axtens \ - Date: 2021-05-28 07:48:52 - Message: https://ratatoskr.run/linuxppc-dev/2021/05/9522903.md ```text Make our stack-walking code KASAN-safe by using READ_ONCE_NOCHECK - generic code, arm64, s390 and x86 all do this for similar sorts of reasons: when unwinding a stack, we might touch memory that KASAN has marked as being out-of-bounds. In ppc64 KASAN development, I hit this sometimes when checking for an exception frame - because we're checking an arbitrary offset into the stack frame. See commit 20955746320e ("s390/kasan: avoid false positives during stack unwind"), commit bcaf669b4bdb ("arm64: disable kasan when accessing frame->fp in unwind_frame"), commit 91e08ab0c851 ("x86/dumpstack: Prevent KASAN false positive warnings") and commit 6e22c8366416 ("tracing, kasan: Silence Kasan warning in check_stack of stack_tracer"). Signed-off-by: Daniel Axtens --- arch/powerpc/kernel/process.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 89e34aa273e2..430cf06f9406 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -2151,8 +2151,8 @@ void show_stack(struct task_struct *tsk, unsigned long *stack, break; stack = (unsigned long *) sp; - newsp = stack[0]; - ip = stack[STACK_FRAME_LR_SAVE]; + newsp = READ_ONCE_NOCHECK(stack[0]); + ip = READ_ONCE_NOCHECK(stack[STACK_FRAME_LR_SAVE]); if (!firstframe || ip != lr) { printk("%s["REG"] ["REG"] %pS", loglvl, sp, ip, (void *)ip); @@ -2170,17 +2170,19 @@ void show_stack(struct task_struct *tsk, unsigned long *stack, * See if this is an exception frame. * We look for the "regshere" marker in the current frame. */ - if (validate_sp(sp, tsk, STACK_FRAME_WITH_PT_REGS) - && stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) { + if (validate_sp(sp, tsk, STACK_FRAME_WITH_PT_REGS) && + (READ_ONCE_NOCHECK(stack[STACK_FRAME_MARKER]) == + STACK_FRAME_REGS_MARKER)) { struct pt_regs *regs = (struct pt_regs *) (sp + STACK_FRAME_OVERHEAD); - lr = regs->link; + lr = READ_ONCE_NOCHECK(regs->link); printk("%s--- interrupt: %lx at %pS\n", - loglvl, regs->trap, (void *)regs->nip); + loglvl, READ_ONCE_NOCHECK(regs->trap), + (void *)READ_ONCE_NOCHECK(regs->nip)); __show_regs(regs); printk("%s--- interrupt: %lx\n", - loglvl, regs->trap); + loglvl, READ_ONCE_NOCHECK(regs->trap)); firstframe = 1; } -- 2.27.0 ``` ## Re: \[PATCH\] powerpc: make show\_stack's stack walking KASAN-safe - From: Christophe Leroy \ - Date: 2021-06-01 07:23:20 - Message: https://ratatoskr.run/linuxppc-dev/2021/06/9522948.md ```text Le 28/05/2021 à 09:48, Daniel Axtens a écrit : > Make our stack-walking code KASAN-safe by using READ_ONCE_NOCHECK - > generic code, arm64, s390 and x86 all do this for similar sorts of > reasons: when unwinding a stack, we might touch memory that KASAN has > marked as being out-of-bounds. In ppc64 KASAN development, I hit this > sometimes when checking for an exception frame - because we're checking > an arbitrary offset into the stack frame. > > See commit 20955746320e ("s390/kasan: avoid false positives during stack > unwind"), commit bcaf669b4bdb ("arm64: disable kasan when accessing > frame->fp in unwind_frame"), commit 91e08ab0c851 ("x86/dumpstack: > Prevent KASAN false positive warnings") and commit 6e22c8366416 > ("tracing, kasan: Silence Kasan warning in check_stack of stack_tracer"). > > Signed-off-by: Daniel Axtens > --- > arch/powerpc/kernel/process.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 89e34aa273e2..430cf06f9406 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -2151,8 +2151,8 @@ void show_stack(struct task_struct *tsk, unsigned long *stack, > break; > > stack = (unsigned long *) sp; > - newsp = stack[0]; > - ip = stack[STACK_FRAME_LR_SAVE]; > + newsp = READ_ONCE_NOCHECK(stack[0]); > + ip = READ_ONCE_NOCHECK(stack[STACK_FRAME_LR_SAVE]); > if (!firstframe || ip != lr) { > printk("%s["REG"] ["REG"] %pS", > loglvl, sp, ip, (void *)ip); > @@ -2170,17 +2170,19 @@ void show_stack(struct task_struct *tsk, unsigned long *stack, > * See if this is an exception frame. > * We look for the "regshere" marker in the current frame. > */ > - if (validate_sp(sp, tsk, STACK_FRAME_WITH_PT_REGS) > - && stack[STACK_FRAME_MARKER] == STACK_FRAME_REGS_MARKER) { > + if (validate_sp(sp, tsk, STACK_FRAME_WITH_PT_REGS) && > + (READ_ONCE_NOCHECK(stack[STACK_FRAME_MARKER]) == > + STACK_FRAME_REGS_MARKER)) { > struct pt_regs *regs = (struct pt_regs *) > (sp + STACK_FRAME_OVERHEAD); > > - lr = regs->link; > + lr = READ_ONCE_NOCHECK(regs->link); > printk("%s--- interrupt: %lx at %pS\n", > - loglvl, regs->trap, (void *)regs->nip); > + loglvl, READ_ONCE_NOCHECK(regs->trap), > + (void *)READ_ONCE_NOCHECK(regs->nip)); > __show_regs(regs); > printk("%s--- interrupt: %lx\n", > - loglvl, regs->trap); > + loglvl, READ_ONCE_NOCHECK(regs->trap)); Actually you read regs->trap twice now. Can you use a local var and really read it only once ? > > firstframe = 1; > } > ``` ## Re: \[PATCH\] powerpc: make show\_stack's stack walking KASAN-safe - From: Naveen N. Rao \ - Date: 2021-06-01 09:41:14 - Message: https://ratatoskr.run/linuxppc-dev/2021/06/9522951.md ```text Daniel Axtens wrote: > Make our stack-walking code KASAN-safe by using READ_ONCE_NOCHECK - > generic code, arm64, s390 and x86 all do this for similar sorts of > reasons: when unwinding a stack, we might touch memory that KASAN has > marked as being out-of-bounds. In ppc64 KASAN development, I hit this > sometimes when checking for an exception frame - because we're checking > an arbitrary offset into the stack frame. > > See commit 20955746320e ("s390/kasan: avoid false positives during stack > unwind"), commit bcaf669b4bdb ("arm64: disable kasan when accessing > frame->fp in unwind_frame"), commit 91e08ab0c851 ("x86/dumpstack: > Prevent KASAN false positive warnings") and commit 6e22c8366416 > ("tracing, kasan: Silence Kasan warning in check_stack of stack_tracer"). > > Signed-off-by: Daniel Axtens > --- > arch/powerpc/kernel/process.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 89e34aa273e2..430cf06f9406 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -2151,8 +2151,8 @@ void show_stack(struct task_struct *tsk, unsigned long *stack, > break; > > stack = (unsigned long *) sp; > - newsp = stack[0]; > - ip = stack[STACK_FRAME_LR_SAVE]; > + newsp = READ_ONCE_NOCHECK(stack[0]); > + ip = READ_ONCE_NOCHECK(stack[STACK_FRAME_LR_SAVE]); Just curious: Given that we validate the stack pointer before these accesses, can we annotate show_stack() with __no_sanitize_address instead? I ask because we have other places where we walk the stack: arch_stack_walk(), as well as in perf callchain. Similar changes will be needed there as well. - Naveen ``` ## Re: \[PATCH\] powerpc: make show\_stack's stack walking KASAN-safe - From: Daniel Axtens \ - Date: 2021-06-01 14:42:36 - Message: https://ratatoskr.run/linuxppc-dev/2021/06/9522952.md ```text "Naveen N. Rao" [ref] writes: > Daniel Axtens wrote: >> Make our stack-walking code KASAN-safe by using READ_ONCE_NOCHECK - >> generic code, arm64, s390 and x86 all do this for similar sorts of >> reasons: when unwinding a stack, we might touch memory that KASAN has >> marked as being out-of-bounds. In ppc64 KASAN development, I hit this >> sometimes when checking for an exception frame - because we're checking >> an arbitrary offset into the stack frame. >> >> See commit 20955746320e ("s390/kasan: avoid false positives during stack >> unwind"), commit bcaf669b4bdb ("arm64: disable kasan when accessing >> frame->fp in unwind_frame"), commit 91e08ab0c851 ("x86/dumpstack: >> Prevent KASAN false positive warnings") and commit 6e22c8366416 >> ("tracing, kasan: Silence Kasan warning in check_stack of stack_tracer"). >> >> Signed-off-by: Daniel Axtens >> --- >> arch/powerpc/kernel/process.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c >> index 89e34aa273e2..430cf06f9406 100644 >> --- a/arch/powerpc/kernel/process.c >> +++ b/arch/powerpc/kernel/process.c >> @@ -2151,8 +2151,8 @@ void show_stack(struct task_struct *tsk, unsigned long *stack, >> break; >> >> stack = (unsigned long *) sp; >> - newsp = stack[0]; >> - ip = stack[STACK_FRAME_LR_SAVE]; >> + newsp = READ_ONCE_NOCHECK(stack[0]); >> + ip = READ_ONCE_NOCHECK(stack[STACK_FRAME_LR_SAVE]); > > Just curious: > Given that we validate the stack pointer before these accesses, can we > annotate show_stack() with __no_sanitize_address instead? > > I ask because we have other places where we walk the stack: > arch_stack_walk(), as well as in perf callchain. Similar changes will be > needed there as well. Oh good points. Yes, it probably makes most sense to mark all the functions with __no_sanitize_address, that resolves Christophe's issue as well. I'll send a v2. Kind regards, Daniel > > > - Naveen ```