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

Jiakai Xu posted 1 patch 1 week ago
arch/riscv/kernel/stacktrace.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
[PATCH v2] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe()
Posted by Jiakai Xu 1 week ago
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 the absolute task stack boundary 
(task_pt_regs(task)) instead. This ensures that once the frame pointer 
walks past the actual end of the stack, the check consistently fails 
and the unwinding terminates.

Fixes: a2a4d4a6a0bf ("riscv: stacktrace: fixed walk_stackframe()")
Signed-off-by: Jiakai Xu <xujiakai2025@iscas.ac.cn>
Assisted-by: OpenClaw:DeepSeek-V3.2
---
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.
---
 arch/riscv/kernel/stacktrace.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index b41b6255751c..6ce3465bec2c 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -35,12 +35,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 +48,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;
 	int graph_idx = 0;
 	int level = 0;
 
@@ -68,19 +68,24 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		pc = task->thread.ra;
 	}
 
+	if (!task)
+		task = current;
+
+	high = (unsigned long)task_pt_regs(task);
+
 	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 v2] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe()
Posted by Matthew Bystrin 6 days, 9 hours ago
Hi!

> @@ -68,19 +68,24 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  		pc = task->thread.ra;
>  	}
>  
> +	if (!task)
> +		task = current;
> +
> +	high = (unsigned long)task_pt_regs(task);

After noticing end_of_stack() function I'm not sure task_pt_regs() is the best
candidate for the boundary. Let's see what maintainers are going to say.

> +
>  	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

All in all looks fine.

Reviewed-by: Matthew Bystrin <dev.mbstr@gmail.com>

-- 
Best regards
Matt