Re: [PATCH v4 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[KP Singh <kpsingh@kernel.org>]

On Fri, Jun 24, 2022 at 5:03 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 23, 2022 at 9:56 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > kfuncs can handle pointers to memory when the next argument is
> > the size of the memory that can be read and verify these as
> > ARG_CONST_SIZE_OR_ZERO
> >
> > Similarly add support for string constants (const char *) and
> > verify it similar to ARG_PTR_TO_CONST_STR.
> >
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  include/linux/bpf_verifier.h |  2 +
> >  kernel/bpf/btf.c             | 25 ++++++++++
> >  kernel/bpf/verifier.c        | 89 +++++++++++++++++++++---------------
> >  3 files changed, 78 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 81b19669efba..f6d8898270d5 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -560,6 +560,8 @@ 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);
> > +int check_const_str(struct bpf_verifier_env *env,
> > +                   const struct bpf_reg_state *reg, int 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 668ecf61649b..b31e8d8f2d4d 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6162,6 +6162,23 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
> >         return true;
> >  }
> >
> > +static bool btf_param_is_const_str_ptr(const struct btf *btf,
> > +                                      const struct btf_param *param)
> > +{
> > +       const struct btf_type *t;
> > +
> > +       t = btf_type_by_id(btf, param->type);
> > +       if (!btf_type_is_ptr(t))
> > +               return false;
> > +
> > +       t = btf_type_by_id(btf, t->type);
> > +       if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)
> > +               return false;
> > +
> > +       t = btf_type_skip_modifiers(btf, t->type, NULL);
>
> nit: this looks a bit fragile, you assume CONST comes first and then
> skip the rest of modifiers (including typedefs). Maybe either make it
> more permissive and then check that CONST is somewhere there in the
> chain (you'll have to open-code btf_type_skip_modifiers() loop), or
> make it more restrictive and say that it has to be `const char *` and
> nothing else (no volatile, no restrict, no typedefs)?

I did not bother doing that since they are kfuncs and we have a limited set of
types, but I agree that it will confuse someone, someday. So, I updated it.
Also, while I was at it, I moved the comment for the arg_mem_size below
where it should have.

Does this seem okay to you?


diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 9f289b346790..a97e664e4d4d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6166,17 +6166,21 @@ static bool btf_param_is_const_str_ptr(const
struct btf *btf,
                                       const struct btf_param *param)
 {
        const struct btf_type *t;
+       bool is_const = false;

        t = btf_type_by_id(btf, param->type);
        if (!btf_type_is_ptr(t))
                return false;

        t = btf_type_by_id(btf, t->type);
-       if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)
-               return false;
+       while (btf_type_is_modifier(t)) {
+               if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST)
+                       is_const = true;
+               t = btf_type_by_id(btf, t->type);
+       }

-       t = btf_type_skip_modifiers(btf, t->type, NULL);
-       return !strcmp(btf_name_by_offset(btf, t->name_off), "char");
+       return (is_const &&
+               !strcmp(btf_name_by_offset(btf, t->name_off), "char"));
 }

 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
@@ -6366,12 +6370,7 @@ static int btf_check_func_arg_match(struct
bpf_verifier_env *env,
                        if (is_kfunc) {
                                bool arg_mem_size = i + 1 < nargs &&
is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);

-                               /* Permit pointer to mem, but only when argument
-                                * type is pointer to scalar, or struct composed
-                                * (recursively) of scalars.
-                                * When arg_mem_size is true, the pointer can be
-                                * void *.
-                                */
+
                                if (btf_param_is_const_str_ptr(btf, &args[i])) {
                                        err = check_const_str(env, reg, regno);
                                        if (err < 0)
@@ -6379,6 +6378,12 @@ static int btf_check_func_arg_match(struct
bpf_verifier_env *env,
                                        continue;
                                }

+                               /* Permit pointer to mem, but only when argument
+                                * type is pointer to scalar, or struct composed
+                                * (recursively) of scalars.
+                                * When arg_mem_size is true, the pointer can be
+                                * void *.
+                                */
                                if (!btf_type_is_scalar(ref_t) &&
                                    !__btf_type_is_scalar_struct(log,
btf, ref_t, 0) &&
                                    (arg_mem_size ?
!btf_type_is_void(ref_t) : 1)) {


>
> > +       return !strcmp(btf_name_by_offset(btf, t->name_off), "char");
> > +}
> > +
>
> [...]