Re: [PATCH 6.12.y v3 2/3] bpf: Propagate referenced dynptr release errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch successfully fixes a kernel panic by propagating the error from `release_reference()` for dynptrs, but leaves identical vulnerabilities in `unmark_stack_slots_iter()` and `mark_ptr_or_null_regs()`, where `WARN_ON_ONCE` is still used and will trigger panics when callbacks interact with caller references.
--

commit aa0864ea661b640ba7fd188bf0b1fcebb309d1b3
Author: Amery Hung <ameryhung@gmail.com>

bpf: Propagate referenced dynptr release errors

This commit propagates the error from release_reference() for dynptrs to
avoid a kernel panic when a BPF callback attempts to release a caller's
reference. It extracts the upstream behavior for the old stable verifier.

Link: https://lore.kernel.org/r/20260529014936.2811085-6-ameryhung@gmail.com

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index dbc9cfcffcd60..65ee9afaa5ff0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -838,7 +838,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
>  	 * Mirror destroy_if_dynptr_stack_slot(), which already invalidates
>  	 * slices by dynptr_id.
>  	 */
> -	WARN_ON_ONCE(release_reference(env, ref_obj_id));
> +	err = release_reference(env, ref_obj_id);
> +	if (err)
> +		return err;

[Severity: High]
This is a pre-existing issue, but while this successfully fixes the
kernel panic for dynptrs, does it leave identical issues in
unmark_stack_slots_iter() and mark_ptr_or_null_regs()?

For iterators, if a BPF program acquires an iterator (e.g., bpf_iter_num_new) on
the caller's stack, passes it as the ctx argument to bpf_loop(), and calls
bpf_iter_num_destroy() on it from within the callback, unmark_stack_slots_iter()
will still trigger a panic because of WARN_ON_ONCE:

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

Similarly, if a BPF program acquires a reference that can be NULL (e.g.,
bpf_sk_lookup_tcp()), spills it to the caller's stack, passes it as the ctx
argument to bpf_loop(), and performs a NULL check on it inside the callback,
mark_ptr_or_null_regs() also triggers a panic:

kernel/bpf/verifier.c:mark_ptr_or_null_regs() {
	...
	if (ref_obj_id && ref_obj_id == id && is_null)
		/* regs[regno] is in the " == NULL" branch.
		 * No one could have freed the reference state before
		 * doing the NULL check.
		 */
		WARN_ON_ONCE(release_reference_state(state, id));
	...
}

Should these two locations also have their errors correctly propagated instead
of triggering a warning?

>  	invalidate_slices_of_dynptr(env, state->stack[spi].spilled_ptr.id);
>  
>  	/* Invalidate any dynptr clones */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829102638.105170-1-xyx2021@mail.ustc.edu.cn?part=2