Re: [PATCH bpf-next v1 01/13] bpf: Refactor ARG_PTR_TO_DYNPTR checks into process_dynptr_func

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Thu, Oct 20, 2022 at 04:29:57AM IST, Joanne Koong wrote:
> On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > ARG_PTR_TO_DYNPTR is akin to ARG_PTR_TO_TIMER, ARG_PTR_TO_KPTR, where
> > the underlying register type is subjected to more special checks to
> > determine the type of object represented by the pointer and its state
> > consistency.
> >
> > Move dynptr checks to their own 'process_dynptr_func' function so that
> > is consistent and in-line with existing code. This also makes it easier
> > to reuse this code for kfunc handling.
> >
> > To this end, remove the dependency on bpf_call_arg_meta parameter by
> > instead taking the uninit_dynptr_regno by pointer. This is only needed
> > to be set to a valid pointer when arg_type has MEM_UNINIT.
> >
> > Then, reuse this consolidated function in kfunc dynptr handling too.
> > Note that for kfuncs, the arg_type constraint of DYNPTR_TYPE_LOCAL has
> > been lifted.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf_verifier.h                  |   8 +-
> >  kernel/bpf/btf.c                              |  17 +--
> >  kernel/bpf/verifier.c                         | 115 ++++++++++--------
> >  .../bpf/prog_tests/kfunc_dynptr_param.c       |   5 +-
> >  .../bpf/progs/test_kfunc_dynptr_param.c       |  12 --
> >  5 files changed, 69 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 9e1e6965f407..a33683e0618b 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -593,11 +593,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
> >                              u32 regno);
> >  int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> >                    u32 regno, u32 mem_size);
> > -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > -                             struct bpf_reg_state *reg);
> > -bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> > -                            struct bpf_reg_state *reg,
> > -                            enum bpf_arg_type arg_type);
> > +int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > +                       enum bpf_arg_type arg_type, int argno,
> > +                       u8 *uninit_dynptr_regno);
> >
> >  /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
> >  static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index eba603cec2c5..1827d889e08a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6486,23 +6486,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                                                 return -EINVAL;
> >                                         }
> >
> > -                                       if (!is_dynptr_reg_valid_init(env, reg)) {
> > -                                               bpf_log(log,
> > -                                                       "arg#%d pointer type %s %s must be valid and initialized\n",
> > -                                                       i, btf_type_str(ref_t),
> > -                                                       ref_tname);
> > +                                       if (process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR, i, NULL))
>
> I think it'd be helpful to add a bpf_log statement here that this failed
>

I left it out because process_dynptr_func itself will do the logging we were
doing here before.

> >                                                 return -EINVAL;
> > -                                       }
> > -
> > -                                       if (!is_dynptr_type_expected(env, reg,
> > -                                                       ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL)) {
> > -                                               bpf_log(log,
> > -                                                       "arg#%d pointer type %s %s points to unsupported dynamic pointer type\n",
> > -                                                       i, btf_type_str(ref_t),
> > -                                                       ref_tname);
> > -                                               return -EINVAL;
> > -                                       }
> > -
> >                                         continue;
> >                                 }
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6f6d2d511c06..31c0c999448e 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -782,8 +782,7 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> >         return true;
> >  }
> >
> > -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > -                             struct bpf_reg_state *reg)
> > +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> >  {
> >         struct bpf_func_state *state = func(env, reg);
> >         int spi = get_spi(reg->off);
> > @@ -802,9 +801,8 @@ bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> >         return true;
> >  }
> >
> > -bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> > -                            struct bpf_reg_state *reg,
> > -                            enum bpf_arg_type arg_type)
> > +static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > +                                   enum bpf_arg_type arg_type)
> >  {
> >         struct bpf_func_state *state = func(env, reg);
> >         enum bpf_dynptr_type dynptr_type;
> > @@ -5573,6 +5571,65 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
> >         return 0;
> >  }
> >
> > +int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > +                       enum bpf_arg_type arg_type, int argno,
>
> Do we need both regno and argno given that regno is always argno +
> BPF_REG_1 and in this function we only use the argno param for "argno
> + 1"? I think we could just pass in regno.
>

Hmm, not really. I can drop it.

> > +                       u8 *uninit_dynptr_regno)
>
> nit: this is personal preference, but I think it looks cleaner passing
> "struct bpf_call_arg_meta *meta" here instead of "u8
> *uninit_dynptr_regno".
>

