linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] RISC-V: Protect reads from other harts stack frames
@ 2021-05-06  6:13 Palmer Dabbelt
  2021-05-06  8:08 ` Mark Rutland
  0 siblings, 1 reply; 2+ messages in thread
From: Palmer Dabbelt @ 2021-05-06  6:13 UTC (permalink / raw)
  To: linux-riscv
  Cc: Paul Walmsley, Palmer Dabbelt, aou, wangkefeng.wang, akpm,
	0x7f454c46, rostedt, chenhuang5, linux-riscv, linux-kernel,
	kernel-team, Palmer Dabbelt, syzbot+0806291048161061627c,
	Dmitry Vyukov

From: Palmer Dabbelt <palmerdabbelt@google.com>

The stack walking code wasn't correctly decorated with READ_ONCE_NOCHECK
when reading from other harts stack frames, which can trigger a kasan
failure.  This may also manifest as a bug, as without the READ_ONCE we
may get inconsistent results.

Reported-by: syzbot+0806291048161061627c@syzkaller.appspotmail.com
Fixes: 5d8544e2d007 ("RISC-V: Generic library routines and assembly")
Suggested-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>

---

I don't actually have a test for stack walking aside from just crashing
the kernel and making sure things look roughly OK.  I haven't gotten
around to that because this got lost in the merge window shuffle, but I
thought I'd send this out in case someone has a better test for stack
walking so I can start running that.
---
 arch/riscv/kernel/stacktrace.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 3f893c9d9d85..7f3914756915 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -18,6 +18,9 @@ register const unsigned long sp_in_global __asm__("sp");
 
 #ifdef CONFIG_FRAME_POINTER
 
+#define READ_FRAME(frame, off)	\
+	(READ_ONCE_NOCHECK(*(unsigned long *)(frame + offsetof(struct stackframe, off))))
+
 void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 			     bool (*fn)(void *, unsigned long), void *arg)
 {
@@ -40,7 +43,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 
 	for (;;) {
 		unsigned long low, high;
-		struct stackframe *frame;
+		unsigned long frame;
 
 		if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
 			break;
@@ -51,14 +54,14 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		if (unlikely(fp < low || fp > high || fp & 0x7))
 			break;
 		/* Unwind stack frame */
-		frame = (struct stackframe *)fp - 1;
+		frame = fp - sizeof(struct stackframe);
 		sp = fp;
-		if (regs && (regs->epc == pc) && (frame->fp & 0x7)) {
-			fp = frame->ra;
+		if (regs && (regs->epc == pc) && (READ_FRAME(frame, fp) & 0x7)) {
+			fp = READ_FRAME(frame, ra);
 			pc = regs->ra;
 		} else {
-			fp = frame->fp;
-			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
+			fp = READ_FRAME(frame, fp);
+			pc = ftrace_graph_ret_addr(current, NULL, READ_FRAME(frame, ra),
 						   (unsigned long *)(fp - 8));
 		}
 
-- 
2.31.1.527.g47e6f16901-goog


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] RISC-V: Protect reads from other harts stack frames
  2021-05-06  6:13 [PATCH] RISC-V: Protect reads from other harts stack frames Palmer Dabbelt
@ 2021-05-06  8:08 ` Mark Rutland
  0 siblings, 0 replies; 2+ messages in thread
From: Mark Rutland @ 2021-05-06  8:08 UTC (permalink / raw)
  To: Palmer Dabbelt, Kees Cook
  Cc: linux-riscv, Paul Walmsley, aou, wangkefeng.wang, akpm,
	0x7f454c46, rostedt, chenhuang5, linux-kernel, kernel-team,
	Palmer Dabbelt, syzbot+0806291048161061627c, Dmitry Vyukov

On Wed, May 05, 2021 at 11:13:52PM -0700, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmerdabbelt@google.com>
> 
> The stack walking code wasn't correctly decorated with READ_ONCE_NOCHECK
> when reading from other harts stack frames, which can trigger a kasan
> failure.  This may also manifest as a bug, as without the READ_ONCE we
> may get inconsistent results.
> 
> Reported-by: syzbot+0806291048161061627c@syzkaller.appspotmail.com
> Fixes: 5d8544e2d007 ("RISC-V: Generic library routines and assembly")
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
> 
> ---
> 
> I don't actually have a test for stack walking aside from just crashing
> the kernel and making sure things look roughly OK.  I haven't gotten
> around to that because this got lost in the merge window shuffle, but I
> thought I'd send this out in case someone has a better test for stack
> walking so I can start running that.

