Re: [PATCH bpf-next v4 08/12] bpf: Unify release handling for helpers and kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardcoding meta->release_regno = BPF_REG_1 for KF_RELEASE kfuncs creates a Use-After-Free vulnerability for kfuncs releasing non-first arguments.
- [Medium] Incorrect evaluation of the release_dynptr flag leads to false rejection of valid BPF programs.
--

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

bpf: Unify release handling for helpers and kfuncs

Introduce release_reg() to consolidate the release logic shared by both
helpers and kfuncs. The patch determines release_regno from the function
prototype or metadata before argument checking.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 542912c7983f7..e40f09cefa291 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -9870,37 +9928,11 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	}
>  
>  	if (meta.release_regno) {
[ ... ]
> +		struct bpf_reg_state *reg = &regs[meta.release_regno];
> +		bool convert_rcu = (func_id == BPF_FUNC_kptr_xchg) && in_rcu_cs(env) &&
> +				   (reg->type & MEM_ALLOC) && (reg->type & MEM_PERCPU);
> +
> +		err = release_reg(env, reg, convert_rcu, !!meta.dynptr.ref_obj_id);
>  		if (err)
>  			return err;
>  	}

Does passing !!meta.dynptr.ref_obj_id as the release_dynptr flag cause valid
programs to be rejected?

If a kfunc or helper accepts both a normal PTR_TO_BTF_ID to be released and
a referenced dynptr as a normal (non-release) argument,
meta.dynptr.ref_obj_id would be set. This would cause release_reg() to
mistakenly invoke unmark_stack_slots_dynptr() on the non-dynptr release
register, returning -EINVAL.

Could the release condition be strictly derived from the argument type of
the specific release register instead?

[ ... ]
> @@ -12083,6 +12107,9 @@ int bpf_fetch_kfunc_arg_meta(struct bpf_verifier_env *env,
>  
>  	meta->kfunc_flags = *kfunc.flags;
>  
> +	if (is_kfunc_release(meta))
> +		meta->release_regno = BPF_REG_1;
> +
>  	return 0;
>  }

By unconditionally hardcoding BPF_REG_1 here, could this create a
use-after-free if a kernel module defines a release kfunc that frees an
argument other than the first?

If a BPF program passes a dummy referenced object to R1 and the actual
target object to R2, the verifier might drop the reference for R1 while
retaining R2, even though the kfunc frees R2 at runtime.

Should we still rely on dynamically discovering the release register based
on the argument type to avoid this?

[ ... ]
> @@ -12622,13 +12638,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
[ ... ]
>  	if (meta.release_regno) {
> -		struct bpf_reg_state *reg = &regs[meta.release_regno];
> -
> -		if (meta.dynptr.ref_obj_id) {
> -			err = unmark_stack_slots_dynptr(env, reg);
> -		} else {
> -			err = release_reference(env, reg->ref_obj_id);
> -		}
> +		err = release_reg(env, &regs[meta.release_regno], false, !!meta.dynptr.ref_obj_id);
>  		if (err)
>  			return err;
>  	}

Similar to the check_helper_call() case above, does this global dynptr
metadata check safely handle mixed arguments when releasing registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260506142709.2298255-1-ameryhung@gmail.com?part=8