[PATCH v4] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe()

[PATCH v4] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe()

[Jiakai Xu]

The fp_is_valid() function uses ALIGN(sp, THREAD_SIZE) as the upper
bound for the frame pointer check. This bound is calculated relative
to the current sp and shifts upward when sp itself exceeds the valid
stack region, allowing the unwinder to read past the end of the
allocated task stack and triggering KASAN stack-out-of-bounds.

Fix this by using absolute stack boundaries determined once before
the unwind loop:

- When sp is on the task stack, use the task's pt_regs as the upper
  bound.
- When sp is on the overflow_stack (CONFIG_VMAP_STACK=y), use the
  overflow_stack's top as the boundary.
- When sp is on the IRQ stack (CONFIG_IRQ_STACKS=y), use the IRQ
  stack's top as the boundary.
- When sp is not on any known stack, warn and return.
- For remote tasks (task != current), if sp is not on the task
  stack, warn and return since we cannot reliably determine the
  correct boundary from a different CPU's stacks.

Fixes: a2a4d4a6a0bf ("riscv: stacktrace: fixed walk_stackframe()")
Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
Assisted-by: YuanSheng:DeepSeek-V3.2
---
V3 -> V4:
- Switched to #include <asm/irq_stack.h> instead of hand-written
  DECLARE_PER_CPU, as suggested by Nam Cao.  This also fixes a
  build failure when CONFIG_IRQ_STACKS=n since the header
  unconditionally declares irq_stack_ptr.
- Added dedicated overflow_stack handling (CONFIG_VMAP_STACK)
  with the correct 4 KiB boundary. v3 fell through to the 32 KiB 
  IRQ stack bound when unwinding from the overflow_stack, 
  triggering KASAN OOB reads past the small overflow_stack 
  allocation. Reported by Sashiko AI review and reproduced by Xiao Wu.
- Changed the fallback else branch to warn and return instead of
  silently falling back to task_pt_regs(), as suggested by Nam Cao.
- For remote tasks (task != current), bail out with a warning when
  sp is not on the task stack, since we cannot reliably use the
  local CPU's IRQ/overflow stacks as the boundary.
https://lore.kernel.org/linux-riscv/20260625123906.211981-1-xujiakai2025@iscas.ac.cn/T/#u

V2 -> V3:
- Handled the case where sp is on the IRQ stack (CONFIG_IRQ_STACKS),
  as suggested by Nam Cao: when sp is not within the task stack
  range, use the IRQ stack's top (irq_stack_ptr + IRQ_STACK_SIZE)
  as the upper bound instead of task_pt_regs(), which would point
  to the wrong stack. The check uses the sp variable directly rather
  than on_thread_stack(), because on_thread_stack() reads the
  current CPU register sp which may differ from the unwinder's sp
  when regs is provided.
- Used IS_ENABLED(CONFIG_IRQ_STACKS) to guard the IRQ stack check,
  and cast irq_stack_ptr to unsigned long for byte-level arithmetic.
https://lore.kernel.org/linux-riscv/20260517143704.659416-1-xujiakai2025@iscas.ac.cn/T/#u

V1 -> V2:
- Moved the NULL task check from fp_is_valid() into walk_stackframe(),
  as suggested by Matthew Bystrin.
- Changed the upper bound from task_stack_page(task) + THREAD_SIZE to
  task_pt_regs(task) for a tighter boundary, as suggested by Matthew
  Bystrin.
https://lore.kernel.org/linux-riscv/20260514100711.838895-1-xujiakai2025@iscas.ac.cn/t/#u
---
 arch/riscv/kernel/stacktrace.c | 46 +++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index c7555447149b..4a3369620bc8 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -12,6 +12,7 @@
 #include <linux/stacktrace.h>
 #include <linux/ftrace.h>
 
+#include <asm/irq_stack.h>
 #include <asm/stacktrace.h>
 
 #ifdef CONFIG_FRAME_POINTER
@@ -35,12 +36,12 @@
 extern asmlinkage void handle_exception(void);
 extern unsigned long ret_from_exception_end;
 
-static inline int fp_is_valid(unsigned long fp, unsigned long sp)
+static inline int fp_is_valid(unsigned long fp, unsigned long sp,
+			       unsigned long high)
 {
-	unsigned long low, high;
+	unsigned long low;
 
 	low = sp + sizeof(struct stackframe);
-	high = ALIGN(sp, THREAD_SIZE);
 
 	return !(fp < low || fp > high || fp & 0x07);
 }
@@ -48,7 +49,7 @@ static inline int fp_is_valid(unsigned long fp, unsigned long sp)
 void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 			     bool (*fn)(void *, unsigned long), void *arg)
 {
-	unsigned long fp, sp, pc;
+	unsigned long fp, sp, pc, high = 0;
 	int graph_idx = 0;
 	int level = 0;
 
@@ -68,19 +69,52 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		pc = task->thread.ra;
 	}
 