LKDTM has a CORRUPT_STACK test, but IIUC it doesn't have a cross-cpu variant.
Maybe Kees feels like adding one... ;)

It might be worth giving the CORRUPT_STACK test a spin regardless, since
that'll show whehtre your unwinder is generally robust to cases you might see
for racy unwinding.

To build that in, select CONFIG_LKDTM=y. You can get a list of the built-in
crash triggers with:

# cat /sys/kernel/debug/lkdtm/provoke-crash/DIRECT

... and to run the CORRUPT_STACK test specifically, run:

# echo CORRUPT_STACK > /sys/kernel/debug/lkdtm/provoke-crash/DIRECT

For comparison, on arm64 defconfig + LKDTM, that gives me the following:

| # echo CORRUPT_STACK > /sys/kernel/debug/provoke-crash/DIRECT 
| [   82.233662] lkdtm: Performing direct entry CORRUPT_STACK
| [   82.234771] lkdtm: Corrupting stack containing char array ...
| [   82.235927] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: lkdtm_CORRUPT_STACK+0x48/0x58
| [   82.238031] CPU: 1 PID: 184 Comm: bash Not tainted 5.12.0-00001-gd387589d98cd-dirty #5
| [   82.239635] Hardware name: linux,dummy-virt (DT)
| [   82.240572] Call trace:
| [   82.241131]  dump_backtrace+0x0/0x1a0
| [   82.241895]  show_stack+0x18/0x70
| [   82.242579]  dump_stack+0xd0/0x12c
| [   82.243289]  panic+0x16c/0x334
| [   82.243919]  __stack_chk_fail+0x34/0x40
| [   82.244699]  lkdtm_CORRUPT_STACK+0x48/0x58
| [   82.245534]  lkdtm_do_action+0x24/0x30
| [   82.246346]  0xffffffffffffffff
| [   82.246994] SMP: stopping secondary CPUs
| [   82.247862] Kernel Offset: 0x5edcf4200000 from 0xffff800010000000
| [   82.249091] PHYS_OFFSET: 0xffffa3b4c0000000
| [   82.249959] CPU features: 0x00044002,63800038
| [   82.250918] Memory Limit: none
| [   82.251618] ---[ end Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: lkdtm_CORRUPT_STACK+0x48/0x58 ]---

... arm64 will stop when we hit the first bogus FP value as we check
that all frame records are within known stack bounds prior to
dereference. You might want to do similar on riscv.

Thanks,
Mark.

> ---
>  arch/riscv/kernel/stacktrace.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index 3f893c9d9d85..7f3914756915 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -18,6 +18,9 @@ register const unsigned long sp_in_global __asm__("sp");
>  
>  #ifdef CONFIG_FRAME_POINTER
>  
> +#define READ_FRAME(frame, off)	\
> +	(READ_ONCE_NOCHECK(*(unsigned long *)(frame + offsetof(struct stackframe, off))))
> +
>  void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  			     bool (*fn)(void *, unsigned long), void *arg)
>  {
> @@ -40,7 +43,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  
>  	for (;;) {
>  		unsigned long low, high;
> -		struct stackframe *frame;
> +		unsigned long frame;
>  
>  		if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
>  			break;
> @@ -51,14 +54,14 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  		if (unlikely(fp < low || fp > high || fp & 0x7))
>  			break;
>  		/* Unwind stack frame */
> -		frame = (struct stackframe *)fp - 1;
> +		frame = fp - sizeof(struct stackframe);
>  		sp = fp;
> -		if (regs && (regs->epc == pc) && (frame->fp & 0x7)) {
> -			fp = frame->ra;
> +		if (regs && (regs->epc == pc) && (READ_FRAME(frame, fp) & 0x7)) {
> +			fp = READ_FRAME(frame, ra);
>  			pc = regs->ra;
>  		} else {
> -			fp = frame->fp;
> -			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
> +			fp = READ_FRAME(frame, fp);
> +			pc = ftrace_graph_ret_addr(current, NULL, READ_FRAME(frame, ra),
>  						   (unsigned long *)(fp - 8));
>  		}
>  
> -- 
> 2.31.1.527.g47e6f16901-goog
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-05-06  8:08 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06  6:13 [PATCH] RISC-V: Protect reads from other harts stack frames Palmer Dabbelt
2021-05-06  8:08 ` Mark Rutland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).