lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH bpf-next v9 01/23] bpf/verifier: allow all functions to read user provided context
    On Fri, Aug 26, 2022 at 3:51 AM Kumar Kartikeya Dwivedi
    <memxor@gmail.com> wrote:
    >
    > On Fri, 26 Aug 2022 at 03:42, Alexei Starovoitov
    > <alexei.starovoitov@gmail.com> wrote:
    > >
    > > On Wed, Aug 24, 2022 at 6:41 AM Benjamin Tissoires
    > > <benjamin.tissoires@redhat.com> wrote:
    > > >
    > > > 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 <benjamin.tissoires@redhat.com>
    > > >
    > > > ---
    > > >
    > > > 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.

    The test I have that is failing in patch 2/23 is the following, with
    args being set to NULL by userspace:

    SEC("syscall")
    int kfunc_syscall_test_null(struct syscall_test_args *args)
    {
    bpf_kfunc_call_test_mem_len_pass1(args, 0);

    return 0;
    }

    Basically:
    if userspace declares the following:
    DECLARE_LIBBPF_OPTS(bpf_test_run_opts, syscall_topts,
    .ctx_in = NULL,
    .ctx_size_in = 0,
    );

    The verifier is happy with the current released kernel:
    kfunc_syscall_test_fail() never dereferences the ctx pointer, it just
    passes it around to bpf_kfunc_call_test_mem_len_pass1(), which in turn
    is also happy because it says it is not accessing the data at all (0
    size memory parameter).

    In the current code, check_helper_mem_access() actually returns
    -EINVAL, but doesn't change max_ctx_offset (it's still at the value of
    0 here). The program is now marked as unreliable, but the verifier
    goes on.

    When adding this patch, if we declare a syscall eBPF (or any other
    function that doesn't have env->ops->convert_ctx_access), the previous
    "test" is failing because this ensures the syscall program has to have
    a valid ctx pointer.
    btf_check_func_arg_match() now calls check_mem_access() which
    basically validates the fact that the program can dereference the ctx.

    So now, without the max_ctx_offset store/restore, the verifier
    enforces that the provided ctx is not null.

    What I thought that would happen was that if we were to pass a NULL
    context from userspace, but the eBPF program dereferences it (or in
    that case have a subprog or a function call that dereferences it),
    then max_ctx_offset would still be set to the proper value because of
    that internal dereference, and so the verifier would reject with
    -EINVAL the call to the eBPF program.

    If I add another test that has the following ebpf prog (with ctx_in
    being set to NULL by the userspace):

    SEC("syscall")
    int kfunc_syscall_test_null_fail(struct syscall_test_args *args)
    {
    bpf_kfunc_call_test_mem_len_pass1(args, sizeof(*args));

    return 0;
    }

    Then the call of the program is actually failing with -EINVAL, even
    with this patch.

    But again, if setting from userspace a ctx of NULL with a 0 size is
    not considered as valid, then we can just drop that hunk and add a
    test to enforce it.

    Cheers,
    Benjamin

    >
    > >
    > > > +
    > > > /* 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.
    >

    \
     
     \ /
      Last update: 2022-08-30 16:30    [W:4.533 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site