Re: [PATCH 4/8] kprobes: Switch ftrace ops when adding probe with post_handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kprobes: premature kprobe publication onto ap->list in register_aggr_kprobe()
- [High] kprobes: unchecked rollback arm_kprobe_ftrace() in register_aggr_kprobe()
--

commit 90db6f90ad3cb02409cec15b4dabeb1b192665e6
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

kprobes: Switch ftrace ops when adding probe with post_handler

This commit switches an armed ftrace-based kprobe from kprobe_ftrace_ops to
kprobe_ipmodify_ops in register_aggr_kprobe() when a new kprobe with a
post_handler is added to an aggregated kprobe that previously had no
post_handler. It also restores the ftrace ops registration and counter if
ftrace_set_filter_ip() fails in __disarm_kprobe_ftrace().

> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 0b41d69540ae4..135fec5d49cf5 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c

[ ... ]

> @@ -1654,18 +1657,51 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
>  					| KPROBE_FLAG_DISABLED;
>  		}
>  
> +		if (p->post_handler && !ap->post_handler) {
> +			reset_post_handler = true;
> +			if (kprobe_ftrace(ap) && !kprobes_all_disarmed &&
> +			    !kprobe_disabled(ap))
> +				ftrace_switch_ops = true;
> +			else
> +				ap->post_handler = aggr_post_handler;
> +		}
> +
>  		/* Copy the insn slot of 'p' to 'ap'. */
>  		copy_kprobe(ap, p);
> -		ret = add_new_kprobe(ap, p);
> +		add_new_kprobe(ap, p);
> +	}

[Severity: High]
When ftrace_switch_ops is true in register_aggr_kprobe(), add_new_kprobe(ap, p)
publishes p onto ap->list via list_add_rcu() while ap is still armed on
kprobe_ftrace_ops and ap->post_handler is still NULL.

If another CPU hits the probed function before disarm_kprobe_ftrace(ap)
finishes updating the ftrace callsite, kprobe_ftrace_handler() will call
aggr_pre_handler() -> p->pre_handler(), and then skip aggr_post_handler()
because ap->post_handler is NULL:

arch/x86/kernel/kprobes/ftrace.c:kprobe_ftrace_handler() {
	...
	if (!p->pre_handler || !p->pre_handler(p, regs)) {
		if (unlikely(p->post_handler)) {
			instruction_pointer_set(regs, ip + MCOUNT_INSN_SIZE);
			kcb->kprobe_status = KPROBE_HIT_SSDONE;
			p->post_handler(p, regs, 0);
		}
	...
}

In addition, p->pre_handler() executes under kprobe_ftrace_ops before
arm_kprobe_ftrace(ap) has verified that kprobe_ipmodify_ops
(FTRACE_OPS_FL_IPMODIFY) does not conflict with an active livepatch. A similar
window also occurs in the error path below if arm_kprobe_ftrace(ap) fails,
where ap->post_handler is reset to NULL and ap is re-armed on
kprobe_ftrace_ops before list_del_rcu(&p->list) removes p.

Could add_new_kprobe(ap, p) be deferred until after ap has been switched to
kprobe_ipmodify_ops, or could p be removed from ap->list before re-arming ap
on rollback?

> +
> +	if (ftrace_switch_ops) {
> +		/*
> +		 * Must switch ftrace ops outside of text_mutex and cpus_read_lock
> +		 * to prevent deadlocks when ftrace code modification acquires text_mutex.
> +		 */
> +		ret = disarm_kprobe_ftrace(ap);
> +		if (ret) {
> +			list_del_rcu(&p->list);
> +			synchronize_rcu();
> +			return ret;
> +		}
> +		ap->post_handler = aggr_post_handler;
> +		ret = arm_kprobe_ftrace(ap);
> +		if (ret) {
> +			ap->post_handler = NULL;
> +			arm_kprobe_ftrace(ap);
> +			list_del_rcu(&p->list);
> +			synchronize_rcu();
> +			return ret;
> +		}
>  	}

[Severity: High]
What happens if the rollback call to arm_kprobe_ftrace(ap) in
register_aggr_kprobe() also fails (for example, with -ENOMEM in
ftrace_set_filter_ip() -> alloc_and_copy_ftrace_hash())?

Because the return value of the rollback arm_kprobe_ftrace(ap) is ignored and
KPROBE_FLAG_DISABLED is not set on ap->flags, ap remains marked as enabled
even though it is no longer registered in kprobe_ftrace_ops's filter hash.

When the remaining probe on ap is later disabled or unregistered via
unregister_kprobe() -> __unregister_kprobe_top() -> __disable_kprobe(),
__disable_kprobe() sees !kprobe_disabled(orig_p) and calls
disarm_kprobe_ftrace(), which fails with -ENOENT in ftrace_set_filter_ip(),
triggers the WARN_ONCE() in __disarm_kprobe_ftrace(), and aborts
unregistration while leaving the probe hashed in kprobe_table:

kernel/kprobes.c:__disable_kprobe() {
	...
	if (!kprobes_all_disarmed && !kprobe_disabled(orig_p)) {
		ret = disarm_kprobe(orig_p, true);
		if (ret) {
			p->flags &= ~KPROBE_FLAG_DISABLED;
			return ERR_PTR(ret);
		}
	}
	...
}

Should the rollback path check the return value of arm_kprobe_ftrace(ap) and
set KPROBE_FLAG_DISABLED on ap->flags if re-arming fails?

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