Re: [PATCH bpf-next 1/2] bpf: Only check nmi_uaccess_okay() when signalling current

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Multi-line comment does not follow BPF subsystem standards.

Pre-existing issues:
- [High] A TOCTOU data race on the per-CPU `send_signal_work` structure allows NMI reentrancy to corrupt irq_work data and leak a task_struct reference.
--

commit b0fb28992ab88d35a628c6d9650cd4ab7e67c397
Author: Aditya Sharma <adi.sharma@zohomail.in>

bpf: Only check nmi_uaccess_okay() when signalling current

nmi_uaccess_okay() takes no task argument and is a statement about current.
This commit restricts the check to only apply when signalling the current
task, fixing a bug where bpf_send_signal_task() returned -EPERM for kernel
threads.

Link: https://lore.kernel.org/bpf/20260819124324.43162-1-adi.sharma@zohomail.in/T/#u

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 29260951aa871..f7a41f2225999 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -871,7 +871,10 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type, struct task_struc
>  	 */
>  	if (unlikely(task->flags & (PF_KTHREAD | PF_EXITING)))
>  		return -EPERM;
> -	if (unlikely(!nmi_uaccess_okay()))
> +	/* Since nmi_uaccess_okay() is only for the current
> +	 * task, check if task is current.
> +	 */

[Severity: Low]
This isn't a bug, but does this newly added comment violate the BPF subsystem
multi-line comment style?

The BPF subsystem strictly requires multi-line comments to have the opening
/* on its own line, with the text starting on the next line.

> +	if (task == current && unlikely(!nmi_uaccess_okay()))
>  		return -EPERM;
>  	/* Task should not be pid=1 to avoid kernel panic. */
>  	if (unlikely(is_global_init(task)))

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU data race on the per-CPU
send_signal_work structure later in bpf_send_signal_common()?

If a BPF program executing in a context with IRQs disabled calls
bpf_send_signal(), it evaluates irq_work_is_busy() as false. Before it
queues the work, an NMI could fire on the same CPU and also call
bpf_send_signal():

kernel/trace/bpf_trace.c:bpf_send_signal_common() {
        ...
	if (preempt_count() != 0 || irqs_disabled()) {
                ...
		work = this_cpu_ptr(&send_signal_work);
		if (irq_work_is_busy(&work->irq_work))
			return -EBUSY;

                /* NMI fires here, calls bpf_send_signal_common(), evaluating
                 * irq_work_is_busy() as false since it's not queued yet.
                 * NMI writes its data and queues the work.
                 */

		work->task = get_task_struct(task);
		work->has_siginfo = siginfo == &info;
		if (work->has_siginfo)
			copy_siginfo(&work->info, &info);
		work->sig = sig;
		work->type = type;
		irq_work_queue(&work->irq_work);
		return 0;
	}
        ...
}

Upon return from the NMI, the interrupted context resumes, overwrites
work->task with a new get_task_struct(), and fails to queue the work since
the NMI already queued it.

Could this allow NMI reentrancy to corrupt irq_work data and permanently leak
a task_struct reference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902150439.692725-1-adi.sharma@zohomail.in?part=1