Re: [PATCH bpf-next v1 02/20] bpf: Invalidate what a bpf_task_park() sleep may free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: bpf_ringbuf_reserve() pointers kept valid across
  bpf_task_park()
--

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

bpf: Invalidate what a bpf_task_park() sleep may free

This commit teaches the verifier to reject bpf_task_park() calls while live
dynptrs or non-num iterators are on the stack, and to invalidate pointers
whose lifetime depends on the Tasks Trace RCU read section or disabled
migration dropped during the sleep.

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 20b1b9ff574c7..e90cc127aedb8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11222,6 +11222,98 @@ static void invalidate_non_owning_refs(struct bpf_verifier_env *env)
>  	}));
>  }
>  
> +/*
> + * bpf_task_park() sleeps without the Tasks Trace RCU read section and the
> + * disabled migration that a sleepable program otherwise runs under. A dynptr
> + * or an iterator on the stack cannot simply be invalidated across such a
> + * sleep: the program must still release or destroy it, and an object that
> + * stays usable may point at memory the sleep lets go. Refuse the park while
> + * any is live, except a bpf_iter_num, whose state lives entirely on the stack
> + * and whose next and destroy touch nothing else.
> + */
> +static int check_park_stack_objects(struct bpf_verifier_env *env, int insn_idx)
> +{
> +	struct bpf_verifier_state *vstate = env->cur_state;
> +	int i, spi;
> +
> +	for (i = 0; i <= vstate->curframe; i++) {
> +		struct bpf_func_state *frame = vstate->frame[i];
> +
> +		for (spi = 0; spi < bpf_stack_nr_slots(frame); spi++) {
> +			struct bpf_stack_state *slot = bpf_stack_slot(frame, spi);
> +			struct bpf_reg_state *st = &slot->spilled_ptr;
> +
> +			switch (slot->slot_type[BPF_REG_SIZE - 1]) {
> +			case STACK_DYNPTR:
> +				/* Report the slot the program named, not the second half. */
> +				if (!st->dynptr.first_slot)
> +					continue;
> +				verbose(env, "bpf_task_park() with a live dynptr at frame %d fp%d\n",
> +					i, -(spi + 1) * BPF_REG_SIZE);
> +				break;
> +			case STACK_ITER:
> +				if (!st->id)
> +					continue;
> +				if (st->iter.btf == btf_vmlinux &&
> +				    !strcmp(iter_type_str(st->iter.btf, st->iter.btf_id), "num"))
> +					continue;
> +				verbose(env, "bpf_task_park() with a live %s iterator at frame %d fp%d\n",
> +					iter_type_str(st->iter.btf, st->iter.btf_id), i,
> +					-(spi + 1) * BPF_REG_SIZE);
> +				break;
> +			default:
> +				continue;
> +			}
> +			bpf_diag_res(env, insn_idx, "live stack object across bpf_task_park()",
> +				     "A dynptr or iterator on the stack may refer to memory that the sleep in bpf_task_park() lets go, and the verifier cannot invalidate it without losing the path that releases its reference.",
> +				     "Release or destroy the dynptr or iterator before calling bpf_task_park(), or move the park out of the loop that uses it.");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Invalidate what the sleep in bpf_task_park() may free. Keep only pointers
> + * whose lifetime does not rest on the dropped protection: the stack and
> + * instruction metadata; the context and the maps the program references
> + * directly, which the invocation holds; the values of its array maps, which
> + * the map allocates and frees with itself, and which hold the program's
> + * global variables; arena memory, which the arena map holds and whose
> + * accesses tolerate faults; and pointers backed by an explicit
> + * verifier-tracked reference, found through their own id or the id of their
> + * parent. An inner map found through a map-in-map lookup is not kept, nor
> + * are its values: the outer map's element alone holds it, and its deletion
> + * frees it after a grace period the sleeping program no longer delays.
> + * Everything else, values of maps that free elements on delete or on their
> + * owner's teardown, RCU and trusted pointers without a reference, per-CPU
> + * and local storage pointers, is gone after the call.
> + */
> +static void invalidate_regs_across_park(struct bpf_verifier_env *env)
> +{
> +	struct bpf_stack_state *stack;
> +	struct bpf_func_state *state;
> +	struct bpf_reg_state *reg;
> +
> +	bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, stack, 1 << STACK_SPILL, ({
> +		enum bpf_reg_type type = base_type(reg->type);
> +
> +		if (!is_pointer_regtype(reg->type))
> +			continue;
> +		if ((reg->id && find_reference_state(env->cur_state, reg->id)) ||
> +		    (reg->parent_id && find_reference_state(env->cur_state, reg->parent_id)))
> +			continue;

[Severity: High]
Does this allow a PTR_TO_MEM | MEM_RINGBUF sample pointer from
bpf_ringbuf_reserve() to survive across bpf_task_park()?

is_acquire_function() treats BPF_FUNC_ringbuf_reserve as an acquire call,
so check_helper_call() records a REF_TYPE_PTR entry in env->cur_state->refs
for the returned register.  Unlike refcounted kernel objects,
__bpf_ringbuf_reserve() only sets BPF_RINGBUF_BUSY_BIT on the record header
and does not take a reference on the underlying struct bpf_ringbuf_map.

Because BPF_MAP_TYPE_RINGBUF can be used as an inner map in a map-in-map,
its lifetime during a sleepable program is only protected by the outer map
element and the Tasks Trace RCU read section.  If the inner ringbuf map is
removed and freed while bpf_task_park() sleeps, the preserved sample
pointer becomes dangling and a subsequent access or bpf_ringbuf_submit()
triggers a use-after-free.  Like bpf_ringbuf_reserve_dynptr(), should
bpf_task_park() be rejected while a ringbuf reservation is live?

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