Re: [PATCH bpf-next v6 05/23] bpf/verifier: allow kfunc to return an allocated mem

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.

[...]