Re: [PATCH bpf-next v1 08/20] bpf: Register bpf_task_park() for syscall programs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: bpf_ringbuf_reserve() pointer survives bpf_task_park() in
  syscall progs
--

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

bpf: Register bpf_task_park() for syscall programs

This commit registers bpf_task_park() as a sleepable kfunc for
BPF_PROG_TYPE_SYSCALL programs and documents in bpf_prog_run_pin_on_cpu()
that syscall programs may migrate across a sleep in bpf_task_park().

> diff --git a/kernel/bpf/task_park.c b/kernel/bpf/task_park.c
> index 5043ccdfc0b8d..0e3d7f3ec597e 100644
> --- a/kernel/bpf/task_park.c
> +++ b/kernel/bpf/task_park.c
> @@ -276,3 +276,18 @@ __bpf_kfunc int bpf_task_wake(struct task_struct *task, u64 flags)
>  }
>  
>  __bpf_kfunc_end_defs();
> +
> +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)
> +
> +static const struct btf_kfunc_id_set bpf_task_park_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &bpf_task_park_kfunc_ids,
> +};
> +
> +static int __init bpf_task_park_kfunc_init(void)
> +{
> +	return register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_task_park_kfunc_set);
> +}
> +late_initcall(bpf_task_park_kfunc_init);

[Severity: High]
Can a BPF_PROG_TYPE_SYSCALL program hold a bpf_ringbuf_reserve() reservation
across bpf_task_park() and trigger a use-after-free when the ringbuf is an
inner map?

While check_park_stack_objects() rejects live STACK_DYNPTR slots (such as a
reservation from bpf_ringbuf_reserve_dynptr()), bpf_ringbuf_reserve() returns
a PTR_TO_MEM | MEM_RINGBUF register with a reference state ID tracked in
env->cur_state->refs.  invalidate_regs_across_park() keeps any pointer backed
by a reference state:

kernel/bpf/verifier.c:invalidate_regs_across_park() {
	bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, stack, 1 << STACK_SPILL, ({
		...
		if ((reg->id && find_reference_state(env->cur_state, reg->id)) ||
		    (reg->parent_id && find_reference_state(env->cur_state, reg->parent_id)))
			continue;
}

However, bpf_ringbuf_reserve() only sets BPF_RINGBUF_BUSY_BIT on the record
header and does not take a reference on the underlying bpf_ringbuf_map.  When
the reservation comes from an inner BPF_MAP_TYPE_RINGBUF map looked up from a
map-in-map container, only the Tasks Trace RCU read-side critical section
keeps the inner map alive.  When bpf_task_park() drops rcu_read_unlock_trace()
and sleeps, the inner map can be deleted from the outer map and freed via
bpf_ringbuf_free() -> vunmap() once the grace period completes.  After
bpf_task_park() returns, the program still holds the valid MEM_RINGBUF
pointer and must pass it to bpf_ringbuf_submit() or bpf_ringbuf_discard(),
touching unmapped memory.  Even on a direct ringbuf map, parking with an
uncommitted reservation leaves BPF_RINGBUF_BUSY_BIT set across an unbounded
sleep, stalling ringbuf consumers.

Should the verifier refuse bpf_task_park() when any MEM_RINGBUF reservation
is live, just as it does for STACK_DYNPTR?

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