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?
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");
> > +}
> > +
>
> [...]
