Re: [PATCH bpf-next v3 4/9] bpf: Refactor object relationship tracking and fix dynptr UAF bug

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2026-04-27 at 13:21 -0700, Amery Hung wrote:
> On Fri, Apr 24, 2026 at 3:48 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Tue, 2026-04-21 at 15:10 -0700, Amery Hung wrote:
> > 
> > Tbh, I find current state of affairs with id/ref_obj_id/parent_id hard
> > to follow. The release_reference() is an improvement, but the means by
> > which the fields are propagated to bpf_reg_state objects are convoluted.
> > I wonder if having a separate "object table" in bpf_verifier_state and
> > having bpf_reg_state->id refer to objects within this table would make
> > things more straight forward.
> 
> I think this is a good idea in the long term. First, we need to make
> id a stable and unique object identifier (i.e., always assign id to
> objects; different objects have different ids). Then, we can create
> the object table indexed by id and each entry contains the ref_obj_id
> and parent_id moved from bpf_reg_state. Then, there will be helpers
> managing the lifetime, relationship (e.g., bpf_obj_create,
> bpf_obj_release, bpf_obj_clone) with clear semantics to centralize how
> id,parent_id,ref_obj_id are manipulated.
> 
> If this makes sense, I can send this as a follow up patchset.

I'd play with it a bit, might make sense if we move a lot of objects
into this table. E.g. forgo find_good_pkt_pointers() and keep packet
info as one of the objects, etc.  On the other hand, dynptr and iters
are inherently stack allocated objects, so it might be the case that
the final implementation would be an over-complication.

[...]

> > >  static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > > -                                enum bpf_arg_type arg_type, int insn_idx, int clone_ref_obj_id)
> > > +                                enum bpf_arg_type arg_type, int insn_idx, int parent_id,
> > > +                                struct bpf_dynptr_desc *dynptr)
> > 
> > Having both parent_id and dynptr->parent_id as parameters of this
> > function is very confusing, but I don't have a suggestion on how to
> > better deal with it.
> 
> Does tweaking the argument name and reorder the if-else block make it
> more obvious how these two arguments are used?

Idk, let's go with your original code, maybe?  The eventual goal is to
have a unified handling for both helpers and kfuncs, right?
Meaning that these would be packed into some common arg_info_meta,
with some comments on the structure fields should look ok.

[...]

> > > +/* Release id and objects referencing the id iteratively in a DFS manner */
> > > +static int release_reference(struct bpf_verifier_env *env, int id)
> > > +{
> > > +     u32 mask = (1 << STACK_SPILL) | (1 << STACK_DYNPTR);
> > >       struct bpf_verifier_state *vstate = env->cur_state;
> > > +     struct bpf_idmap *idstack = &env->idmap_scratch;
> > > +     struct bpf_stack_state *stack;
> > >       struct bpf_func_state *state;
> > >       struct bpf_reg_state *reg;
> > > -     int err;
> > > +     int root_id = id, err;
> > > 
> > > -     err = release_reference_nomark(vstate, ref_obj_id);
> > > -     if (err)
> > > -             return err;
> > > +     idstack->cnt = 0;
> > > +     idstack_push(idstack, id);
> > > 
> > > -     bpf_for_each_reg_in_vstate(vstate, state, reg, ({
> > > -             if (reg->ref_obj_id == ref_obj_id)
> > > -                     mark_reg_invalid(env, reg);
> > > -     }));
> > > +     if (find_reference_state(vstate, id))
> > > +             WARN_ON_ONCE(release_reference_nomark(vstate, id));
> > > +
> > > +     while ((id = idstack_pop(idstack))) {
> > > +             bpf_for_each_reg_in_vstate_mask(vstate, state, reg, stack, mask, ({
> > > +                     if (reg->id != id && reg->parent_id != id && reg->ref_obj_id != id)
> > > +                             continue;
> > > +
> > > +                     if (reg->ref_obj_id && id != root_id) {
> > > +                             struct bpf_reference_state *ref_state;
> > > +
> > > +                             ref_state = find_reference_state(env->cur_state, reg->ref_obj_id);
> > > +                             verbose(env, "Unreleased reference id=%d alloc_insn=%d when releasing id=%d\n",
> > > +                                     ref_state->id, ref_state->insn_idx, root_id);
> > > +                             return -EINVAL;
> > > +                     }
> > > +
> > > +                     if (reg->id != id) {
> > > +                             err = idstack_push(idstack, reg->id);
> > > +                             if (err)
> > > +                                     return err;
> > > +                     }
> > > +
> > > +                     if (!stack || stack->slot_type[BPF_REG_SIZE - 1] == STACK_SPILL)
> > > +                             mark_reg_invalid(env, reg);
> > > +                     else if (stack->slot_type[BPF_REG_SIZE - 1] == STACK_DYNPTR)
> > > +                             invalidate_dynptr(env, state, stack);
> > 
> > invalidate_dynptr() rewrites to stack slots, can it be the case that
> > this body of bpf_for_each_reg_in_vstate_mask() is computed for first
> > and second dynptr stack slots, hence triggering invalidate_dynptr() to
> > rewrite three slots instead of two?
> 
> invalidate_dynptr() will mark the two stack slots as STACK_INVALID so
> I didn't bother to check whether it is the first slot or not. Am I
> missing anything?

Oh, right, the == STACK_DYNPTR won't fire for the second slot.
Sorry for the noise.

[...]