On Fri, 26 Aug 2022 at 03:42, Alexei Starovoitov
[off-list ref] wrote:

On Wed, Aug 24, 2022 at 6:41 AM Benjamin Tissoires
[off-list ref] wrote:

quoted

When a function was trying to access data from context in a syscall eBPF
program, the verifier was rejecting the call unless it was accessing the
first element.
This is because the syscall context is not known at compile time, and
so we need to check this when actually accessing it.

Check for the valid memory access if there is no convert_ctx callback,
and allow such situation to happen.

There is a slight hiccup with subprogs. btf_check_subprog_arg_match()
will check that the types are matching, which is a good thing, but to
have an accurate result, it hides the fact that the context register may
be null. This makes env->prog->aux->max_ctx_offset being set to the size
of the context, which is incompatible with a NULL context.

Solve that last problem by storing max_ctx_offset before the type check
and restoring it after.

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

---

changes in v9:
- rewrote the commit title and description
- made it so all functions can make use of context even if there is
  no convert_ctx
- remove the is_kfunc field in bpf_call_arg_meta

changes in v8:
- fixup comment
- return -EACCESS instead of -EINVAL for consistency

changes in v7:
- renamed access_t into atype
- allow zero-byte read
- check_mem_access() to the correct offset/size

new in v6
---
 kernel/bpf/btf.c      | 11 ++++++++++-
 kernel/bpf/verifier.c | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 903719b89238..386300f52b23 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -6443,8 +6443,8 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
 {
        struct bpf_prog *prog = env->prog;
        struct btf *btf = prog->aux->btf;
+       u32 btf_id, max_ctx_offset;
        bool is_global;
-       u32 btf_id;
        int err;

        if (!prog->aux->func_info)

@@ -6457,9 +6457,18 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog,
        if (prog->aux->func_info_aux[subprog].unreliable)
                return -EINVAL;

+       /* subprogs arguments are not actually accessing the data, we need
+        * to check for the types if they match.
+        * Store the max_ctx_offset and restore it after btf_check_func_arg_match()
+        * given that this function will have a side effect of changing it.
+        */
+       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);

+       env->prog->aux->max_ctx_offset = max_ctx_offset;

I don't understand this.
If we pass a ctx into a helper and it's going to
access [0..N] bytes from it why do we need to hide it?
max_ctx_offset will be used later raw_tp, tp, syscall progs
to determine whether it's ok to load them.
By hiding the actual size of access somebody can construct
a prog that reads out of bounds.
How is this related to NULL-ness property?

Same question, was just typing exactly the same thing.

quoted

+
        /* Compiler optimizations can remove arguments from static functions
         * or mismatched type can be passed into a global function.
         * In such cases mark the function as unreliable from BTF point of view.

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2c1f8069f7b7..d694f43ab911 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -5229,6 +5229,25 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
                                env,
                                regno, reg->off, access_size,
                                zero_size_allowed, ACCESS_HELPER, meta);
+       case PTR_TO_CTX:
+               /* in case the function doesn't know how to access the context,
+                * (because we are in a program of type SYSCALL for example), we
+                * can not statically check its size.
+                * Dynamically check it now.
+                */
+               if (!env->ops->convert_ctx_access) {
+                       enum bpf_access_type atype = meta && meta->raw_mode ? BPF_WRITE : BPF_READ;
+                       int offset = access_size - 1;
+
+                       /* Allow zero-byte read from PTR_TO_CTX */
+                       if (access_size == 0)
+                               return zero_size_allowed ? 0 : -EACCES;
+
+                       return check_mem_access(env, env->insn_idx, regno, offset, BPF_B,
+                                               atype, -1, false);
+               }

This part looks good alone. Without max_ctx_offset save/restore.

+1, save/restore would be incorrect.