Re: [PATCH v10 6/9] perf: RISC-V: support callchains with SSE delivery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Kernel callchains for SSE PMU events are incorrectly disabled when CONFIG_FRAME_POINTER is not set.
- [High] arch_stack_walk_user still uses __copy_from_user_inatomic, triggering exception-table handled hardware page faults from the SSE handler context.

Pre-existing issues:
- [High] Missing access_ok() check in the non-SSE fast path allows dumping arbitrary kernel memory.
--

commit 43924918205cbb887aeaebe13586db0a4ccfaa5d
Author: Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com>

perf: RISC-V: support callchains with SSE delivery

This commit adds support for perf callchains through the RISC-V SSE PMU
overflow delivery. It attempts to provide a non-faulting user memory copy
for DWARF stacks and restricts kernel stack walks when interrupted contexts
are not safe.

> diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
> index b465bc9eb870e..ec75689c7aecb 100644
> --- a/arch/riscv/kernel/perf_callchain.c
> +++ b/arch/riscv/kernel/perf_callchain.c
[ ... ]
> +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]
Does this code silently break kernel callchains for kernels compiled without
CONFIG_FRAME_POINTER?

When an SSE PMU overflow occurs, sse_kernel_regs_safe() unconditionally reads
frame_pointer(regs) and verifies it falls within the task's stack bounds. If
CONFIG_FRAME_POINTER is disabled, s0 is a general-purpose register containing
arbitrary values, causing the bounds check to fail and the stack walk to be
discarded.

The non-frame-pointer implementation of walk_stackframe() unwinds sequentially
using only sp and pc, so enforcing a valid fp seems unnecessary and breaks the
heuristic stack walker.

[ ... ]
> +unsigned long riscv_perf_out_copy_user(void *dst, const void *src,
> +				       unsigned long n)
> +{
> +	unsigned long addr = (unsigned long)src;
> +	unsigned long copied = 0;
> +
> +	/* Keep the generic fast path unchanged outside an SSE handler. */
> +	if (!riscv_sse_get_interrupted_context()) {
> +		unsigned long ret;
> +
> +		pagefault_disable();
> +		ret = __copy_from_user_inatomic(dst, src, n);
> +		pagefault_enable();
> +		return ret;
> +	}

[Severity: High]
This is a pre-existing issue, but does this missing access_ok() check in the
non-SSE fast path allow arbitrary kernel memory reads?

If riscv_sse_get_interrupted_context() returns false, the function directly
calls __copy_from_user_inatomic() without verifying access_ok(src, n). On
RISC-V, this translates to raw_copy_from_user(), which executes loads in
supervisor mode.

Since supervisor mode has inherent read access to kernel pages, the loads
succeed regardless of the SUM bit. A malicious user could craft a perf event
with PERF_SAMPLE_STACK_USER, spoof their regs->sp to a kernel address, and
trigger a PMU interrupt to dump kernel memory into the perf ring buffer.

[ ... ]
>  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;
>  	}
>  
>  	arch_stack_walk_user(fill_callchain, entry, regs);
>  }

[Severity: High]
Is it safe to invoke arch_stack_walk_user() unconditionally from the SSE PMU
overflow handler?

The commit message states "User callchains continue through the existing
nofault RISC-V user unwinder", but arch_stack_walk_user() is not completely
fault-free. It calls unwind_user_frame(), which uses
__copy_from_user_inatomic():

arch/riscv/kernel/stacktrace.c:unwind_user_frame() {
    ...
    if (__copy_from_user_inatomic(&buftail, user_frame_tail,
                                  sizeof(buftail)))
        return 0;
    ...
}

If the user stack page is not resident, this triggers an exception-table
handled hardware page fault. While the exception table handles the fault
gracefully, the CPU still takes a hardware trap through handle_exception(),
which routes execution through the exact dangerous trap entry path this
patch intends to avoid during an SSE PMU handler.

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