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

On Thu, May 7, 2026 at 12:59 AM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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?

No existing helper and kfunc releases a ref_obj that also has another
dynptr argument.

Kfunc already use this check and I choose to make helper align with
kfunc to make it the same without overcomplicating things. In the
future when there is a universal proto for heleprs and kfuncs, we can
check meta->fn->arg_type[release_regno] to make it more robust.

>
> [ ... ]
> > @@ -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?

If anyone is developing a new kfunc whose ref_obj to be released is
not BPF_REG_1, the verifier should scream "unreleased reference", so
the bug is unlikely to make it into the kernel.

Some reason for this approach: The longterm goal is to remove the
dependency of bpf_reg_state from kfunc proto generation. In this case,
I want to tag OBJ_RELEASE by looking at the proto. This is currently
impossible since we haved enable decl_tag for kfunc arguments yet. I
think hardcode to BPF_REG_1 is a reasonable workaround for now.

If this sounds okay. I will add this temporary constraint to kfuncs.rst.

>
> [ ... ]
> > @@ -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/[email protected]?part=8