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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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 = ®s[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.
