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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxx>

---

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.
[...]



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux