| Message ID | 20220712145850.599666-6-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On 7/12/22 7:58 AM, Benjamin Tissoires wrote: > When a kfunc is not returning a pointer to a struct but to a plain type, > we can consider it is a valid allocated memory assuming that: > - one of the arguments is either called rdonly_buf_size or > rdwr_buf_size > - and this argument is a const from the caller point of view > > We can then use this parameter as the size of the allocated memory. > > The memory is either read-only or read-write based on the name > of the size parameter. If I understand correctly, this permits a kfunc like int *kfunc(..., int rdonly_buf_size); ... int *p = kfunc(..., 20); so the 'p' points to a memory buffer with size 20. This looks like a strange interface although probably there is a valid reason for this as I didn't participated in earlier discussions. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > changes in v6: > - code review from Kartikeya: > - remove comment change that had no reasons to be > - remove handling of PTR_TO_MEM with kfunc releases > - introduce struct bpf_kfunc_arg_meta > - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match > - reverted most of the changes in verifier.c > - make sure kfunc acquire is using a struct pointer, not just a plain > pointer > - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free > the allocated memory > > changes in v5: > - updated PTR_TO_MEM comment in btf.c to match upstream > - make it read-only or read-write based on the name of size > > new in v4 > --- > include/linux/bpf.h | 10 ++++++- > include/linux/btf.h | 12 ++++++++ > kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- > kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- > 4 files changed, 121 insertions(+), 17 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 2b21f2a3452f..5b8eadb6e7bc 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1916,12 +1916,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, > const char *func_name, > struct btf_func_model *m); > > +struct bpf_kfunc_arg_meta { > + u64 r0_size; > + bool r0_rdonly; > + int ref_obj_id; > + bool multiple_ref_obj_id; > +}; > + > struct bpf_reg_state; > int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *regs); > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > - struct bpf_reg_state *regs); > + struct bpf_reg_state *regs, > + struct bpf_kfunc_arg_meta *meta); > int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *reg); > int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog, > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 1bfed7fa0428..31da4273c2ec 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -420,4 +420,16 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt > } > #endif > > +static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t) > +{ > + /* t comes in already as a pointer */ > + t = btf_type_by_id(btf, t->type); > + > + /* allow const */ > + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) > + t = btf_type_by_id(btf, t->type); > + > + return btf_type_is_struct(t); > +} > + > #endif > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 4423045b8ff3..552d7bc05a0c 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6168,10 +6168,36 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, > return true; > } > > +static bool btf_is_kfunc_arg_mem_size(const struct btf *btf, > + const struct btf_param *arg, > + const struct bpf_reg_state *reg, > + const char *name) > +{ > + int len, target_len = strlen(name); > + const struct btf_type *t; > + const char *param_name; > + > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > + return false; > + > + param_name = btf_name_by_offset(btf, arg->name_off); > + if (str_is_empty(param_name)) > + return false; > + len = strlen(param_name); > + if (len != target_len) > + return false; > + if (strncmp(param_name, name, target_len)) strcmp(param_name, name) is enough. len == target_len and both len and target_len is computed from strlen(...). > + return false; > + > + return true; > +} > + > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > struct bpf_reg_state *regs, > - bool ptr_to_mem_ok) > + bool ptr_to_mem_ok, > + struct bpf_kfunc_arg_meta *kfunc_meta) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > struct bpf_verifier_log *log = &env->log; > @@ -6225,6 +6251,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > if (btf_type_is_scalar(t)) { > + if (is_kfunc && kfunc_meta) { > + bool is_buf_size = false; > + > + /* check for any const scalar parameter of name "rdonly_buf_size" > + * or "rdwr_buf_size" > + */ > + if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > + "rdonly_buf_size")) { > + kfunc_meta->r0_rdonly = true; > + is_buf_size = true; > + } else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > + "rdwr_buf_size")) > + is_buf_size = true; > + > + if (is_buf_size) { > + if (kfunc_meta->r0_size) { > + bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc"); > + return -EINVAL; > + } > + > + kfunc_meta->r0_size = reg->var_off.value; Did we check 'reg' is a constant somewhere? > + } > + } > + > if (reg->type == SCALAR_VALUE) > continue; > bpf_log(log, "R%d is not a scalar\n", regno); > @@ -6246,6 +6296,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > if (ret < 0) > return ret; > > + /* kptr_get is only valid for kfunc */ > + if (kfunc_meta && reg->ref_obj_id) { > + /* check for any one ref_obj_id to keep track of memory */ > + if (kfunc_meta->ref_obj_id) > + kfunc_meta->multiple_ref_obj_id = true; > + kfunc_meta->ref_obj_id = reg->ref_obj_id; > + } > + > /* kptr_get is only true for kfunc */ > if (i == 0 && kptr_get) { > struct bpf_map_value_off_desc *off_desc; > @@ -6441,7 +6499,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > return -EINVAL; > > is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global); > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL); > > /* Compiler optimizations can remove arguments from static functions > * or mismatched type can be passed into a global function. [...]
On Tue, 12 Jul 2022 at 17:03, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > When a kfunc is not returning a pointer to a struct but to a plain type, > we can consider it is a valid allocated memory assuming that: > - one of the arguments is either called rdonly_buf_size or > rdwr_buf_size > - and this argument is a const from the caller point of view > > We can then use this parameter as the size of the allocated memory. > > The memory is either read-only or read-write based on the name > of the size parameter. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > changes in v6: > - code review from Kartikeya: > - remove comment change that had no reasons to be > - remove handling of PTR_TO_MEM with kfunc releases > - introduce struct bpf_kfunc_arg_meta > - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match > - reverted most of the changes in verifier.c > - make sure kfunc acquire is using a struct pointer, not just a plain > pointer > - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free > the allocated memory > > changes in v5: > - updated PTR_TO_MEM comment in btf.c to match upstream > - make it read-only or read-write based on the name of size > > new in v4 > --- > include/linux/bpf.h | 10 ++++++- > include/linux/btf.h | 12 ++++++++ > kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- > kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- > 4 files changed, 121 insertions(+), 17 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 2b21f2a3452f..5b8eadb6e7bc 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1916,12 +1916,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, > const char *func_name, > struct btf_func_model *m); > > +struct bpf_kfunc_arg_meta { > + u64 r0_size; > + bool r0_rdonly; > + int ref_obj_id; > + bool multiple_ref_obj_id; > +}; > + > struct bpf_reg_state; > int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *regs); > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > - struct bpf_reg_state *regs); > + struct bpf_reg_state *regs, > + struct bpf_kfunc_arg_meta *meta); > int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *reg); > int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog, > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 1bfed7fa0428..31da4273c2ec 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -420,4 +420,16 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt > } > #endif > > +static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t) > +{ > + /* t comes in already as a pointer */ > + t = btf_type_by_id(btf, t->type); > + > + /* allow const */ > + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) > + t = btf_type_by_id(btf, t->type); Any specific reason to not allow any other modifiers apart from const? volatile, restrict, typedef..? If not, just use btf_type_skip_modifiers instead. > + > + return btf_type_is_struct(t); > +} > + > #endif > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 4423045b8ff3..552d7bc05a0c 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6168,10 +6168,36 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, > return true; > } > > +static bool btf_is_kfunc_arg_mem_size(const struct btf *btf, > + const struct btf_param *arg, > + const struct bpf_reg_state *reg, > + const char *name) It would be nicer if we could reuse some code from is_kfunc_arg_mem_size, the only difference is matching suffix vs full string. But don't feel too strongly about it. > +{ > + int len, target_len = strlen(name); > + const struct btf_type *t; > + const char *param_name; > + > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > + return false; > + > + param_name = btf_name_by_offset(btf, arg->name_off); > + if (str_is_empty(param_name)) > + return false; > + len = strlen(param_name); > + if (len != target_len) > + return false; > + if (strncmp(param_name, name, target_len)) > + return false; > + > + return true; > +} > + > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > struct bpf_reg_state *regs, > - bool ptr_to_mem_ok) > + bool ptr_to_mem_ok, > + struct bpf_kfunc_arg_meta *kfunc_meta) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > struct bpf_verifier_log *log = &env->log; > @@ -6225,6 +6251,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > if (btf_type_is_scalar(t)) { > + if (is_kfunc && kfunc_meta) { > + bool is_buf_size = false; > + > + /* check for any const scalar parameter of name "rdonly_buf_size" > + * or "rdwr_buf_size" > + */ > + if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > + "rdonly_buf_size")) { > + kfunc_meta->r0_rdonly = true; > + is_buf_size = true; > + } else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > + "rdwr_buf_size")) > + is_buf_size = true; > + > + if (is_buf_size) { > + if (kfunc_meta->r0_size) { > + bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc"); > + return -EINVAL; > + } > + > + kfunc_meta->r0_size = reg->var_off.value; As Yonghong pointed out, you need to ensure the register holds a constant value, by using tnum_is_const(reg->var_off), and giving an error otherwise, because we need a constant size to be set for R0. > + } > + } > + > if (reg->type == SCALAR_VALUE) > continue; > bpf_log(log, "R%d is not a scalar\n", regno); > @@ -6246,6 +6296,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > if (ret < 0) > return ret; > > + /* kptr_get is only valid for kfunc */ Invalid comment > + if (kfunc_meta && reg->ref_obj_id) { > + /* check for any one ref_obj_id to keep track of memory */ > + if (kfunc_meta->ref_obj_id) > + kfunc_meta->multiple_ref_obj_id = true; Why not just return the error here itself? And then no need to keep the multiple_ref_obj_id member. When you return the error here, you can move a similar check in the if (reg->type == PTR_TO_BTF_ID) block to this place so that we don't do it twice. > + kfunc_meta->ref_obj_id = reg->ref_obj_id; > + } > + > /* kptr_get is only true for kfunc */ > if (i == 0 && kptr_get) { > struct bpf_map_value_off_desc *off_desc; > @@ -6441,7 +6499,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > return -EINVAL; > > is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global); > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL); > > /* Compiler optimizations can remove arguments from static functions > * or mismatched type can be passed into a global function. > @@ -6454,9 +6512,10 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > - struct bpf_reg_state *regs) > + struct bpf_reg_state *regs, > + struct bpf_kfunc_arg_meta *meta) > { > - return btf_check_func_arg_match(env, btf, func_id, regs, true); > + return btf_check_func_arg_match(env, btf, func_id, regs, true, meta); > } > > /* Convert BTF of a function into bpf_reg_state if possible > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 3adcc0d123af..77556132db15 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -7561,6 +7561,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > { > const struct btf_type *t, *func, *func_proto, *ptr_type; > struct bpf_reg_state *regs = cur_regs(env); > + struct bpf_kfunc_arg_meta meta = { 0 }; > const char *func_name, *ptr_type_name; > u32 i, nargs, func_id, ptr_type_id; > int err, insn_idx = *insn_idx_p; > @@ -7592,7 +7593,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > BTF_KFUNC_TYPE_ACQUIRE, func_id); > > /* Check the arguments */ > - err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs); > + err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs, &meta); > if (err < 0) > return err; > /* In case of release function, we get register number of refcounted > @@ -7613,7 +7614,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > /* Check return type */ > t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL); > > - if (acq && !btf_type_is_ptr(t)) { > + if (acq && !btf_type_is_struct_ptr(desc_btf, t)) { > verbose(env, "acquire kernel function does not return PTR_TO_BTF_ID\n"); > return -EINVAL; > } > @@ -7625,17 +7626,41 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > ptr_type = btf_type_skip_modifiers(desc_btf, t->type, > &ptr_type_id); > if (!btf_type_is_struct(ptr_type)) { > - ptr_type_name = btf_name_by_offset(desc_btf, > - ptr_type->name_off); > - verbose(env, "kernel function %s returns pointer type %s %s is not supported\n", > - func_name, btf_type_str(ptr_type), > - ptr_type_name); > - return -EINVAL; > + if (!meta.r0_size) { > + ptr_type_name = btf_name_by_offset(desc_btf, > + ptr_type->name_off); > + verbose(env, > + "kernel function %s returns pointer type %s %s is not supported\n", > + func_name, > + btf_type_str(ptr_type), > + ptr_type_name); > + return -EINVAL; > + } > + > + if (meta.multiple_ref_obj_id) { > + verbose(env, > + "kernel function %s has multiple memory tracked objects\n", > + func_name); > + return -EINVAL; > + } > + > + mark_reg_known_zero(env, regs, BPF_REG_0); > + regs[BPF_REG_0].type = PTR_TO_MEM; > + regs[BPF_REG_0].mem_size = meta.r0_size; > + > + if (meta.r0_rdonly) > + regs[BPF_REG_0].type |= MEM_RDONLY; > + > + /* Ensures we don't access the memory after a release_reference() */ > + if (meta.ref_obj_id) > + regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; > + } else { > + mark_reg_known_zero(env, regs, BPF_REG_0); > + regs[BPF_REG_0].btf = desc_btf; > + regs[BPF_REG_0].type = PTR_TO_BTF_ID; > + regs[BPF_REG_0].btf_id = ptr_type_id; > } > - mark_reg_known_zero(env, regs, BPF_REG_0); > - regs[BPF_REG_0].btf = desc_btf; > - regs[BPF_REG_0].type = PTR_TO_BTF_ID; > - regs[BPF_REG_0].btf_id = ptr_type_id; > + > if (btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog), > BTF_KFUNC_TYPE_RET_NULL, func_id)) { > regs[BPF_REG_0].type |= PTR_MAYBE_NULL; > -- > 2.36.1 >
On Sat, Jul 16, 2022 at 6:29 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 7/12/22 7:58 AM, Benjamin Tissoires wrote: > > When a kfunc is not returning a pointer to a struct but to a plain type, > > we can consider it is a valid allocated memory assuming that: > > - one of the arguments is either called rdonly_buf_size or > > rdwr_buf_size > > - and this argument is a const from the caller point of view > > > > We can then use this parameter as the size of the allocated memory. > > > > The memory is either read-only or read-write based on the name > > of the size parameter. > > If I understand correctly, this permits a kfunc like > int *kfunc(..., int rdonly_buf_size); > ... > int *p = kfunc(..., 20); > so the 'p' points to a memory buffer with size 20. Yes, exactly. > > This looks like a strange interface although probably there > is a valid reason for this as I didn't participated in > earlier discussions. Well, the point is I need to be able to access a memory region that was allocated dynamically. For drivers, the incoming data can not usually be bound to a static value, and so we can not have the data statically defined in the matching struct. So this allows defining a kfunc to return any memory properly allocated and owned by the device. > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > --- > > > > changes in v6: > > - code review from Kartikeya: > > - remove comment change that had no reasons to be > > - remove handling of PTR_TO_MEM with kfunc releases > > - introduce struct bpf_kfunc_arg_meta > > - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match > > - reverted most of the changes in verifier.c > > - make sure kfunc acquire is using a struct pointer, not just a plain > > pointer > > - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free > > the allocated memory > > > > changes in v5: > > - updated PTR_TO_MEM comment in btf.c to match upstream > > - make it read-only or read-write based on the name of size > > > > new in v4 > > --- > > include/linux/bpf.h | 10 ++++++- > > include/linux/btf.h | 12 ++++++++ > > kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- > > kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- > > 4 files changed, 121 insertions(+), 17 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 2b21f2a3452f..5b8eadb6e7bc 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1916,12 +1916,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, > > const char *func_name, > > struct btf_func_model *m); > > > > +struct bpf_kfunc_arg_meta { > > + u64 r0_size; > > + bool r0_rdonly; > > + int ref_obj_id; > > + bool multiple_ref_obj_id; > > +}; > > + > > struct bpf_reg_state; > > int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > > struct bpf_reg_state *regs); > > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > > const struct btf *btf, u32 func_id, > > - struct bpf_reg_state *regs); > > + struct bpf_reg_state *regs, > > + struct bpf_kfunc_arg_meta *meta); > > int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, > > struct bpf_reg_state *reg); > > int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog, > > diff --git a/include/linux/btf.h b/include/linux/btf.h > > index 1bfed7fa0428..31da4273c2ec 100644 > > --- a/include/linux/btf.h > > +++ b/include/linux/btf.h > > @@ -420,4 +420,16 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt > > } > > #endif > > > > +static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t) > > +{ > > + /* t comes in already as a pointer */ > > + t = btf_type_by_id(btf, t->type); > > + > > + /* allow const */ > > + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) > > + t = btf_type_by_id(btf, t->type); > > + > > + return btf_type_is_struct(t); > > +} > > + > > #endif > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 4423045b8ff3..552d7bc05a0c 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -6168,10 +6168,36 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, > > return true; > > } > > > > +static bool btf_is_kfunc_arg_mem_size(const struct btf *btf, > > + const struct btf_param *arg, > > + const struct bpf_reg_state *reg, > > + const char *name) > > +{ > > + int len, target_len = strlen(name); > > + const struct btf_type *t; > > + const char *param_name; > > + > > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > + return false; > > + > > + param_name = btf_name_by_offset(btf, arg->name_off); > > + if (str_is_empty(param_name)) > > + return false; > > + len = strlen(param_name); > > + if (len != target_len) > > + return false; > > + if (strncmp(param_name, name, target_len)) > > strcmp(param_name, name) is enough. len == target_len and both len and > target_len is computed from strlen(...). Ack, fixed locally > > > + return false; > > + > > + return true; > > +} > > + > > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > const struct btf *btf, u32 func_id, > > struct bpf_reg_state *regs, > > - bool ptr_to_mem_ok) > > + bool ptr_to_mem_ok, > > + struct bpf_kfunc_arg_meta *kfunc_meta) > > { > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > struct bpf_verifier_log *log = &env->log; > > @@ -6225,6 +6251,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > > if (btf_type_is_scalar(t)) { > > + if (is_kfunc && kfunc_meta) { > > + bool is_buf_size = false; > > + > > + /* check for any const scalar parameter of name "rdonly_buf_size" > > + * or "rdwr_buf_size" > > + */ > > + if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > > + "rdonly_buf_size")) { > > + kfunc_meta->r0_rdonly = true; > > + is_buf_size = true; > > + } else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > > + "rdwr_buf_size")) > > + is_buf_size = true; > > + > > + if (is_buf_size) { > > + if (kfunc_meta->r0_size) { > > + bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc"); > > + return -EINVAL; > > + } > > + > > + kfunc_meta->r0_size = reg->var_off.value; > > Did we check 'reg' is a constant somewhere? I used to check for it in the previous version, but I think it got dropped in this revision. Re-adding this thanks to Kumar's help :) Cheers, Benjamin > > > + } > > + } > > + > > if (reg->type == SCALAR_VALUE) > > continue; > > bpf_log(log, "R%d is not a scalar\n", regno); > > @@ -6246,6 +6296,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > if (ret < 0) > > return ret; > > > > + /* kptr_get is only valid for kfunc */ > > + if (kfunc_meta && reg->ref_obj_id) { > > + /* check for any one ref_obj_id to keep track of memory */ > > + if (kfunc_meta->ref_obj_id) > > + kfunc_meta->multiple_ref_obj_id = true; > > + kfunc_meta->ref_obj_id = reg->ref_obj_id; > > + } > > + > > /* kptr_get is only true for kfunc */ > > if (i == 0 && kptr_get) { > > struct bpf_map_value_off_desc *off_desc; > > @@ -6441,7 +6499,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > > return -EINVAL; > > > > is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > > - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global); > > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL); > > > > /* Compiler optimizations can remove arguments from static functions > > * or mismatched type can be passed into a global function. > [...] >
On Sat, Jul 16, 2022 at 10:33 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Tue, 12 Jul 2022 at 17:03, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > When a kfunc is not returning a pointer to a struct but to a plain type, > > we can consider it is a valid allocated memory assuming that: > > - one of the arguments is either called rdonly_buf_size or > > rdwr_buf_size > > - and this argument is a const from the caller point of view > > > > We can then use this parameter as the size of the allocated memory. > > > > The memory is either read-only or read-write based on the name > > of the size parameter. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > --- > > > > changes in v6: > > - code review from Kartikeya: > > - remove comment change that had no reasons to be > > - remove handling of PTR_TO_MEM with kfunc releases > > - introduce struct bpf_kfunc_arg_meta > > - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match > > - reverted most of the changes in verifier.c > > - make sure kfunc acquire is using a struct pointer, not just a plain > > pointer > > - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free > > the allocated memory > > > > changes in v5: > > - updated PTR_TO_MEM comment in btf.c to match upstream > > - make it read-only or read-write based on the name of size > > > > new in v4 > > --- > > include/linux/bpf.h | 10 ++++++- > > include/linux/btf.h | 12 ++++++++ > > kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- > > kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- > > 4 files changed, 121 insertions(+), 17 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 2b21f2a3452f..5b8eadb6e7bc 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1916,12 +1916,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, > > const char *func_name, > > struct btf_func_model *m); > > > > +struct bpf_kfunc_arg_meta { > > + u64 r0_size; > > + bool r0_rdonly; > > + int ref_obj_id; > > + bool multiple_ref_obj_id; > > +}; > > + > > struct bpf_reg_state; > > int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > > struct bpf_reg_state *regs); > > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > > const struct btf *btf, u32 func_id, > > - struct bpf_reg_state *regs); > > + struct bpf_reg_state *regs, > > + struct bpf_kfunc_arg_meta *meta); > > int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, > > struct bpf_reg_state *reg); > > int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog, > > diff --git a/include/linux/btf.h b/include/linux/btf.h > > index 1bfed7fa0428..31da4273c2ec 100644 > > --- a/include/linux/btf.h > > +++ b/include/linux/btf.h > > @@ -420,4 +420,16 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt > > } > > #endif > > > > +static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t) > > +{ > > + /* t comes in already as a pointer */ > > + t = btf_type_by_id(btf, t->type); > > + > > + /* allow const */ > > + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) > > + t = btf_type_by_id(btf, t->type); > > Any specific reason to not allow any other modifiers apart from const? > volatile, restrict, typedef..? > If not, just use btf_type_skip_modifiers instead. Good point. Fixed locally > > > + > > + return btf_type_is_struct(t); > > +} > > + > > #endif > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 4423045b8ff3..552d7bc05a0c 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -6168,10 +6168,36 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, > > return true; > > } > > > > +static bool btf_is_kfunc_arg_mem_size(const struct btf *btf, > > + const struct btf_param *arg, > > + const struct bpf_reg_state *reg, > > + const char *name) > > It would be nicer if we could reuse some code from > is_kfunc_arg_mem_size, the only difference is matching suffix vs full > string. But don't feel too strongly about it. I'll see what I can do. There is indeed a lot of code duplication, but I wasn't sure how I could express the differences while making it obvious. > > > +{ > > + int len, target_len = strlen(name); > > + const struct btf_type *t; > > + const char *param_name; > > + > > + t = btf_type_skip_modifiers(btf, arg->type, NULL); > > + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) > > + return false; > > + > > + param_name = btf_name_by_offset(btf, arg->name_off); > > + if (str_is_empty(param_name)) > > + return false; > > + len = strlen(param_name); > > + if (len != target_len) > > + return false; > > + if (strncmp(param_name, name, target_len)) > > + return false; > > + > > + return true; > > +} > > + > > static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > const struct btf *btf, u32 func_id, > > struct bpf_reg_state *regs, > > - bool ptr_to_mem_ok) > > + bool ptr_to_mem_ok, > > + struct bpf_kfunc_arg_meta *kfunc_meta) > > { > > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > struct bpf_verifier_log *log = &env->log; > > @@ -6225,6 +6251,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > t = btf_type_skip_modifiers(btf, args[i].type, NULL); > > if (btf_type_is_scalar(t)) { > > + if (is_kfunc && kfunc_meta) { > > + bool is_buf_size = false; > > + > > + /* check for any const scalar parameter of name "rdonly_buf_size" > > + * or "rdwr_buf_size" > > + */ > > + if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > > + "rdonly_buf_size")) { > > + kfunc_meta->r0_rdonly = true; > > + is_buf_size = true; > > + } else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, > > + "rdwr_buf_size")) > > + is_buf_size = true; > > + > > + if (is_buf_size) { > > + if (kfunc_meta->r0_size) { > > + bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc"); > > + return -EINVAL; > > + } > > + > > + kfunc_meta->r0_size = reg->var_off.value; > > As Yonghong pointed out, you need to ensure the register holds a > constant value, by using tnum_is_const(reg->var_off), and giving an > error otherwise, because we need a constant size to be set for R0. Thanks for the hint on how to solve this. It saved me from doing searches in the archives :) As mentioned to Yonghong, I had it in a previous version, but the refactoring dropped it :( > > > + } > > + } > > + > > if (reg->type == SCALAR_VALUE) > > continue; > > bpf_log(log, "R%d is not a scalar\n", regno); > > @@ -6246,6 +6296,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > if (ret < 0) > > return ret; > > > > + /* kptr_get is only valid for kfunc */ > > Invalid comment oops > > > + if (kfunc_meta && reg->ref_obj_id) { > > + /* check for any one ref_obj_id to keep track of memory */ > > + if (kfunc_meta->ref_obj_id) > > + kfunc_meta->multiple_ref_obj_id = true; > > Why not just return the error here itself? And then no need to keep > the multiple_ref_obj_id member. > When you return the error here, you can move a similar check in the if > (reg->type == PTR_TO_BTF_ID) block to this place so that we don't do > it twice. Good point. I extracted the block from (reg->type == PTR_TO_BTF_ID) here, and at the end did a small "if (kfunc_meta && ref_obj_id) kfunc_meta->ref_obj_id = ref_obj_id" This way this path is done only once for all cases. Cheers, Benjamin > > > + kfunc_meta->ref_obj_id = reg->ref_obj_id; > > + } > > + > > /* kptr_get is only true for kfunc */ > > if (i == 0 && kptr_get) { > > struct bpf_map_value_off_desc *off_desc; > > @@ -6441,7 +6499,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > > return -EINVAL; > > > > is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > > - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global); > > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL); > > > > /* Compiler optimizations can remove arguments from static functions > > * or mismatched type can be passed into a global function. > > @@ -6454,9 +6512,10 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > > > > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > > const struct btf *btf, u32 func_id, > > - struct bpf_reg_state *regs) > > + struct bpf_reg_state *regs, > > + struct bpf_kfunc_arg_meta *meta) > > { > > - return btf_check_func_arg_match(env, btf, func_id, regs, true); > > + return btf_check_func_arg_match(env, btf, func_id, regs, true, meta); > > } > > > > /* Convert BTF of a function into bpf_reg_state if possible > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 3adcc0d123af..77556132db15 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7561,6 +7561,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > { > > const struct btf_type *t, *func, *func_proto, *ptr_type; > > struct bpf_reg_state *regs = cur_regs(env); > > + struct bpf_kfunc_arg_meta meta = { 0 }; > > const char *func_name, *ptr_type_name; > > u32 i, nargs, func_id, ptr_type_id; > > int err, insn_idx = *insn_idx_p; > > @@ -7592,7 +7593,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > BTF_KFUNC_TYPE_ACQUIRE, func_id); > > > > /* Check the arguments */ > > - err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs); > > + err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs, &meta); > > if (err < 0) > > return err; > > /* In case of release function, we get register number of refcounted > > @@ -7613,7 +7614,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > /* Check return type */ > > t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL); > > > > - if (acq && !btf_type_is_ptr(t)) { > > + if (acq && !btf_type_is_struct_ptr(desc_btf, t)) { > > verbose(env, "acquire kernel function does not return PTR_TO_BTF_ID\n"); > > return -EINVAL; > > } > > @@ -7625,17 +7626,41 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > ptr_type = btf_type_skip_modifiers(desc_btf, t->type, > > &ptr_type_id); > > if (!btf_type_is_struct(ptr_type)) { > > - ptr_type_name = btf_name_by_offset(desc_btf, > > - ptr_type->name_off); > > - verbose(env, "kernel function %s returns pointer type %s %s is not supported\n", > > - func_name, btf_type_str(ptr_type), > > - ptr_type_name); > > - return -EINVAL; > > + if (!meta.r0_size) { > > + ptr_type_name = btf_name_by_offset(desc_btf, > > + ptr_type->name_off); > > + verbose(env, > > + "kernel function %s returns pointer type %s %s is not supported\n", > > + func_name, > > + btf_type_str(ptr_type), > > + ptr_type_name); > > + return -EINVAL; > > + } > > + > > + if (meta.multiple_ref_obj_id) { > > + verbose(env, > > + "kernel function %s has multiple memory tracked objects\n", > > + func_name); > > + return -EINVAL; > > + } > > + > > + mark_reg_known_zero(env, regs, BPF_REG_0); > > + regs[BPF_REG_0].type = PTR_TO_MEM; > > + regs[BPF_REG_0].mem_size = meta.r0_size; > > + > > + if (meta.r0_rdonly) > > + regs[BPF_REG_0].type |= MEM_RDONLY; > > + > > + /* Ensures we don't access the memory after a release_reference() */ > > + if (meta.ref_obj_id) > > + regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; > > + } else { > > + mark_reg_known_zero(env, regs, BPF_REG_0); > > + regs[BPF_REG_0].btf = desc_btf; > > + regs[BPF_REG_0].type = PTR_TO_BTF_ID; > > + regs[BPF_REG_0].btf_id = ptr_type_id; > > } > > - mark_reg_known_zero(env, regs, BPF_REG_0); > > - regs[BPF_REG_0].btf = desc_btf; > > - regs[BPF_REG_0].type = PTR_TO_BTF_ID; > > - regs[BPF_REG_0].btf_id = ptr_type_id; > > + > > if (btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog), > > BTF_KFUNC_TYPE_RET_NULL, func_id)) { > > regs[BPF_REG_0].type |= PTR_MAYBE_NULL; > > -- > > 2.36.1 > > >
On 7/18/22 7:36 AM, Benjamin Tissoires wrote: > On Sat, Jul 16, 2022 at 6:29 AM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 7/12/22 7:58 AM, Benjamin Tissoires wrote: >>> When a kfunc is not returning a pointer to a struct but to a plain type, >>> we can consider it is a valid allocated memory assuming that: >>> - one of the arguments is either called rdonly_buf_size or >>> rdwr_buf_size >>> - and this argument is a const from the caller point of view >>> >>> We can then use this parameter as the size of the allocated memory. >>> >>> The memory is either read-only or read-write based on the name >>> of the size parameter. >> >> If I understand correctly, this permits a kfunc like >> int *kfunc(..., int rdonly_buf_size); >> ... >> int *p = kfunc(..., 20); >> so the 'p' points to a memory buffer with size 20. > > Yes, exactly. > >> >> This looks like a strange interface although probably there >> is a valid reason for this as I didn't participated in >> earlier discussions. > > Well, the point is I need to be able to access a memory region that > was allocated dynamically. For drivers, the incoming data can not > usually be bound to a static value, and so we can not have the data > statically defined in the matching struct. > So this allows defining a kfunc to return any memory properly > allocated and owned by the device. Okay, thanks for explanation. > >> >>> >>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >>> >>> --- >>> >>> changes in v6: >>> - code review from Kartikeya: >>> - remove comment change that had no reasons to be >>> - remove handling of PTR_TO_MEM with kfunc releases >>> - introduce struct bpf_kfunc_arg_meta >>> - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match >>> - reverted most of the changes in verifier.c >>> - make sure kfunc acquire is using a struct pointer, not just a plain >>> pointer >>> - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free >>> the allocated memory >>> >>> changes in v5: >>> - updated PTR_TO_MEM comment in btf.c to match upstream >>> - make it read-only or read-write based on the name of size >>> >>> new in v4 >>> --- >>> include/linux/bpf.h | 10 ++++++- >>> include/linux/btf.h | 12 ++++++++ >>> kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- >>> kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- >>> 4 files changed, 121 insertions(+), 17 deletions(-) >>> [...]
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 2b21f2a3452f..5b8eadb6e7bc 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1916,12 +1916,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, const char *func_name, struct btf_func_model *m); +struct bpf_kfunc_arg_meta { + u64 r0_size; + bool r0_rdonly; + int ref_obj_id; + bool multiple_ref_obj_id; +}; + struct bpf_reg_state; int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, struct bpf_reg_state *regs); int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, - struct bpf_reg_state *regs); + struct bpf_reg_state *regs, + struct bpf_kfunc_arg_meta *meta); int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog, struct bpf_reg_state *reg); int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog, diff --git a/include/linux/btf.h b/include/linux/btf.h index 1bfed7fa0428..31da4273c2ec 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -420,4 +420,16 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt } #endif +static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t) +{ + /* t comes in already as a pointer */ + t = btf_type_by_id(btf, t->type); + + /* allow const */ + if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST) + t = btf_type_by_id(btf, t->type); + + return btf_type_is_struct(t); +} + #endif diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 4423045b8ff3..552d7bc05a0c 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6168,10 +6168,36 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf, return true; } +static bool btf_is_kfunc_arg_mem_size(const struct btf *btf, + const struct btf_param *arg, + const struct bpf_reg_state *reg, + const char *name) +{ + int len, target_len = strlen(name); + const struct btf_type *t; + const char *param_name; + + t = btf_type_skip_modifiers(btf, arg->type, NULL); + if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE) + return false; + + param_name = btf_name_by_offset(btf, arg->name_off); + if (str_is_empty(param_name)) + return false; + len = strlen(param_name); + if (len != target_len) + return false; + if (strncmp(param_name, name, target_len)) + return false; + + return true; +} + static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, - bool ptr_to_mem_ok) + bool ptr_to_mem_ok, + struct bpf_kfunc_arg_meta *kfunc_meta) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); struct bpf_verifier_log *log = &env->log; @@ -6225,6 +6251,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, t = btf_type_skip_modifiers(btf, args[i].type, NULL); if (btf_type_is_scalar(t)) { + if (is_kfunc && kfunc_meta) { + bool is_buf_size = false; + + /* check for any const scalar parameter of name "rdonly_buf_size" + * or "rdwr_buf_size" + */ + if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, + "rdonly_buf_size")) { + kfunc_meta->r0_rdonly = true; + is_buf_size = true; + } else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg, + "rdwr_buf_size")) + is_buf_size = true; + + if (is_buf_size) { + if (kfunc_meta->r0_size) { + bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc"); + return -EINVAL; + } + + kfunc_meta->r0_size = reg->var_off.value; + } + } + if (reg->type == SCALAR_VALUE) continue; bpf_log(log, "R%d is not a scalar\n", regno); @@ -6246,6 +6296,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, if (ret < 0) return ret; + /* kptr_get is only valid for kfunc */ + if (kfunc_meta && reg->ref_obj_id) { + /* check for any one ref_obj_id to keep track of memory */ + if (kfunc_meta->ref_obj_id) + kfunc_meta->multiple_ref_obj_id = true; + kfunc_meta->ref_obj_id = reg->ref_obj_id; + } + /* kptr_get is only true for kfunc */ if (i == 0 && kptr_get) { struct bpf_map_value_off_desc *off_desc; @@ -6441,7 +6499,7 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, return -EINVAL; is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global); + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL); /* Compiler optimizations can remove arguments from static functions * or mismatched type can be passed into a global function. @@ -6454,9 +6512,10 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, - struct bpf_reg_state *regs) + struct bpf_reg_state *regs, + struct bpf_kfunc_arg_meta *meta) { - return btf_check_func_arg_match(env, btf, func_id, regs, true); + return btf_check_func_arg_match(env, btf, func_id, regs, true, meta); } /* Convert BTF of a function into bpf_reg_state if possible diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 3adcc0d123af..77556132db15 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7561,6 +7561,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, { const struct btf_type *t, *func, *func_proto, *ptr_type; struct bpf_reg_state *regs = cur_regs(env); + struct bpf_kfunc_arg_meta meta = { 0 }; const char *func_name, *ptr_type_name; u32 i, nargs, func_id, ptr_type_id; int err, insn_idx = *insn_idx_p; @@ -7592,7 +7593,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, BTF_KFUNC_TYPE_ACQUIRE, func_id); /* Check the arguments */ - err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs); + err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs, &meta); if (err < 0) return err; /* In case of release function, we get register number of refcounted @@ -7613,7 +7614,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, /* Check return type */ t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL); - if (acq && !btf_type_is_ptr(t)) { + if (acq && !btf_type_is_struct_ptr(desc_btf, t)) { verbose(env, "acquire kernel function does not return PTR_TO_BTF_ID\n"); return -EINVAL; } @@ -7625,17 +7626,41 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, ptr_type = btf_type_skip_modifiers(desc_btf, t->type, &ptr_type_id); if (!btf_type_is_struct(ptr_type)) { - ptr_type_name = btf_name_by_offset(desc_btf, - ptr_type->name_off); - verbose(env, "kernel function %s returns pointer type %s %s is not supported\n", - func_name, btf_type_str(ptr_type), - ptr_type_name); - return -EINVAL; + if (!meta.r0_size) { + ptr_type_name = btf_name_by_offset(desc_btf, + ptr_type->name_off); + verbose(env, + "kernel function %s returns pointer type %s %s is not supported\n", + func_name, + btf_type_str(ptr_type), + ptr_type_name); + return -EINVAL; + } + + if (meta.multiple_ref_obj_id) { + verbose(env, + "kernel function %s has multiple memory tracked objects\n", + func_name); + return -EINVAL; + } + + mark_reg_known_zero(env, regs, BPF_REG_0); + regs[BPF_REG_0].type = PTR_TO_MEM; + regs[BPF_REG_0].mem_size = meta.r0_size; + + if (meta.r0_rdonly) + regs[BPF_REG_0].type |= MEM_RDONLY; + + /* Ensures we don't access the memory after a release_reference() */ + if (meta.ref_obj_id) + regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; + } else { + mark_reg_known_zero(env, regs, BPF_REG_0); + regs[BPF_REG_0].btf = desc_btf; + regs[BPF_REG_0].type = PTR_TO_BTF_ID; + regs[BPF_REG_0].btf_id = ptr_type_id; } - mark_reg_known_zero(env, regs, BPF_REG_0); - regs[BPF_REG_0].btf = desc_btf; - regs[BPF_REG_0].type = PTR_TO_BTF_ID; - regs[BPF_REG_0].btf_id = ptr_type_id; + if (btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog), BTF_KFUNC_TYPE_RET_NULL, func_id)) { regs[BPF_REG_0].type |= PTR_MAYBE_NULL;
When a kfunc is not returning a pointer to a struct but to a plain type, we can consider it is a valid allocated memory assuming that: - one of the arguments is either called rdonly_buf_size or rdwr_buf_size - and this argument is a const from the caller point of view We can then use this parameter as the size of the allocated memory. The memory is either read-only or read-write based on the name of the size parameter. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- changes in v6: - code review from Kartikeya: - remove comment change that had no reasons to be - remove handling of PTR_TO_MEM with kfunc releases - introduce struct bpf_kfunc_arg_meta - do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match - reverted most of the changes in verifier.c - make sure kfunc acquire is using a struct pointer, not just a plain pointer - also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free the allocated memory changes in v5: - updated PTR_TO_MEM comment in btf.c to match upstream - make it read-only or read-write based on the name of size new in v4 --- include/linux/bpf.h | 10 ++++++- include/linux/btf.h | 12 ++++++++ kernel/bpf/btf.c | 67 ++++++++++++++++++++++++++++++++++++++++--- kernel/bpf/verifier.c | 49 +++++++++++++++++++++++-------- 4 files changed, 121 insertions(+), 17 deletions(-)