Re: [PATCH v12 07/11] perf: RISC-V: support callchains with SSE delivery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] perf: riscv: missing guest check in riscv_perf_out_copy_user()
- [High] perf: riscv: missing CSR_SATP check in perf_callchain_user()
- [High] perf: riscv: unsafe entry unwind and OOB read in sse_kernel_regs_safe()
- [High] perf: riscv: NULL task dereference via walk_stackframe() in fgraph
- [High] perf: riscv: NULL mm dereference in riscv_perf_out_copy_user()
--

commit 1d4832e59d0c42c6027b431628d4fdd87c2373eb
Author: Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com>

perf: RISC-V: support callchains with SSE delivery

This commit adds callchain and raw user-stack sampling support on RISC-V when
PMU overflow events are delivered via SBI Supervisor Software Events (SSE). It
validates the interrupted context before walking kernel or user stacks and
provides an SSE-aware riscv_perf_out_copy_user() helper.

Link: https://lore.kernel.org/r/as79MHtNYd-VbUug@hu-himchau-blr.qualcomm.com/

> diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
> index b465bc9eb870e..216f5bd14c001 100644
> --- a/arch/riscv/kernel/perf_callchain.c
> +++ b/arch/riscv/kernel/perf_callchain.c
> @@ -11,6 +17,126 @@ static bool fill_callchain(void *entry, unsigned long pc)
>  	return perf_callchain_store(entry, pc) == 0;
>  }
>  
> +#ifdef CONFIG_RISCV_SBI_SSE
> +static bool sse_addr_on_task_stack(unsigned long addr, unsigned long size)
> +{
> +	unsigned long end = addr + size;
> +	unsigned long stack;
> +
> +	if (end < addr)
> +		return false;
> +
> +	stack = (unsigned long)task_stack_page(current);
> +	if (addr >= stack && end <= stack + THREAD_SIZE)
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool sse_kernel_regs_safe(struct pt_regs *regs)
> +{
> +	unsigned long fp = frame_pointer(regs);
> +	unsigned long pc = instruction_pointer(regs);
> +	unsigned long sp = user_stack_pointer(regs);
> +
> +	if (!__kernel_text_address(pc))
> +		return false;
> +	if (!sse_addr_on_task_stack(sp, sizeof(unsigned long)))
> +		return false;
> +	if (fp < sizeof(struct stackframe))
> +		return false;
> +
> +	return sse_addr_on_task_stack(fp - sizeof(struct stackframe),
> +				      sizeof(struct stackframe));
> +}

[Severity: High]
Can sse_kernel_regs_safe() return true in sensitive entry windows where s0 is
not a valid kernel frame pointer, or when fp equals stack + THREAD_SIZE,
causing walk_stackframe() to read out of bounds past the end of the task
stack?

In arch/riscv/kernel/entry.S, once sp points to the kernel task stack, there
are three windows where s0 does not point to a valid struct stackframe:

1. In ret_from_exception when returning to userspace:

   addi s0, sp, PT_SIZE_ON_STACK
   REG_S s0, TASK_TI_KERNEL_SP(tp)

Here s0 (fp) is set to stack + THREAD_SIZE while sp is on the task stack.
Because [fp - sizeof(struct stackframe), fp] lies within
[stack, stack + THREAD_SIZE], sse_kernel_regs_safe() returns true.

In arch/riscv/kernel/stacktrace.c:walk_stackframe():

   frame = (struct stackframe *)fp - 1;
   sp = fp;
   ...
   if (pc >= (unsigned long)handle_exception &&
       pc < (unsigned long)&ret_from_exception_end) {
           if (unlikely(!fn(arg, pc)))
                   break;

           pc = ((struct pt_regs *)sp)->epc;
           fp = ((struct pt_regs *)sp)->s0;
   }

When fp is stack + THREAD_SIZE, frame aliases the end of task_pt_regs(current)
(pt_regs->cause and pt_regs->orig_a0), and sp becomes stack + THREAD_SIZE. If
frame->ra (pt_regs->orig_a0, which is user-controlled a0 on a syscall) falls
in [handle_exception, ret_from_exception_end), walk_stackframe() dereferences
((struct pt_regs *)sp)->epc at stack + THREAD_SIZE, reading out of bounds into
the unmapped VMAP_STACK guard page in SSE context.

2. In handle_exception and ret_from_exception right after switching sp to the
kernel stack (or before restoring user sp), s0 still holds the untrusted
userspace s0 value. If userspace sets s0 to an address within its kernel task
stack range, sse_kernel_regs_safe() succeeds and walk_stackframe() unwinds
from the user-chosen kernel stack address.

3. In handle_exception on a trap from kernel mode, REG_L s0,
TASK_TI_USER_SP(tp) sets s0 to sp + PT_SIZE_ON_STACK before s0 is saved,
causing walk_stackframe() to interpret the top of the incomplete pt_regs as a
stackframe.

Should sse_kernel_regs_safe() reject PCs in
[handle_exception, ret_from_exception_end) and require fp to be strictly below
(unsigned long)task_pt_regs(current)?

> +
> +static bool sse_callchain_is_guest(const struct riscv_sse_interrupted_context *context)
> +{
> +	return context && (context->hstatus & HSTATUS_SPV);
> +}
> +
> +static bool sse_callchain_kernel(struct perf_callchain_entry_ctx *entry,
> +				 struct pt_regs *regs)
> +{
> +	const struct riscv_sse_interrupted_context *context;
> +	unsigned long pc;
> +
> +	context = riscv_sse_get_interrupted_context();
> +	if (!context || context->regs != regs)
> +		return false;
> +
> +	/* A guest stack cannot be walked using the host kernel address space. */
> +	if (sse_callchain_is_guest(context))
> +		return true;
> +
> +	if (user_mode(regs))
> +		return true;
> +
> +	if (sse_kernel_regs_safe(regs)) {
> +		walk_stackframe(NULL, regs, fill_callchain, entry);
> +		return true;
> +	}

[Severity: High]
Does passing NULL as the task argument to walk_stackframe() in
sse_callchain_kernel() (and in perf_callchain_kernel()) cause a NULL pointer
dereference when function_graph tracing is active?

In arch/riscv/kernel/stacktrace.c:walk_stackframe(), when regs is non-NULL,
task is not initialized to current and remains NULL:

   if (regs) {
           fp = frame_pointer(regs);
           sp = user_stack_pointer(regs);
           pc = instruction_pointer(regs);
   } else if (task == NULL || task == current) {
           ...
   }
   ...
   fp = READ_ONCE_TASK_STACK(task, frame->fp);
   pc = READ_ONCE_TASK_STACK(task, frame->ra);
   pc = ftrace_graph_ret_addr(task, &graph_idx, pc,
                              &frame->ra);

When a stack frame's return address is hooked by function_graph
(ret == return_handler), kernel/trace/fgraph.c:ftrace_graph_ret_addr()
evaluates:

   i = *idx ? : task->curr_ret_stack;

With *idx == 0 and task == NULL, this dereferences a NULL task pointer in SSE
context. Should walk_stackframe() be called with current instead of NULL?

[ ... ]

> +unsigned long riscv_perf_out_copy_user(void *dst, const void *src,
> +				       unsigned long n)
> +{
> +	unsigned long ret;
> +

[ ... ]

> +	if (riscv_sse_get_interrupted_context()) {
> +		if (!access_ok(src, n))
> +			return n;
> +		if (!current->mm ||
> +		    (csr_read(CSR_SATP) & SATP_PPN) != virt_to_pfn(current->mm->pgd))
> +			return n;
> +	}
> +
> +	pagefault_disable();
> +	ret = __copy_from_user_inatomic(dst, src, n);
> +	pagefault_enable();
> +
> +	return ret;
> +}
> +#endif

[Severity: High]
What prevents riscv_perf_out_copy_user() from copying host VMM user memory
when an SSE interrupts a running KVM guest?

The comment notes that comparing CSR_SATP against current->mm->pgd avoids
walking an unrelated address space after interrupting a KVM guest. However, on
RISC-V with the H-extension, arch/riscv/kvm/vcpu.c:kvm_riscv_vcpu_enter_exit()
switches hstatus, vsatp, and hgatp while leaving the host HS-mode CSR_SATP
loaded with the host VMM process's current->mm->pgd.

When an SSE with PERF_SAMPLE_STACK_USER interrupts a guest in VU-mode
(SR_PP == 0) or VS-mode (SR_PP == 1),
perf_prepare_sample()->perf_sample_regs_user() populates data->regs_user.regs
(using the guest's regs->sp for VU-mode, or task_pt_regs(current) for
VS-mode), and perf_output_sample_ustack() calls riscv_perf_out_copy_user().
Because CSR_SATP still matches virt_to_pfn(current->mm->pgd), the check
passes and __copy_from_user_inatomic() reads host VMM user memory at the
guest-controlled stack pointer into the perf sample.

Should riscv_perf_out_copy_user() check sse_callchain_is_guest(context) just
like sse_callchain_kernel() and perf_callchain_user() do?

[Severity: High]
Additionally, in riscv_perf_out_copy_user(), can calling access_ok(src, n)
before checking !current->mm (or calling __copy_from_user_inatomic() outside
SSE context when current->mm is NULL) dereference a NULL current->mm when
CONFIG_RISCV_ISA_SUPM is enabled?

In arch/riscv/include/asm/uaccess.h, access_ok() and raw_copy_from_user()
invoke untagged_addr(addr), which passes current->mm to
__untagged_addr_remote():

   static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigned long addr)
   {
           if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) {
                   u8 pmlen = mm->context.pmlen;

If a user task is sampled in do_exit() after exit_mm() has set
current->mm = NULL while is_user_task(current) is still true,
perf_sample_regs_user() sets regs_user->regs = task_pt_regs(current) and
perf_output_sample_ustack()->__output_copy_user() calls
riscv_perf_out_copy_user(dst, src, 0), dereferencing current->mm->context.pmlen
before reaching the !current->mm check.

[ ... ]

> @@ -28,6 +154,15 @@ static bool fill_callchain(void *entry, unsigned long pc)
>  void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
>  			 struct pt_regs *regs)
>  {
> +#ifdef CONFIG_RISCV_SBI_SSE
> +	const struct riscv_sse_interrupted_context *context;
> +
> +	context = riscv_sse_get_interrupted_context();
> +	/* A guest stack cannot be walked using the host address space. */
> +	if (sse_callchain_is_guest(context))
> +		return;
> +#endif
> +
>  	if (perf_guest_state()) {
>  		/* TODO: We don't support guest os callchain now */
>  		return;

[Severity: High]
When perf_callchain_user() runs in SSE context, does it also need the
!current->mm and CSR_SATP page-table root check that riscv_perf_out_copy_user()
performs?

When an SSE interrupts a user task executing in kernel mode
(!user_mode(regs) && is_user_task(current)),
kernel/events/callchain.c:get_perf_callchain() substitutes
regs = task_pt_regs(current) and calls
perf_callchain_user()->arch_stack_walk_user()->unwind_user_frame(), which calls
access_ok() and __copy_from_user_inatomic().

Because SSE can interrupt kernel regions where IRQs are disabled, CSR_SATP or
current->mm may not match the current user task: for example, in
context_switch() after switch_mm_irqs_off() loads next->mm->pgd into CSR_SATP
before __switch_to() updates tp and __sbi_sse_entry_task, during
efi_virtmap_load() when CSR_SATP points to efi_mm.pgd, or in do_exit() after
exit_mm() sets current->mm to NULL. Without checking !current->mm and CSR_SATP
in perf_callchain_user(), unwind_user_frame() can walk the wrong address space
or dereference a NULL current->mm in access_ok().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029074853.35467-1-zhangzhanpeng.jasper@bytedance.com?part=7