| Message ID | 20220624045636.3668195-3-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add bpf_getxattr | expand |
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)? > + return !strcmp(btf_name_by_offset(btf, t->name_off), "char"); > +} > + [...]
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], ®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"); > > +} > > + > > [...]
On Fri, Jun 24, 2022 at 6:26 PM KP Singh <kpsingh@kernel.org> wrote: > > 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? > 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"); > > > +} > > > + > > > > [...]
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); + return !strcmp(btf_name_by_offset(btf, t->name_off), "char"); +} + static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, @@ -6344,6 +6361,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, } else if (ptr_to_mem_ok) { const struct btf_type *resolve_ret; u32 type_size; + int err; if (is_kfunc) { bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); @@ -6354,6 +6372,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, * 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) + return err; + continue; + } + 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)) { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a20d7736a5b2..8c1a73e77a1d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5840,6 +5840,56 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state return state->stack[spi].spilled_ptr.id; } +int check_const_str(struct bpf_verifier_env *env, + const struct bpf_reg_state *reg, int regno) +{ + struct bpf_map *map; + int map_off; + u64 map_addr; + char *str_ptr; + int err; + + if (reg->type != PTR_TO_MAP_VALUE) + return -EACCES; + + map = reg->map_ptr; + if (!bpf_map_is_rdonly(map)) { + verbose(env, "R%d does not point to a readonly map'\n", regno); + return -EACCES; + } + + if (!tnum_is_const(reg->var_off)) { + verbose(env, "R%d is not a constant address'\n", regno); + return -EACCES; + } + + if (!map->ops->map_direct_value_addr) { + verbose(env, + "no direct value access support for this map type\n"); + return -EACCES; + } + + err = check_map_access(env, regno, reg->off, map->value_size - reg->off, + false, ACCESS_HELPER); + if (err) + return err; + + map_off = reg->off + reg->var_off.value; + err = map->ops->map_direct_value_addr(map, &map_addr, map_off); + if (err) { + verbose(env, "direct value access on string failed\n"); + return err; + } + + str_ptr = (char *)(long)(map_addr); + if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) { + verbose(env, "string is not zero-terminated\n"); + return -EINVAL; + } + + return 0; +} + static int check_func_arg(struct bpf_verifier_env *env, u32 arg, struct bpf_call_arg_meta *meta, const struct bpf_func_proto *fn) @@ -6074,44 +6124,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, return err; err = check_ptr_alignment(env, reg, 0, size, true); } else if (arg_type == ARG_PTR_TO_CONST_STR) { - struct bpf_map *map = reg->map_ptr; - int map_off; - u64 map_addr; - char *str_ptr; - - if (!bpf_map_is_rdonly(map)) { - verbose(env, "R%d does not point to a readonly map'\n", regno); - return -EACCES; - } - - if (!tnum_is_const(reg->var_off)) { - verbose(env, "R%d is not a constant address'\n", regno); - return -EACCES; - } - - if (!map->ops->map_direct_value_addr) { - verbose(env, "no direct value access support for this map type\n"); - return -EACCES; - } - - err = check_map_access(env, regno, reg->off, - map->value_size - reg->off, false, - ACCESS_HELPER); - if (err) - return err; - - map_off = reg->off + reg->var_off.value; - err = map->ops->map_direct_value_addr(map, &map_addr, map_off); - if (err) { - verbose(env, "direct value access on string failed\n"); - return err; - } - - str_ptr = (char *)(long)(map_addr); - if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) { - verbose(env, "string is not zero-terminated\n"); - return -EINVAL; - } + err = check_const_str(env, reg, regno); } else if (arg_type == ARG_PTR_TO_KPTR) { if (process_kptr_func(env, regno, meta)) return -EACCES;
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(-)