Re: [PATCH bpf-next v1 09/20] bpf: Register bpf_task_wake() for all program types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: UAF and refcount UAF in bpf_task_wake_defer() on dying or
  unborn task
--

commit 80d3e640be3a3d90e547a97946f0b4ff6582d291
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Register bpf_task_wake() for all program types

This commit registers bpf_task_wake() in its own kfunc set for all program
types so that any BPF program can wake a parked task.

> diff --git a/kernel/bpf/task_park.c b/kernel/bpf/task_park.c
> index 0e3d7f3ec597e..9de1b496ead1b 100644
> --- a/kernel/bpf/task_park.c
> +++ b/kernel/bpf/task_park.c
> @@ -281,13 +281,31 @@ BTF_KFUNCS_START(bpf_task_park_kfunc_ids)
>  BTF_ID_FLAGS(func, bpf_task_park, KF_SLEEPABLE)
>  BTF_KFUNCS_END(bpf_task_park_kfunc_ids)
>  
> +BTF_KFUNCS_START(bpf_task_wake_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_task_wake, KF_RCU)
> +BTF_KFUNCS_END(bpf_task_wake_kfunc_ids)
> +
>  static const struct btf_kfunc_id_set bpf_task_park_kfunc_set = {
>  	.owner = THIS_MODULE,
>  	.set = &bpf_task_park_kfunc_ids,
>  };
>  
> +static const struct btf_kfunc_id_set bpf_task_wake_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &bpf_task_wake_kfunc_ids,
> +};
> +
> +/*
> + * The park stays with syscall programs, whose runner holds the program across
> + * the sleep. The wake touches only the task and defers itself where it cannot
> + * be issued, so every program type has it, to wake a parked task from the
> + * kernel event that makes it worth waking.
> + */
>  static int __init bpf_task_park_kfunc_init(void)
>  {
> -	return register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_task_park_kfunc_set);
> +	int ret;
> +
> +	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_task_park_kfunc_set);
> +	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC, &bpf_task_wake_kfunc_set);
>  }

[Severity: High]
Can registering bpf_task_wake() for all program types cause a refcount
addition-on-zero and use-after-free in bpf_task_wake_defer() when called on a
task that is being freed?

Exposing bpf_task_wake() to all program types lets lsm, tracing, and
struct_ops programs call it on a trusted task_struct argument after
task->usage and task->rcu_users have already dropped to zero, such as from
__put_task_struct() -> security_task_free(), or during a failing
copy_process() before task->rcu_users is initialized.  When called with
interrupts disabled, bpf_task_wake() defers the wakeup through
bpf_task_wake_defer():

kernel/bpf/task_park.c:bpf_task_wake_defer() {
    ...
	if (cmpxchg_relaxed(&node->next, NULL, WAKE_Q_TAIL))
		return;
	get_task_struct(task);
	head = this_cpu_read(bpf_task_wake_queue);
	do {
		WRITE_ONCE(node->next, head ?: WAKE_Q_TAIL);
	} while (!try_cmpxchg(this_cpu_ptr(&bpf_task_wake_queue), &head, node));
	irq_work_queue(this_cpu_ptr(&bpf_task_wake_work));
}

get_task_struct() increments task->usage from zero while __put_task_struct()
or bad_fork_free proceeds to free the task_struct, so bpf_task_wake_deferred()
later dereferences and puts the freed task from irq_work.

Guarding the deferred wakeup with refcount_inc_not_zero(&task->rcu_users) and
releasing it with put_task_struct_rcu_user(), as bpf_task_acquire() and
bpf_task_work_schedule() do, would avoid queueing a task whose lifetime has
ended or has not yet started.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794269506.git.memxor@gmail.com?part=9