Thread (44 messages) 44 messages, 4 authors, 2022-09-02

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

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date: 2022-08-26 01:25:21
Also in: bpf, linux-doc, linux-input, linux-kselftest, lkml

On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires
[off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted>

---

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.
quoted hunk ↗ jump to hunk
+                               }
+                       }
+
                        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.
[...]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help