| Message ID | 20220621012811.2683313-3-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add bpf_getxattr | expand |
On Tue, Jun 21, 2022 at 06:58:08AM IST, KP Singh 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 | 29 ++++++++++++ > kernel/bpf/verifier.c | 85 ++++++++++++++++++++---------------- > 3 files changed, 79 insertions(+), 37 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 3930c963fa67..60d490354397 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -548,6 +548,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..02d7951591ae 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6162,6 +6162,26 @@ 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); > + if (!strcmp(btf_name_by_offset(btf, t->name_off), "char")) > + return true; > + > + return false; > +} > + > 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 +6364,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 +6375,14 @@ 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])) { Here, we need to check whether reg is a PTR_TO_MAP_VALUE, otherwise in check_const_str, reg->map_ptr may be NULL. Probably best to do it in btf_param_is_const_str_ptr itself. > + err = check_const_str(env, reg, regno); > + if (err < 0) > + return err; > + i++; > + 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 2859901ffbe3..14a434792d7b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5840,6 +5840,52 @@ 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 = reg->map_ptr; > + int map_off; > + u64 map_addr; > + char *str_ptr; > + int err; > + > + 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 +6120,9 @@ 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"); > + err = check_const_str(env, reg, regno); > + if (err < 0) > 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; > - } > } else if (arg_type == ARG_PTR_TO_KPTR) { > if (process_kptr_func(env, regno, meta)) > return -EACCES; > -- > 2.37.0.rc0.104.g0611611a94-goog > -- Kartikeya
On Tue, Jun 21, 2022 at 2:50 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Tue, Jun 21, 2022 at 06:58:08AM IST, KP Singh 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> > > --- [...] > > if (is_kfunc) { > > bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], ®s[regno + 1]); > > @@ -6354,6 +6375,14 @@ 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])) { > > Here, we need to check whether reg is a PTR_TO_MAP_VALUE, otherwise in > check_const_str, reg->map_ptr may be NULL. Probably best to do it in > btf_param_is_const_str_ptr itself. I added it to the check_const_str as: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 14a434792d7b..5300e022398a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5843,12 +5843,16 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state int check_const_str(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno) { - struct bpf_map *map = reg->map_ptr; + 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; > > > + err = check_const_str(env, reg, regno); > > + if (err < 0) > > + return err; > > + i++; > > + continue; > > + } [...] > > -- > > 2.37.0.rc0.104.g0611611a94-goog > > > > -- > Kartikeya
On Mon, Jun 20, 2022 at 6:29 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 | 29 ++++++++++++ > kernel/bpf/verifier.c | 85 ++++++++++++++++++++---------------- > 3 files changed, 79 insertions(+), 37 deletions(-) > [...] > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 668ecf61649b..02d7951591ae 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6162,6 +6162,26 @@ 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)) "if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)" looks clearer to me > + return false; > + > + t = btf_type_skip_modifiers(btf, t->type, NULL); > + if (!strcmp(btf_name_by_offset(btf, t->name_off), "char")) "return !strcmp(btf_name_by_offset(btf, t->name_off), "char")" looks clearer to me here too > + return true; > + > + return false; > +} > + > 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 +6364,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 +6375,14 @@ 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; > + i++; > + continue; If I'm understanding it correctly, this patch is intended to allow helper functions to take in a kfunc as an arg as long as the next arg is the size of the memory. Do we need to check the memory size access here (eg like a call to check_mem_size_reg() in the verifier) to ensure that memory accesses of that size are safe? > + } > + > 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 2859901ffbe3..14a434792d7b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state > return state->stack[spi].spilled_ptr.id; > } [...] > + > 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 +6120,9 @@ 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"); > + err = check_const_str(env, reg, regno); > + if (err < 0) > return err; nit: I don't think you need the if check here since thsi function will return err automatically in the next line > - } > - > - 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; > - } > } else if (arg_type == ARG_PTR_TO_KPTR) { > if (process_kptr_func(env, regno, meta)) > return -EACCES; > -- > 2.37.0.rc0.104.g0611611a94-goog >
On Tue, Jun 21, 2022 at 8:04 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > On Mon, Jun 20, 2022 at 6:29 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 | 29 ++++++++++++ > > kernel/bpf/verifier.c | 85 ++++++++++++++++++++---------------- > > 3 files changed, 79 insertions(+), 37 deletions(-) > > > [...] > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 668ecf61649b..02d7951591ae 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -6162,6 +6162,26 @@ 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)) > "if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)" looks clearer to me > > + return false; > > + > > + t = btf_type_skip_modifiers(btf, t->type, NULL); > > + if (!strcmp(btf_name_by_offset(btf, t->name_off), "char")) > "return !strcmp(btf_name_by_offset(btf, t->name_off), "char")" looks > clearer to me here too Agreed. Updated. > > + return true; > > + > > + return false; > > +} > > + > > 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 +6364,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 +6375,14 @@ 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; > > + i++; > > + continue; > If I'm understanding it correctly, this patch is intended to allow > helper functions to take in a kfunc as an arg as long as the next arg > is the size of the memory. Do we need to check the memory size access > here (eg like a call to check_mem_size_reg() in the verifier) to > ensure that memory accesses of that size are safe? No, this is different. We already have the verification for where we pair a void * pointer to a size argument in the next arg. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/btf.c#n6366 This logic is similar to the verification we do for ARG_PTR_TO_CONST_STR where we do not need a matching size argument and we just check for a null terminated string passed via a R/O map. > > + } > > + > > 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 2859901ffbe3..14a434792d7b 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state > > return state->stack[spi].spilled_ptr.id; > > } > [...] > > + > > 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 +6120,9 @@ 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"); > > + err = check_const_str(env, reg, regno); > > + if (err < 0) > > return err; > nit: I don't think you need the if check here since thsi function will > return err automatically in the next line Makes sense. Fixed. > > > - } > > - > > - 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; > > - } > > } else if (arg_type == ARG_PTR_TO_KPTR) { > > if (process_kptr_func(env, regno, meta)) > > return -EACCES; > > -- > > 2.37.0.rc0.104.g0611611a94-goog > >
On Tue, Jun 21, 2022 at 3:19 PM KP Singh <kpsingh@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 8:04 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > > > On Mon, Jun 20, 2022 at 6:29 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 | 29 ++++++++++++ > > > kernel/bpf/verifier.c | 85 ++++++++++++++++++++---------------- > > > 3 files changed, 79 insertions(+), 37 deletions(-) > > > > > [...] > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > index 668ecf61649b..02d7951591ae 100644 > > > --- a/kernel/bpf/btf.c > > > +++ b/kernel/bpf/btf.c > > > @@ -6162,6 +6162,26 @@ 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)) > > "if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)" looks clearer to me > > > + return false; > > > + > > > + t = btf_type_skip_modifiers(btf, t->type, NULL); > > > + if (!strcmp(btf_name_by_offset(btf, t->name_off), "char")) > > "return !strcmp(btf_name_by_offset(btf, t->name_off), "char")" looks > > clearer to me here too > > Agreed. Updated. > > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > 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 +6364,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 +6375,14 @@ 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; > > > + i++; > > > + continue; > > If I'm understanding it correctly, this patch is intended to allow > > helper functions to take in a kfunc as an arg as long as the next arg > > is the size of the memory. Do we need to check the memory size access > > here (eg like a call to check_mem_size_reg() in the verifier) to > > ensure that memory accesses of that size are safe? I see what confused you, it's the i++ that's incorrectly added here. Kumar spotted it in my next rev. > > No, this is different. We already have the verification for where we pair a > void * pointer to a size argument in the next arg. > > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/btf.c#n6366 > > This logic is similar to the verification we do for ARG_PTR_TO_CONST_STR where > we do not need a matching size argument and we just check for a null > terminated string > passed via a R/O map. > > > > > + } > > > + > > > 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 2859901ffbe3..14a434792d7b 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state > > > return state->stack[spi].spilled_ptr.id; > > > } > > [...] > > > + > > > 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 +6120,9 @@ 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"); > > > + err = check_const_str(env, reg, regno); > > > + if (err < 0) > > > return err; > > nit: I don't think you need the if check here since thsi function will > > return err automatically in the next line > > Makes sense. Fixed. > > > > > > - } > > > - > > > - 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; > > > - } > > > } else if (arg_type == ARG_PTR_TO_KPTR) { > > > if (process_kptr_func(env, regno, meta)) > > > return -EACCES; > > > -- > > > 2.37.0.rc0.104.g0611611a94-goog > > >
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 3930c963fa67..60d490354397 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -548,6 +548,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..02d7951591ae 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6162,6 +6162,26 @@ 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); + if (!strcmp(btf_name_by_offset(btf, t->name_off), "char")) + return true; + + return false; +} + 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 +6364,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 +6375,14 @@ 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; + i++; + 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 2859901ffbe3..14a434792d7b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5840,6 +5840,52 @@ 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 = reg->map_ptr; + int map_off; + u64 map_addr; + char *str_ptr; + int err; + + 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 +6120,9 @@ 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"); + err = check_const_str(env, reg, regno); + if (err < 0) 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; - } } 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 | 29 ++++++++++++ kernel/bpf/verifier.c | 85 ++++++++++++++++++++---------------- 3 files changed, 79 insertions(+), 37 deletions(-)