+	if (!task)
+		task = current;
+
+	if (sp >= (unsigned long)task_stack_page(task) &&
+	    sp < (unsigned long)task_stack_page(task) + THREAD_SIZE) {
+		high = (unsigned long)task_pt_regs(task);
+	} else if (task != current) {
+		pr_warn("%s: sp (%lx) is not in task stack of %s\n",
+			__func__, sp, task->comm);
+		return;
+	}
+#ifdef CONFIG_VMAP_STACK
+	else {
+		unsigned long ovf_base =
+			(unsigned long)this_cpu_ptr(overflow_stack);
+
+		if (sp >= ovf_base && sp < ovf_base + OVERFLOW_STACK_SIZE)
+			high = ovf_base + OVERFLOW_STACK_SIZE;
+	}
+#endif
+	if (IS_ENABLED(CONFIG_IRQ_STACKS) && !high) {
+		unsigned long irq_base =
+			(unsigned long)this_cpu_read(irq_stack_ptr);
+
+		if (sp >= irq_base && sp < irq_base + IRQ_STACK_SIZE)
+			high = irq_base + IRQ_STACK_SIZE;
+	}
+	if (!high) {
+		pr_warn("%s: sp (%lx) is not on any known stack\n",
+			__func__, sp);
+		return;
+	}
+
 	for (;;) {
 		struct stackframe *frame;
 
 		if (unlikely(!__kernel_text_address(pc) || (level++ >= 0 && !fn(arg, pc))))
 			break;
 
-		if (unlikely(!fp_is_valid(fp, sp)))
+		if (unlikely(!fp_is_valid(fp, sp, high)))
 			break;
 
 		/* Unwind stack frame */
 		frame = (struct stackframe *)fp - 1;
 		sp = fp;
-		if (regs && (regs->epc == pc) && fp_is_valid(frame->ra, sp)) {
+		if (regs && (regs->epc == pc) && fp_is_valid(frame->ra, sp, high)) {
 			/* We hit function where ra is not saved on the stack */
 			fp = frame->ra;
 			pc = regs->ra;
-- 
2.34.1

Re: [PATCH v4] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe()

[Nam Cao]

Jiakai Xu <xujiakai2025@iscas.ac.cn> writes:
> +	if (sp >= (unsigned long)task_stack_page(task) &&
> +	    sp < (unsigned long)task_stack_page(task) + THREAD_SIZE) {
> +		high = (unsigned long)task_pt_regs(task);
> +	} else if (task != current) {
> +		pr_warn("%s: sp (%lx) is not in task stack of %s\n",
> +			__func__, sp, task->comm);
> +		return;
> +	}
> +#ifdef CONFIG_VMAP_STACK
> +	else {
> +		unsigned long ovf_base =
> +			(unsigned long)this_cpu_ptr(overflow_stack);
> +
> +		if (sp >= ovf_base && sp < ovf_base + OVERFLOW_STACK_SIZE)
> +			high = ovf_base + OVERFLOW_STACK_SIZE;
> +	}
> +#endif

Looks functionally correct to me. But this #ifdef goes against the
kernel's coding style. See:
https://docs.kernel.org/process/coding-style.html

Can we
    else if (IS_ENABLED(CONFIG_VMAP_STACK))
just like the irq stack thing?

> +	if (IS_ENABLED(CONFIG_IRQ_STACKS) && !high) {
> +		unsigned long irq_base =
> +			(unsigned long)this_cpu_read(irq_stack_ptr);
> +
> +		if (sp >= irq_base && sp < irq_base + IRQ_STACK_SIZE)
> +			high = irq_base + IRQ_STACK_SIZE;
> +	}

Re: [PATCH v4] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe()

[Jiakai Xu]

Hi! Thanks for your review!

> > +	if (sp >= (unsigned long)task_stack_page(task) &&
> > +	    sp < (unsigned long)task_stack_page(task) + THREAD_SIZE) {
> > +		high = (unsigned long)task_pt_regs(task);
> > +	} else if (task != current) {
> > +		pr_warn("%s: sp (%lx) is not in task stack of %s\n",
> > +			__func__, sp, task->comm);
> > +		return;
> > +	}
> > +#ifdef CONFIG_VMAP_STACK
> > +	else {
> > +		unsigned long ovf_base =
> > +			(unsigned long)this_cpu_ptr(overflow_stack);
> > +
> > +		if (sp >= ovf_base && sp < ovf_base + OVERFLOW_STACK_SIZE)
> > +			high = ovf_base + OVERFLOW_STACK_SIZE;
> > +	}
> > +#endif
> 
> Looks functionally correct to me. But this #ifdef goes against the
> kernel's coding style. See:
> https://docs.kernel.org/process/coding-style.html
> 
> Can we
>     else if (IS_ENABLED(CONFIG_VMAP_STACK))
> just like the irq stack thing?

You are right, that is my mistake. I will fix this.

I will wait a couple of days to see if there are any other suggestions 
from others. If not, I will submit a v5 with this change.

Thanks for the review!

> 
> > +	if (IS_ENABLED(CONFIG_IRQ_STACKS) && !high) {
> > +		unsigned long irq_base =
> > +			(unsigned long)this_cpu_read(irq_stack_ptr);
> > +
> > +		if (sp >= irq_base && sp < irq_base + IRQ_STACK_SIZE)
> > +			high = irq_base + IRQ_STACK_SIZE;
> > +	}

Regards,
Jiakai