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

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

Note that the frame pointer unwinder has no mechanism to detect
when the unwind has crossed from the task stack onto a different
kernel stack (e.g., IRQ stack). Using the absolute task stack
boundary provides correct protection against out-of-bounds reads
at the end of the task stack.

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

diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index b41b6255751c..d23681873b5a 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -35,12 +35,16 @@
 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,
+			      struct task_struct *task)
 {
 	unsigned long low, high;
 
+	if (!task)
+		task = current;
+
 	low = sp + sizeof(struct stackframe);
-	high = ALIGN(sp, THREAD_SIZE);
+	high = (unsigned long)task_stack_page(task) + THREAD_SIZE;
 
 	return !(fp < low || fp > high || fp & 0x07);
 }
@@ -74,13 +78,13 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		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, task)))
 			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, task)) {
 			/* We hit function where ra is not saved on the stack */
 			fp = frame->ra;
 			pc = regs->ra;
-- 
2.34.1
Re: [PATCH] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe
Posted by Matthew Bystrin 4 weeks ago
Hi, Jiakai!

Thanks for your valid correction!

Jiakai Xu, May 14, 2026 at 13:07:
> -static inline int fp_is_valid(unsigned long fp, unsigned long sp)
> +static inline int fp_is_valid(unsigned long fp, unsigned long sp,
> +			      struct task_struct *task)
>  {
>  	unsigned long low, high;
>  
> +	if (!task)
> +		task = current;
> +

I would suggest to move this `if` into walk_stackframe() function in order to do
this only once before walking loop.

>  	low = sp + sizeof(struct stackframe);
> -	high = ALIGN(sp, THREAD_SIZE);
> +	high = (unsigned long)task_stack_page(task) + THREAD_SIZE;

Also after grepping `task_stack_page` I've noticed that pt_regs structure is
located at the end of stack. Maybe it is a good idea to adjust border even
"lower" to check that sp does not points inside pt_regs? (see task_pt_regs
macro)

>  	return !(fp < low || fp > high || fp & 0x07);
>  }

-- 
Best regards,
Matt
Re: [PATCH] riscv: stacktrace: fix stack-out-of-bounds in walk_stackframe
Posted by Jiakai Xu 4 weeks ago
Hi, Matt!

Thank you for your review and valuable suggestions!

> > -static inline int fp_is_valid(unsigned long fp, unsigned long sp)
> > +static inline int fp_is_valid(unsigned long fp, unsigned long sp,
> > +			      struct task_struct *task)
> >  {
> >  	unsigned long low, high;
> >  
> > +	if (!task)
> > +		task = current;
> > +
> 
> I would suggest to move this `if` into walk_stackframe() function in order to do
> this only once before walking loop.

Moving the `if (!task)` check into `walk_stackframe()` is a reasonable 
micro-optimization. I'll adopt this in v2.

> 
> >  	low = sp + sizeof(struct stackframe);
> > -	high = ALIGN(sp, THREAD_SIZE);
> > +	high = (unsigned long)task_stack_page(task) + THREAD_SIZE;
> 
> Also after grepping `task_stack_page` I've noticed that pt_regs structure is
> located at the end of stack. Maybe it is a good idea to adjust border even
> "lower" to check that sp does not points inside pt_regs? (see task_pt_regs
> macro)

Using `task_pt_regs(task)` as the upper bound provides a tighter and more 
precise boundary. I'll update the high bound accordingly in v2.

I’m planning to submit the v2 patch in two days. I’d like to wait and see 
whether others have any suggestions for this patch.

Thanks again for your time and suggestions!

Best regards,
Jiakai Xu