Right, the thinking was that kfuncs could also handle MEM_UNINIT case, in both
cases the meta type is different but this could be same, but let's think about
that when/if dynptr API function is added as a kfunc.

> > +{
> > +       struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> > +
> > +       /* We only need to check for initialized / uninitialized helper
> > +        * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the
> > +        * assumption is that if it is, that a helper function
> > +        * initialized the dynptr on behalf of the BPF program.
> > +        */
> > +       if (base_type(reg->type) == PTR_TO_DYNPTR)
> > +               return 0;
> > +       if (arg_type & MEM_UNINIT) {
> > +               if (!is_dynptr_reg_valid_uninit(env, reg)) {
> > +                       verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> > +                       return -EINVAL;
> > +               }
> > +
> > +               /* We only support one dynptr being uninitialized at the moment,
> > +                * which is sufficient for the helper functions we have right now.
> > +                */
> > +               if (*uninit_dynptr_regno) {
> > +                       verbose(env, "verifier internal error: multiple uninitialized dynptr args\n");
> > +                       return -EFAULT;
> > +               }
> > +
> > +               *uninit_dynptr_regno = regno;
> > +       } else {
> > +               if (!is_dynptr_reg_valid_init(env, reg)) {
> > +                       verbose(env,
> > +                               "Expected an initialized dynptr as arg #%d\n",
> > +                               argno + 1);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               if (!is_dynptr_type_expected(env, reg, arg_type)) {
> > +                       const char *err_extra = "";
> > +
> > +                       switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > +                       case DYNPTR_TYPE_LOCAL:
> > +                               err_extra = "local";
> > +                               break;
> > +                       case DYNPTR_TYPE_RINGBUF:
> > +                               err_extra = "ringbuf";
> > +                               break;
> > +                       default:
> > +                               err_extra = "<unknown>";
> > +                               break;
> > +                       }
> > +                       verbose(env,
> > +                               "Expected a dynptr of type %s as arg #%d\n",
> > +                               err_extra, argno + 1);
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +
> >  static bool arg_type_is_mem_size(enum bpf_arg_type type)
> >  {
> >         return type == ARG_CONST_SIZE ||
> > @@ -6086,52 +6143,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                 err = check_mem_size_reg(env, reg, regno, true, meta);
> >                 break;
> >         case ARG_PTR_TO_DYNPTR:
> > -               /* We only need to check for initialized / uninitialized helper
> > -                * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the
> > -                * assumption is that if it is, that a helper function
> > -                * initialized the dynptr on behalf of the BPF program.
> > -                */
> > -               if (base_type(reg->type) == PTR_TO_DYNPTR)
> > -                       break;
> > -               if (arg_type & MEM_UNINIT) {
> > -                       if (!is_dynptr_reg_valid_uninit(env, reg)) {
> > -                               verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> > -                               return -EINVAL;
> > -                       }
> > -
> > -                       /* We only support one dynptr being uninitialized at the moment,
> > -                        * which is sufficient for the helper functions we have right now.
> > -                        */
> > -                       if (meta->uninit_dynptr_regno) {
> > -                               verbose(env, "verifier internal error: multiple uninitialized dynptr args\n");
> > -                               return -EFAULT;
> > -                       }
> > -
> > -                       meta->uninit_dynptr_regno = regno;
> > -               } else if (!is_dynptr_reg_valid_init(env, reg)) {
> > -                       verbose(env,
> > -                               "Expected an initialized dynptr as arg #%d\n",
> > -                               arg + 1);
> > -                       return -EINVAL;
> > -               } else if (!is_dynptr_type_expected(env, reg, arg_type)) {
> > -                       const char *err_extra = "";
> > -
> > -                       switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > -                       case DYNPTR_TYPE_LOCAL:
> > -                               err_extra = "local";
> > -                               break;
> > -                       case DYNPTR_TYPE_RINGBUF:
> > -                               err_extra = "ringbuf";
> > -                               break;
> > -                       default:
> > -                               err_extra = "<unknown>";
> > -                               break;
> > -                       }
> > -                       verbose(env,
> > -                               "Expected a dynptr of type %s as arg #%d\n",
> > -                               err_extra, arg + 1);
> > -                       return -EINVAL;
> > -               }
> > +               if (process_dynptr_func(env, regno, arg_type, arg, &meta->uninit_dynptr_regno))
> > +                       return -EACCES;
>
> process_dynptr_func could return -EFAULT so I think we should do "err
> = process_dynptr_func(...)" here instead.
>

Agreed, I'll also propagate errors from other similar named functions.