On Fri, Jun 24, 2022 at 6:26 PM KP Singh <kpsingh@xxxxxxxxxx> wrote:
>
> On Fri, Jun 24, 2022 at 5:03 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, Jun 23, 2022 at 9:56 PM KP Singh <kpsingh@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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?
>
yep, thanks!
>
> 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], ®s[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");
> > > +}
> > > +
> >
> > [...]
