| Message ID | 20220621204642.2891979-3-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add bpf_getxattr | expand |
On Wed, Jun 22, 2022 at 02:16:39AM 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 | 26 +++++++++++ > kernel/bpf/verifier.c | 89 +++++++++++++++++++++--------------- > 3 files changed, 79 insertions(+), 38 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..6608e8a0c5ca 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; > + Forgot to change this to !=. > + 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,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++; Sorry for not seeing it before, I think this i++ is incorrect. It is skipping over the next argument. Which means mem, len pair is not being seen, otherwise it should have given an error with the void * argument, because the next argument does not have __sz prefix, so there is no mem, len pair in the kfunc args. The i++ is done for arg_mem_size case because we processed both argno and argno + 1 together, so the next size arg doesn't need to be processed. So the bpf_getxattr declaration needs to change from: noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode, const char *name, void *value, int size) to noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode, const char *name, void *value, int value__sz) You only need the __sz suffix, the part before that is your choice. Then it will actually check the size for the value pointer. Also, I think neither noinline nor __weak are needed. > + 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..7ac501122df0 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; > -- > 2.37.0.rc0.104.g0611611a94-goog > -- Kartikeya
On Tue, Jun 21, 2022 at 8:27 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, Jun 22, 2022 at 02:16:39AM 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 | 26 +++++++++++ > > kernel/bpf/verifier.c | 89 +++++++++++++++++++++--------------- > > 3 files changed, 79 insertions(+), 38 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..6608e8a0c5ca 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; > > + > > Forgot to change this to !=. > > > + 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,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++; > > Sorry for not seeing it before, I think this i++ is incorrect. It is skipping > over the next argument. Which means mem, len pair is not being seen, otherwise > it should have given an error with the void * argument, because the next > argument does not have __sz prefix, so there is no mem, len pair in the kfunc > args. Thanks for spotting this. Updated my patches, will send v4. - KP > The i++ is done for arg_mem_size case because we processed both argno and argno + 1 > together, so the next size arg doesn't need to be processed. > > So the bpf_getxattr declaration needs to change from: > > noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode, > const char *name, void *value, int size) > > to > > noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode, > const char *name, void *value, int value__sz) > > You only need the __sz suffix, the part before that is your choice. > Then it will actually check the size for the value pointer. > Also, I think neither noinline nor __weak are needed. > > > + 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..7ac501122df0 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; > > -- > > 2.37.0.rc0.104.g0611611a94-goog > > > > -- > Kartikeya
On Tue, Jun 21, 2022 at 8:27 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > [...] > > Sorry for not seeing it before, I think this i++ is incorrect. It is skipping > over the next argument. Which means mem, len pair is not being seen, otherwise > it should have given an error with the void * argument, because the next > argument does not have __sz prefix, so there is no mem, len pair in the kfunc > args. > > The i++ is done for arg_mem_size case because we processed both argno and argno + 1 > together, so the next size arg doesn't need to be processed. > > So the bpf_getxattr declaration needs to change from: > > noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode, > const char *name, void *value, int size) > > to > > noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode, > const char *name, void *value, int value__sz) > > You only need the __sz suffix, the part before that is your choice. > Then it will actually check the size for the value pointer. > Also, I think neither noinline nor __weak are needed. > So, yes, I thought about this and you are right, noinline and __weak are indeed not needed. The compiler will inline the call sites. But there are no call sites to be inlined in the kernel. So, we are good with just silencing the compiler warnings
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..6608e8a0c5ca 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,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..7ac501122df0 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 | 26 +++++++++++ kernel/bpf/verifier.c | 89 +++++++++++++++++++++--------------- 3 files changed, 79 insertions(+), 38 deletions(-)