Re: [PATCH] fprobe: Protect fprobe_return() with guard(rcu)()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tracing: fprobe UAF and lockdep splat due to invalid guard(rcu)
--

commit 4b472c8849c5257c65478348507af6f9fee7c3af
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

fprobe: Protect fprobe_return() with guard(rcu)()

In fprobe_return(), the shadow-stack iteration and exit_handler
invocations were protected by preempt_disable_notrace().
However, unregister_fprobe() and unregister_fprobe_async() (used
by BPF kprobe-multi) rely on standard RCU grace periods (synchronize_rcu()
and call_rcu()) to wait until the fprobe is no longer in use before
freeing it.

In preemptible kernels (CONFIG_PREEMPT_RCU=y), standard RCU grace
periods do not wait for pure preempt_disable_notrace() critical
sections. Consequently, an unregistered fprobe may be freed while
a concurrent CPU executing fprobe_return() is still running
fp->exit_handler(), causing a use-after-free.

To resolve this, protect fprobe_return() with guard(rcu)() matching
fprobe_fgraph_entry(). This ensures both synchronous unregister_fprobe()
and asynchronous unregister_fprobe_async() safely wait for in-flight
exit_handlers to complete via standard RCU grace periods.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-e46bcd68-4a56-4f19-a255-e3772980e5e3
Fixes: 657b594b2084 ("fprobe: Fix unregister_fprobe() to wait for RCU grace period")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 1e9b00997ff27..e34c3f7667848 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -656,7 +656,7 @@ static void fprobe_return(struct ftrace_graph_ret *trace,
>  	size_words = SIZE_IN_LONG(size);
>  	ret_ip = ftrace_regs_get_instruction_pointer(fregs);
>  
> -	preempt_disable_notrace();
> +	guard(rcu)();

[Severity: High]
Since ftrace operations lacking the FTRACE_OPS_FL_RCU flag can execute in
idle contexts where RCU is not watching, does adding guard(rcu)() here
trigger an illegal RCU lock usage splat?

Additionally, since RCU doesn't track read-side critical sections that
start when RCU isn't watching, does this mean synchronize_rcu() during
unregistration will fail to wait for these handlers, leaving the fprobe
vulnerable to the same use-after-free this patch attempts to fix?

[ ... ]


Looking at fprobe_fgraph_entry() in the same file:

kernel/trace/fprobe.c:fprobe_fgraph_entry() {
    ...
	if (WARN_ON_ONCE(!fregs))
		return 0;

	guard(rcu)();
	head = rhltable_lookup(&fprobe_ip_table, &func, fprobe_rht_params);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but fprobe_fgraph_entry() also
uses guard(rcu)(). Could this pre-existing usage cause the same lockdep
splats in idle contexts?


Looking at how fgraph operations are initialized:

kernel/trace/fgraph.c:fgraph_init_ops() {
	dst_ops->flags = FTRACE_OPS_FL_PID | FTRACE_OPS_GRAPH_STUB;

#ifdef CONFIG_DYNAMIC_FTRACE
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of
FTRACE_OPS_FL_RCU in these global fgraph operations expose all fgraph
users to execution when RCU is not watching? Should this flag be added
here to prevent tracing callbacks from being invoked in contexts where
RCU is offline?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179055575009.241711.6358052647499787191.stgit@devnote2?part=1