Re: [PATCH 6/8] kprobes: Do not overwrite jump instruction when re-arming unoptimizing probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kprobes: unoptimize_kprobe() unhashes reused aggrprobe with post_handler
--

commit 9db5fa1da3716f288258348f174e01fe00a6f383
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

kprobes: Do not overwrite jump instruction when re-arming unoptimizing probe

This commit avoids calling arch_arm_kprobe() in __arm_kprobe() when a
probe is queued for unoptimization so that the existing jump instruction
is not overwritten with a software breakpoint. It also stops
reuse_unused_kprobe() from clearing KPROBE_FLAG_DISABLED or calling
optimize_kprobe() early, deferring arming and optimization to
register_aggr_kprobe().

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ceacd56245c24..0ef3a8701edad 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1059,19 +1059,11 @@ static int reuse_unused_kprobe(struct kprobe *ap)
>  {
>  	struct optimized_kprobe *op;
>  
> -	/*
> -	 * Unused kprobe MUST be on the way of delayed unoptimizing (means
> -	 * there is still a relative jump) and disabled.
> -	 */
>  	op = container_of(ap, struct optimized_kprobe, kp);
>  	WARN_ON_ONCE(list_empty(&op->list));
> -	/* Enable the probe again */
> -	ap->flags &= ~KPROBE_FLAG_DISABLED;
> -	/* Optimize it again. (remove from 'op->list') */
>  	if (!kprobe_optready(ap))
>  		return -EINVAL;
>  
> -	optimize_kprobe(ap);
>  	return 0;
>  }

[Severity: High]
Can leaving KPROBE_FLAG_DISABLED set and op->list queued here cause
unoptimize_kprobe() to unhash ap from kprobe_table when reusing an unused
aggrprobe for a new kprobe with a post_handler?

In kernel/kprobes.c, register_aggr_kprobe() calls reuse_unused_kprobe(ap)
and then calls add_new_kprobe(ap, p) before clearing KPROBE_FLAG_DISABLED:

register_aggr_kprobe(ap, p)
  if (kprobe_unused(ap)) {
      ret = reuse_unused_kprobe(ap);
      ...
  }
  ...
  add_new_kprobe(ap, p);
  if (kprobe_disabled(ap) && !kprobe_disabled(p)) {
      ap->flags &= ~KPROBE_FLAG_DISABLED;
      if (!kprobes_all_disarmed)
          ret = arm_kprobe(ap);
  }

Inside add_new_kprobe(), if p->post_handler is set, it calls
unoptimize_kprobe(ap, true) before adding p to ap->list:

static void add_new_kprobe(struct kprobe *ap, struct kprobe *p)
{
	if (p->post_handler)
		unoptimize_kprobe(ap, true);	/* Fall back to normal kprobe */

	list_add_rcu(&p->list, &ap->list);
}

Because KPROBE_FLAG_DISABLED is still set on ap, ap->list is still empty,
and op->list is still on unoptimizing_list (or cooling_list),
kprobe_unused(&op->kp) evaluates to true inside unoptimize_kprobe(ap, true):

	if (!list_empty(&op->list)) {
		if (optprobe_queued_unopt(op)) {
			if (force) {
				force_unoptimize_kprobe(op);
				if (kprobe_unused(&op->kp)) {
					if (!kprobe_gone(&op->kp))
						arch_disarm_kprobe(&op->kp);
					hlist_del_init_rcu(&op->kp.hlist);
					list_move(&op->list, &cur->freeing_list);
				} else {
					list_move(&op->list, &cur->cooling_list);
				}
				kick_kprobe_optimizer();
			}
		} else if (optprobe_queued_cooling(op)) {
			if (force && kprobe_unused(&op->kp)) {
				if (!kprobe_gone(&op->kp))
					arch_disarm_kprobe(&op->kp);
				hlist_del_init_rcu(&op->kp.hlist);
				list_move(&op->list, &cur->freeing_list);
				kick_kprobe_optimizer();
			}
		}

It looks like unoptimize_kprobe() will delete ap from kprobe_table via
hlist_del_init_rcu(&op->kp.hlist) and move op->list to cur->freeing_list.
Immediately after, add_new_kprobe() links p onto ap->list and
register_aggr_kprobe() clears KPROBE_FLAG_DISABLED and calls arm_kprobe(ap),
which writes a software breakpoint at ap->addr for a probe that is no longer
in kprobe_table, leading to an unhandled breakpoint trap when ap->addr is
executed and a warning in do_free_cleaned_kprobes().

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