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) <[email protected]> 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
