diff mbox series

[bpf-next,v9,04/23] bpf/verifier: allow kfunc to return an allocated mem

Message ID 20220824134055.1328882-5-benjamin.tissoires@redhat.com
State New
Headers show
Series Introduce eBPF support for HID devices | expand

Commit Message

Benjamin Tissoires Aug. 24, 2022, 1:40 p.m. UTC 
For drivers (outside of network), the incoming data is not statically
defined in a struct. Most of the time the data buffer is kzalloc-ed
and thus we can not rely on eBPF and BTF to explore the data.

This commit allows to return an arbitrary memory, previously allocated by
the driver.
An interesting extra point is that the kfunc can mark the exported
memory region as read only or read/write.

So, 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.

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

---

changes in v9:
- updated to match upstream (replaced kfunc_flag by a field in
  kfunc_meta)

no changes in v8

changes in v7:
- ensures btf_type_is_struct_ptr() checks for a ptr first
  (squashed from next commit)
- remove multiple_ref_obj_id need
- use btf_type_skip_modifiers instead of manually doing it in
  btf_type_is_struct_ptr()
- s/strncmp/strcmp/ in btf_is_kfunc_arg_mem_size()
- check for tnum_is_const when retrieving the size value
- have only one check for "Ensure only one argument is referenced
  PTR_TO_BTF_ID"
- add some more context to the commit message

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

change btf.h

fix allow kfunc to return an allocated mem
---
 include/linux/bpf.h   |  9 +++-
 include/linux/btf.h   | 10 +++++
 kernel/bpf/btf.c      | 98 ++++++++++++++++++++++++++++++++++---------
 kernel/bpf/verifier.c | 43 +++++++++++++------
 4 files changed, 128 insertions(+), 32 deletions(-)

Comments

