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

gentle ping ?

> 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.
> 
> Make the DECLARE_PER_CPU(overflow_stack) unconditional in
> asm/stacktrace.h so that stacktrace.c can use
> IS_ENABLED(CONFIG_VMAP_STACK) instead of #ifdef, in line with the
> kernel coding style which discourages the use of #ifdef in .c files
> (https://docs.kernel.org/process/coding-style.html).  This is safe
> because the DEFINE_PER_CPU (memory allocation) in traps.c remains
> guarded by CONFIG_VMAP_STACK; the reference in stacktrace.c is only
> compiled when IS_ENABLED(CONFIG_VMAP_STACK) evaluates to true.
> 
> Fixes: a2a4d4a6a0bf ("riscv: stacktrace: fixed walk_stackframe()")
> Signed-off-by: Jiakai Xu <[email protected]>
> Assisted-by: YuanSheng:DeepSeek-V3.2
> ---
> V4 -> V5:
> - Made DECLARE_PER_CPU(overflow_stack) unconditional in
>   asm/stacktrace.h so that stacktrace.c can use IS_ENABLED()
>   instead of #ifdef, as suggested by Nam Cao.
> - Converted #ifdef CONFIG_VMAP_STACK to else if
>   (IS_ENABLED(CONFIG_VMAP_STACK)) in walk_stackframe() and folded
>   the overflow_stack and IRQ stack checks into a single if-else
>   chain, eliminating the !high sentinel variable.
> Link: https://lore.kernel.org/linux-riscv/[email protected]/T/#u
> 
> 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.
> Link: https://lore.kernel.org/linux-riscv/[email protected]/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.
> Link: https://lore.kernel.org/linux-riscv/[email protected]/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.
> Link: https://lore.kernel.org/linux-riscv/[email protected]/t/#u
> ---
>  arch/riscv/include/asm/stacktrace.h |  8 +++++--
>  arch/riscv/kernel/stacktrace.c      | 37 ++++++++++++++++++++++++-----
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
> index b1495a7e06ce..2d13c35a8c05 100644
> --- a/arch/riscv/include/asm/stacktrace.h
> +++ b/arch/riscv/include/asm/stacktrace.h
> @@ -22,8 +22,12 @@ static inline bool on_thread_stack(void)
>  }
>  
>  
> -#ifdef CONFIG_VMAP_STACK
> +/*
> + * Declare unconditionally so that arch/riscv/kernel/stacktrace.c can use
> + * IS_ENABLED(CONFIG_VMAP_STACK) rather than #ifdef, in line with the kernel
> + * coding style. The DEFINE_PER_CPU (memory allocation) remains guarded by
> + * CONFIG_VMAP_STACK in traps.c.
> + */
>  DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
> -#endif /* CONFIG_VMAP_STACK */
>  
>  #endif /* _ASM_RISCV_STACKTRACE_H */
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index c7555447149b..7a66c7ee2c81 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,43 @@ 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;
> +	} else if (IS_ENABLED(CONFIG_VMAP_STACK) &&
> +		   sp >= (unsigned long)this_cpu_ptr(overflow_stack) &&
> +		   sp < (unsigned long)this_cpu_ptr(overflow_stack) + OVERFLOW_STACK_SIZE) {
> +		high = (unsigned long)this_cpu_ptr(overflow_stack) + OVERFLOW_STACK_SIZE;
> +	} else if (IS_ENABLED(CONFIG_IRQ_STACKS) &&
> +		   sp >= (unsigned long)this_cpu_read(irq_stack_ptr) &&
> +		   sp < (unsigned long)this_cpu_read(irq_stack_ptr) + IRQ_STACK_SIZE) {
> +		high = (unsigned long)this_cpu_read(irq_stack_ptr) + IRQ_STACK_SIZE;
> +	} else {
> +		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

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv