Re: [PATCH bpf-next v2 5/6] bpf: Bind iterator elements to their iterator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: iterator elements stay PTR_TRUSTED across next() in check_kfunc_call()
- [High] bpf: missing parent_id reference propagation in check_func_arg() and check_kfunc_call()
--

commit 5ca4ad8c9e1677551effb1ab248bfb034b276685
Author: Justin Suess <utilityemal77@gmail.com>

bpf: Bind iterator elements to their iterator

This commit sets parent_id on PTR_TRUSTED elements returned by open-coded
iterator next() kfuncs to the iterator's reference id so that destroying the
iterator invalidates the returned element and any trusted fields derived from
it.

> diff --git a/Documentation/bpf/bpf_iterators.rst b/Documentation/bpf/bpf_iterators.rst
> index 189e3ec1c6c8e..08c9c15b59acd 100644
> --- a/Documentation/bpf/bpf_iterators.rst
> +++ b/Documentation/bpf/bpf_iterators.rst
> @@ -79,7 +79,11 @@ Constructor/next/destructor implementation contract is as follows:
>      will eventually return NULL when elements are exhausted. Once NULL is
>      returned, subsequent next calls *should keep returning NULL*. Next method
>      is marked with KF_ITER_NEXT (and should also have KF_RET_NULL as
> -    NULL-returning kfunc, of course).
> +    NULL-returning kfunc, of course). An element is only guaranteed to be
> +    valid until the iterator is destroyed, so the verifier invalidates it,
> +    and every trusted pointer derived from it, at `bpf_iter_<type>_destroy()`.
> +    Elements of an RCU-protected iterator (see KF_RCU_PROTECTED) are instead
> +    valid until the end of the RCU critical section.
>  
>    - destructor, `bpf_iter_<type>_destroy()`, is always called once. Even if
>      constructor failed or next returned nothing.  Destructor frees up any
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c85c83a6ee63a..9ce5517dc1a02 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15504,6 +15504,19 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			regs[BPF_REG_0].btf = desc_btf;
>  			regs[BPF_REG_0].type = type;
>  			regs[BPF_REG_0].btf_id = ptr_type_id;
> +
> +			/*
> +			 * A trusted element is pinned by the iterator that
> +			 * produced it and dies with it, like the memory
> +			 * returned by bpf_iter_num_next() above. An RCU
> +			 * element is bounded by the critical section instead.
> +			 */
> +			if (bpf_is_iter_next_kfunc(&meta) && (type & PTR_TRUSTED)) {
> +				err = validate_ref_obj(env, &meta.ref_obj);
> +				if (err)
> +					return err;
> +				regs[BPF_REG_0].parent_id = meta.ref_obj.id;
> +			}

[Severity: Critical]
Does binding parent_id to the iterator's reference id still leave elements
and derived pointers valid across subsequent bpf_iter_<type>_next() calls?

In process_iter_arg(), unmark_stack_slots_iter() (which calls
release_reference(env, st->id)) is only invoked when is_iter_destroy_kfunc()
is true:

kernel/bpf/verifier.c:process_iter_arg() {
	...
	meta->iter.spi = spi;
	meta->iter.frameno = reg->frameno;
	update_ref_obj(&meta->ref_obj, &bpf_stack_slot(state, spi)->spilled_ptr);

	if (is_iter_destroy_kfunc(meta)) {
		err = unmark_stack_slots_iter(env, reg, nr_slots);
		if (err)
			return err;
	}
	...
}

However, non-RCU open-coded iterator next() implementations drop the
previous element's reference during the next bpf_iter_<type>_next() call
(including the final call that returns NULL), before destroy is called:

- bpf_iter_task_vma_next() calls bpf_iter_task_vma_snapshot_reset(snap),
  which calls fput(snap->vm_file) and overwrites the snapshot.
- bpf_iter_kmem_cache_next() decrements prev->refcount and calls
  kmem_cache_destroy(prev) when prev->refcount == 1.
- bpf_iter_dmabuf_next() calls dma_buf_iter_next(kit->dmabuf), which calls
  dma_buf_put() on the previous dma_buf.
- bpf_iter_css_task_next() calls css_task_iter_next(kit->css_it), which
  calls put_task_struct(it->cur_task) on the previous task.

If a BPF program saves an element pointer or a trusted field walked from it
(such as file = vma->vm_file) in a callee-saved register or stack spill,
calls bpf_iter_<type>_next() again, and then accesses the saved pointer
before calling bpf_iter_<type>_destroy(), won't the pointer remain
PTR_TRUSTED after the underlying reference has been dropped?

Also, should Documentation/bpf/bpf_iterators.rst state that an element is
only valid until the next bpf_iter_<type>_next() call or
bpf_iter_<type>_destroy()?

[Severity: High]
What happens when a pointer pinned via parent_id (such as vma->vm_file) is
passed to bpf_dynptr_from_file() or a kfunc returning a borrowed
PTR_TO_BTF_ID such as bpf_real_data_inode()?

In check_func_arg(), meta->ref_obj is only updated when reg_is_referenced()
is true, which checks cur_state->refs for reg->id and ignores reg->parent_id:

kernel/bpf/verifier.c:check_func_arg() {
	...
	if (reg_is_referenced(env, reg))
		update_ref_obj(&meta->ref_obj, reg);
	...
}

When file = vma->vm_file (which has reg->parent_id set to the iterator's id
and reg_is_referenced() == false) is passed to bpf_dynptr_from_file(),
mark_stack_slots_dynptr() sees ref_obj->id == 0 and calls
acquire_reference(env, insn_idx, 0). Doesn't this create the file dynptr
with parent_id = 0, allowing the dynptr and its slices to outlive
bpf_iter_task_vma_destroy() after fput(snap->vm_file) has run?

Note also that if a child reference is bound to the iterator's reference id
and left unreleased at destroy time, unmark_stack_slots_iter() ignores the
-EINVAL return from release_reference():

kernel/bpf/verifier.c:unmark_stack_slots_iter() {
	...
	if (i == 0)
		WARN_ON_ONCE(release_reference(env, st->id));
	...
}

Similarly, in check_kfunc_call() above, setting regs[BPF_REG_0].parent_id is
gated on bpf_is_iter_next_kfunc(&meta) (unlike the PTR_TO_MEM branch above
it). If a BPF program calls bpf_real_data_inode(file) on either an acquired
file or vma->vm_file, won't the returned trusted struct inode * have
parent_id == 0 and remain valid after bpf_put_file() or
bpf_iter_task_vma_destroy()?

>  		}
>  
>  		if (is_kfunc_ret_null(&meta)) {

([Morph](https://morph.so))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020145059.2146170-1-utilityemal77@gmail.com?part=5