On Fri, May 13, 2022 at 2:37 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, May 09, 2022 at 03:42:56PM -0700, Joanne Koong wrote:
> > } else if (is_acquire_function(func_id, meta.map_ptr)) {
> > - int id = acquire_reference_state(env, insn_idx);
> > + int id = 0;
> > +
> > + if (is_dynptr_ref_function(func_id)) {
> > + int i;
> > +
> > + /* Find the id of the dynptr we're acquiring a reference to */
> > + for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> > + if (arg_type_is_dynptr(fn->arg_type[i])) {
> > + if (id) {
> > + verbose(env, "verifier internal error: more than one dynptr arg in a dynptr ref func\n");
> > + return -EFAULT;
> > + }
> > + id = stack_slot_get_id(env, ®s[BPF_REG_1 + i]);
>
> I'm afraid this approach doesn't work.
> Consider:
> struct bpf_dynptr ptr;
> u32 *data1, *data2;
>
> bpf_dynptr_alloc(8, 0, &ptr);
> data1 = bpf_dynptr_data(&ptr, 0, 8);
> data2 = bpf_dynptr_data(&ptr, 8, 8);
> if (data1)
> *data2 = 0; /* this will succeed, but shouldn't */
>
> The same 'id' is being reused for data1 and data2 to make sure
> that bpf_dynptr_put(&ptr); will clear data1/data2,
> but data1 and data2 will look the same in mark_ptr_or_null_reg().
>
> > + }
> > + }
> > + if (!id) {
> > + verbose(env, "verifier internal error: no dynptr args to a dynptr ref func\n");
> > + return -EFAULT;
> > + }
> > + } else {
> > + id = acquire_reference_state(env, insn_idx);
> > + if (id < 0)
> > + return id;
> > + }
> >
> > - if (id < 0)
> > - return id;
> > /* For mark_ptr_or_null_reg() */
> > regs[BPF_REG_0].id = id;
> > /* For release_reference() */
> > @@ -9810,7 +9864,8 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno,
> > u32 id = regs[regno].id;
> > int i;
> >
> > - if (ref_obj_id && ref_obj_id == id && is_null)
> > + if (ref_obj_id && ref_obj_id == id && is_null &&
> > + !is_ref_obj_id_dynptr(state, id))
>
> This bit is avoiding doing release of dynptr's id,
> because id is shared between dynptr and slice's id.
>
> In this patch I'm not sure what is the purpose of bpf_dynptr_data()
> being an acquire function. data1 and data2 are not acquiring.
> They're not incrementing refcnt of dynptr.
>
> I think normal logic of check_helper_call() that does:
> if (type_may_be_null(regs[BPF_REG_0].type))
> regs[BPF_REG_0].id = ++env->id_gen;
>
> should be preserved.
> It will give different id-s to data1 and data2 and the problem
> described earlier will not exist.
>
> The transfer of ref_obj_id from dynptr into data1 and data2 needs to happen,
> but this part:
> u32 ref_obj_id = regs[regno].ref_obj_id;
> u32 id = regs[regno].id;
> int i;
>
> 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 be left alone.
> bpf_dynptr_put(&ptr); will release dynptr and will clear data1 and data2.
> if (!data1)
> will not release dynptr, because data1->id != data1->ref_obj_id.
>
> In other words bpf_dynptr_data() should behave like is_ptr_cast_function().
> It should trasnfer ref_obj_id to R0, but should give new R0->id.
> See big comment in bpf_verifier.h next to ref_obj_id.
Great, thanks for your feedback. I agree with everything you wrote. I
will make these changes for v5 and add your data1 data2 example as a
test case.
