Re: [PATCH bpf-next v1 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier

[Joanne Koong <joannelkoong@xxxxxxxxx>]

On Thu, Jul 21, 2022 at 10:02 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Wed, Jul 20, 2022 at 07:48:20PM -0700, Joanne Koong wrote:
> > When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> > the ref obj id of the dynptr must be found and then associated with the data
> > slice.
> >
> > The ref obj id of the dynptr must be found *before* the caller saved regs are
> > reset. Without this fix, the ref obj id tracking is not correct for
> > dynptrs that are at an offset from the frame pointer.
> >
> > Please also note that the data slice's ref obj id must be assigned after the
> > ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
> > zero-marked.
> >
> > Fixes: 34d4ef5775f7("bpf: Add dynptr data slices");
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> >  kernel/bpf/verifier.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index c59c3df0fea6..00f9b5a77734 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -7341,6 +7341,22 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >                       }
> >               }
> >               break;
> > +     case BPF_FUNC_dynptr_data:
> > +             /* Find the id of the dynptr we're tracking the reference of.
> > +              * We must do this before we reset caller saved regs.
> > +              *
> > +              * Please note as well that meta.ref_obj_id after the check_func_arg() calls doesn't
> > +              * already contain the dynptr ref obj id, since dynptrs are stored on the stack.
> > +              */
> > +             for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> > +                     if (arg_type_is_dynptr(fn->arg_type[i])) {
> > +                             if (meta.ref_obj_id) {
> > +                                     verbose(env, "verifier internal error: multiple refcounted args in func\n");
> > +                                     return -EFAULT;
> > +                             }
> > +                             meta.ref_obj_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> check_func_arg() is setting meta->ref_obj_id for each arg.
> Can this meta.ref_obj_id assignment be done in check_func_arg()
> instead of looping all args again here.
>
I think so! Will update for v2.
> > +                     }
> > +             }
> >       }
> >
> >       if (err)
> > @@ -7470,20 +7486,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >               /* For release_reference() */
> >               regs[BPF_REG_0].ref_obj_id = id;
> >       } else if (func_id == BPF_FUNC_dynptr_data) {
> > -             int dynptr_id = 0, 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 (dynptr_id) {
> > -                                     verbose(env, "verifier internal error: multiple dynptr args in func\n");
> > -                                     return -EFAULT;
> > -                             }
> > -                             dynptr_id = stack_slot_get_id(env, &regs[BPF_REG_1 + i]);
> > -                     }
> > -             }
> >               /* For release_reference() */
> > -             regs[BPF_REG_0].ref_obj_id = dynptr_id;
> > +             regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> nit. This will be the same as the earlier is_ptr_cast_function().
> Merge the if statements ?
Will do for v2
>
> >       }
> >
> >       do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
Thanks for taking a look at this patch, Jiri and Martin!
> > --
> > 2.30.2
> >