Re: [PATCH v4 1/1] kprobes: Make optprobe optimizer multi-generational and asynchronous

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kprobes: orphaned INT3 breakpoint and UAF on unregistered cooling probes
--

commit 03a24f729b70417a26e91442518daed709d91828
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

kprobes: Make optprobe optimizer multi-generational and asynchronous

This commit replaces synchronize_rcu_tasks() with call_rcu_tasks() and
introduces a multi-generational ring to queue probe optimizations and
unoptimizations without blocking or dynamic allocations. Locks are now held
only briefly to patch instructions rather than across the entire Tasks RCU
grace period.

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 4edd8ca5c6578..f92039e2561cf 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
[ ... ]
> @@ -624,67 +696,144 @@ static void do_free_cleaned_kprobes(void)
[ ... ]
> +static void optprobe_finalize_generation(struct optprobe_generation *gen)
> +{
> +	struct optimized_kprobe *op, *tmp;
> +
> +	lockdep_assert_held(&kprobe_mutex);
[ ... ]
> +	/* Finalize unoptimized kprobes whose quiescence period completed */
> +	list_for_each_entry_safe(op, tmp, &gen->cooling_list, list) {
> +		if (kprobe_unused(&op->kp)) {
> +			/*
> +			 * Unregistered while quiescence period was in flight.
> +			 * Move to cur_gen's freeing list for release afterwards.
> +			 */
> +			list_move(&op->list, &optprobe_gens[optprobe_cur_gen].freeing_list);
> +			kick_kprobe_optimizer();

[Severity: Critical]
Does moving this unregistered probe to the freeing_list skip necessary cleanup
steps, leading to an orphaned INT3 breakpoint and a use-after-free?

When optprobe_finalize_generation() sees that the probe is unused and moves it
to freeing_list here, it doesn't call arch_disarm_kprobe() or
hlist_del_init_rcu().

Later, when do_free_cleaned_kprobes() frees the struct via free_aggr_kprobe(),
the probe remains hashed and the breakpoint remains armed in the kernel text.
When the CPU executes the orphaned breakpoint, it will dereference the freed
kprobe struct, leading to a crash.

> +		} else {
> +			/* Still in use; now safely disarmed */
> +			list_del_init(&op->list);
> +			if (!kprobe_disabled(&op->kp))
> +				optimize_kprobe(&op->kp);
> +		}
> +	}
[ ... ]
> @@ -819,7 +1010,17 @@ static void unoptimize_kprobe(struct kprobe *p, bool force)
>  				 * in the freeing list for release afterwards.
>  				 */
>  				force_unoptimize_kprobe(op);
> -				list_move(&op->list, &freeing_list);
> +				list_move(&op->list, &optprobe_gens[optprobe_cur_gen].freeing_list);
> +				kick_kprobe_optimizer();
> +			}
> +		} else if (optprobe_queued_cooling(op)) {
> +			if (force) {
> +				/*
> +				 * Already unoptimized, move to freeing list for
> +				 * release afterwards.
> +				 */
> +				list_move(&op->list, &optprobe_gens[optprobe_cur_gen].freeing_list);
> +				kick_kprobe_optimizer();
>  			}
>  		} else {

[Severity: Critical]
Does this logic correctly handle the case when force is false?

When force is false (which is the default when __disarm_kprobe() calls it),
this block does nothing, leaving the probe on the cooling_list.

Because the probe is left on the cooling_list, kprobe_queued() evaluates to
true during __disarm_kprobe(). This causes arch_disarm_kprobe() to be
skipped, deferring the removal of the INT3 breakpoint. Similarly,
__unregister_kprobe_top() skips hlist_del_rcu(), leaving the probe in the
global hash table.

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