Kumar Kartikeya Dwivedi Aug. 31, 2022, 11:06 a.m. UTC | #1 
On Wed, 31 Aug 2022 at 07:50, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Fri, Aug 26, 2022 at 3:25 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > For drivers (outside of network), the incoming data is not statically
> > > defined in a struct. Most of the time the data buffer is kzalloc-ed
> > > and thus we can not rely on eBPF and BTF to explore the data.
> > >
> > > This commit allows to return an arbitrary memory, previously allocated by
> > > the driver.
> > > An interesting extra point is that the kfunc can mark the exported
> > > memory region as read only or read/write.
> > >
> > > So, 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.
> > >
> > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > ---
> > >
> > > changes in v9:
> > > - updated to match upstream (replaced kfunc_flag by a field in
> > >   kfunc_meta)
> > >
> > > no changes in v8
> > >
> > > changes in v7:
> > > - ensures btf_type_is_struct_ptr() checks for a ptr first
> > >   (squashed from next commit)
> > > - remove multiple_ref_obj_id need
> > > - use btf_type_skip_modifiers instead of manually doing it in
> > >   btf_type_is_struct_ptr()
> > > - s/strncmp/strcmp/ in btf_is_kfunc_arg_mem_size()
> > > - check for tnum_is_const when retrieving the size value
> > > - have only one check for "Ensure only one argument is referenced
> > >   PTR_TO_BTF_ID"
> > > - add some more context to the commit message
> > >
> > > 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
> > >
> > > change btf.h
> > >
> > > fix allow kfunc to return an allocated mem
> > > ---
> > >  include/linux/bpf.h   |  9 +++-
> > >  include/linux/btf.h   | 10 +++++
> > >  kernel/bpf/btf.c      | 98 ++++++++++++++++++++++++++++++++++---------
> > >  kernel/bpf/verifier.c | 43 +++++++++++++------
> > >  4 files changed, 128 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 39bd36359c1e..90dd218e0199 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -1932,13 +1932,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
> > >                            const char *func_name,
> > >                            struct btf_func_model *m);
> > > [...]
> > > +
> > >  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,
> > > -                                   u32 kfunc_flags)
> > > +                                   struct bpf_kfunc_arg_meta *kfunc_meta)
> > >  {
> > >         enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > >         bool rel = false, kptr_get = false, trusted_arg = false;
> > > @@ -6207,12 +6232,12 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >                 return -EINVAL;
> > >         }
> > >
> > > -       if (is_kfunc) {
> > > +       if (is_kfunc && kfunc_meta) {
> > >                 /* Only kfunc can be release func */
> > > -               rel = kfunc_flags & KF_RELEASE;
> > > -               kptr_get = kfunc_flags & KF_KPTR_GET;
> > > -               trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > -               sleepable = kfunc_flags & KF_SLEEPABLE;
> > > +               rel = kfunc_meta->flags & KF_RELEASE;
> > > +               kptr_get = kfunc_meta->flags & KF_KPTR_GET;
> > > +               trusted_arg = kfunc_meta->flags & KF_TRUSTED_ARGS;
> > > +               sleepable = kfunc_meta->flags & KF_SLEEPABLE;
> > >         }
> > >
> > >         /* check that BTF function arguments match actual types that the
> > > @@ -6225,6 +6250,35 @@ 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;
> > > +                                       }
> > > +
> > > +                                       if (!tnum_is_const(reg->var_off)) {
> > > +                                               bpf_log(log, "R%d is not a const\n", regno);
> > > +                                               return -EINVAL;
> > > +                                       }
> > > +
> > > +                                       kfunc_meta->r0_size = reg->var_off.value;
> >
> > Sorry for not pointing it out before, but you will need a call to
> > mark_chain_precision here after this, since the value of the scalar is
> > being used to decide the size of the returned pointer.
>
> No worries.
>
> I do however have a couple of questions (I have strictly no idea what
> mark_chain_precision does):

See this patch for some background:
https://lore.kernel.org/bpf/20220823185300.406-2-memxor@gmail.com
Same case here, it is setting the size of r0 PTR_TO_MEM.

> - which register number should I call mark_chain_precision() as
> parameter? r0 or regno (the one with the constant)?

Yes, regno, i.e. the one with the constant.

> - mark_chain_precision() is declared static in verifier.c. Should I
> export it so btf.c can have access to it, or can I delay the call to
> mark_chain_precision() in verifier.c when I set
> regs[BPF_REG_0].mem_size?
>

Yes, but then you have to remember the regno you have to call it for.
So it might be easier to just make it non-static and call in btf.c.

>
> >
> > > +                               }
> > > +                       }
> > > +
> > >                         if (reg->type == SCALAR_VALUE)
> > >                                 continue;
> > >                         bpf_log(log, "R%d is not a scalar\n", regno);
> > > @@ -6255,6 +6309,19 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >                 if (ret < 0)
> > >                         return ret;
> > >
> > > +               if (is_kfunc && reg->type == PTR_TO_BTF_ID) {
> >
> > I think you can drop this extra check 'reg->type == PTR_TO_BTF_ID),
> > this condition of only one ref_obj_id should hold regardless of the
> > type.
>
> Ack.
>
> Cheers,
> Benjamin
>
> >
> > > [...]
> >
>
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 39bd36359c1e..90dd218e0199 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1932,13 +1932,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;
+	u32 flags;
+};
+
 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,
-			      u32 kfunc_flags);
+			      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 ad93c2d9cc1c..1fcc833a8690 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -441,4 +441,14 @@  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)
+{
+	if (!btf_type_is_ptr(t))
+		return false;
+
+	t = btf_type_skip_modifiers(btf, t->type, NULL);
+
+	return btf_type_is_struct(t);
+}
+
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 386300f52b23..c0057ad1088f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6166,11 +6166,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 (strcmp(param_name, name))
+		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,
-				    u32 kfunc_flags)
+				    struct bpf_kfunc_arg_meta *kfunc_meta)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
 	bool rel = false, kptr_get = false, trusted_arg = false;
@@ -6207,12 +6232,12 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
-	if (is_kfunc) {
+	if (is_kfunc && kfunc_meta) {
 		/* Only kfunc can be release func */
-		rel = kfunc_flags & KF_RELEASE;
-		kptr_get = kfunc_flags & KF_KPTR_GET;
-		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
-		sleepable = kfunc_flags & KF_SLEEPABLE;
+		rel = kfunc_meta->flags & KF_RELEASE;
+		kptr_get = kfunc_meta->flags & KF_KPTR_GET;
+		trusted_arg = kfunc_meta->flags & KF_TRUSTED_ARGS;
+		sleepable = kfunc_meta->flags & KF_SLEEPABLE;
 	}
 
 	/* check that BTF function arguments match actual types that the
@@ -6225,6 +6250,35 @@  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;
+					}
+
+					if (!tnum_is_const(reg->var_off)) {
+						bpf_log(log, "R%d is not a const\n", regno);
+						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);
@@ -6255,6 +6309,19 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		if (ret < 0)
 			return ret;
 
+		if (is_kfunc && reg->type == PTR_TO_BTF_ID) {
+			/* Ensure only one argument is referenced PTR_TO_BTF_ID */
+			if (reg->ref_obj_id) {
+				if (ref_obj_id) {
+					bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
+						regno, reg->ref_obj_id, ref_obj_id);
+					return -EFAULT;
+				}
+				ref_regno = regno;
+				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;
@@ -6327,16 +6394,6 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			if (reg->type == PTR_TO_BTF_ID) {
 				reg_btf = reg->btf;
 				reg_ref_id = reg->btf_id;
-				/* Ensure only one argument is referenced PTR_TO_BTF_ID */
-				if (reg->ref_obj_id) {
-					if (ref_obj_id) {
-						bpf_log(log, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
-							regno, reg->ref_obj_id, ref_obj_id);
-						return -EFAULT;
-					}
-					ref_regno = regno;
-					ref_obj_id = reg->ref_obj_id;
-				}
 			} else {
 				reg_btf = btf_vmlinux;
 				reg_ref_id = *reg2btf_ids[base_type(reg->type)];
@@ -6427,6 +6484,9 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
+	if (kfunc_meta && ref_obj_id)
+		kfunc_meta->ref_obj_id = ref_obj_id;
+
 	/* returns argument register number > 0 in case of reference release kfunc */
 	return rel ? ref_regno : 0;
 }
@@ -6465,7 +6525,7 @@  int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 	max_ctx_offset = env->prog->aux->max_ctx_offset;
 
 	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, 0);
+	err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, NULL);
 
 	env->prog->aux->max_ctx_offset = max_ctx_offset;
 
@@ -6481,9 +6541,9 @@  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,
-			      u32 kfunc_flags)
+			      struct bpf_kfunc_arg_meta *meta)
 {
-	return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags);
+	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 13190487fb12..cd50850e139d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7576,6 +7576,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;
@@ -7610,8 +7611,10 @@  static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 
 	acq = *kfunc_flags & KF_ACQUIRE;
 
+	meta.flags = *kfunc_flags;
+
 	/* Check the arguments */
-	err = btf_check_kfunc_arg_match(env, desc_btf, func_id, regs, *kfunc_flags);
+	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
@@ -7632,7 +7635,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;
 	}
@@ -7644,17 +7647,33 @@  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;
+			}
+
+			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 (*kfunc_flags & KF_RET_NULL) {
 			regs[BPF_REG_0].type |= PTR_MAYBE_NULL;
 			/* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */