| Message ID | 20220824134055.1328882-1-benjamin.tissoires@redhat.com |
|---|---|
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
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. > > > + > > /* 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.
Hello: This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Wed, 24 Aug 2022 15:40:30 +0200 you wrote: > Hi, > > here comes the v9 of the HID-BPF series. > > Again, for a full explanation of HID-BPF, please refer to the last patch > in this series (23/23). > > [...] Here is the summary with links: - [bpf-next,v9,01/23] bpf/verifier: allow all functions to read user provided context (no matching commit) - [bpf-next,v9,02/23] bpf/verifier: do not clear meta in check_mem_size (no matching commit) - [bpf-next,v9,03/23] selftests/bpf: add test for accessing ctx from syscall program type (no matching commit) - [bpf-next,v9,04/23] bpf/verifier: allow kfunc to return an allocated mem (no matching commit) - [bpf-next,v9,05/23] selftests/bpf: Add tests for kfunc returning a memory pointer (no matching commit) - [bpf-next,v9,06/23] bpf: prepare for more bpf syscall to be used from kernel and user space. https://git.kernel.org/bpf/bpf-next/c/b88df6979682 - [bpf-next,v9,07/23] libbpf: add map_get_fd_by_id and map_delete_elem in light skeleton https://git.kernel.org/bpf/bpf-next/c/343949e10798 - [bpf-next,v9,08/23] HID: core: store the unique system identifier in hid_device (no matching commit) - [bpf-next,v9,09/23] HID: export hid_report_type to uapi (no matching commit) - [bpf-next,v9,10/23] HID: convert defines of HID class requests into a proper enum (no matching commit) - [bpf-next,v9,11/23] HID: Kconfig: split HID support and hid-core compilation (no matching commit) - [bpf-next,v9,12/23] HID: initial BPF implementation (no matching commit) - [bpf-next,v9,13/23] selftests/bpf: add tests for the HID-bpf initial implementation (no matching commit) - [bpf-next,v9,14/23] HID: bpf: allocate data memory for device_event BPF programs (no matching commit) - [bpf-next,v9,15/23] selftests/bpf/hid: add test to change the report size (no matching commit) - [bpf-next,v9,16/23] HID: bpf: introduce hid_hw_request() (no matching commit) - [bpf-next,v9,17/23] selftests/bpf: add tests for bpf_hid_hw_request (no matching commit) - [bpf-next,v9,18/23] HID: bpf: allow to change the report descriptor (no matching commit) - [bpf-next,v9,19/23] selftests/bpf: add report descriptor fixup tests (no matching commit) - [bpf-next,v9,20/23] selftests/bpf: Add a test for BPF_F_INSERT_HEAD (no matching commit) - [bpf-next,v9,21/23] samples/bpf: add new hid_mouse example (no matching commit) - [bpf-next,v9,22/23] HID: bpf: add Surface Dial example (no matching commit) - [bpf-next,v9,23/23] Documentation: add HID-BPF docs (no matching commit) You are awesome, thank you!
On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > We need to also export the kfunc set to the syscall program type, > and then add a couple of eBPF programs that are testing those calls. > > The first one checks for valid access, and the second one is OK > from a static analysis point of view but fails at run time because > we are trying to access outside of the allocated memory. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > no changes in v9 > > no changes in v8 > > changes in v7: > - add 1 more case to ensure we can read the entire sizeof(ctx) > - add a test case for when the context is NULL > > new in v6 > --- > net/bpf/test_run.c | 1 + > .../selftests/bpf/prog_tests/kfunc_call.c | 28 +++++++++++++++ > .../selftests/bpf/progs/kfunc_call_test.c | 36 +++++++++++++++++++ > 3 files changed, 65 insertions(+) > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 25d8ecf105aa..f16baf977a21 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -1634,6 +1634,7 @@ static int __init bpf_prog_test_run_init(void) > > ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set); > ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_prog_test_kfunc_set); > + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_prog_test_kfunc_set); > return ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc, > ARRAY_SIZE(bpf_prog_test_dtor_kfunc), > THIS_MODULE); > diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c > index eede7c304f86..1edad012fe01 100644 > --- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c > +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c > @@ -9,10 +9,22 @@ > > #include "cap_helpers.h" > > +struct syscall_test_args { > + __u8 data[16]; > + size_t size; > +}; > + > static void test_main(void) > { > struct kfunc_call_test_lskel *skel; > int prog_fd, err; > + struct syscall_test_args args = { > + .size = 10, > + }; > + DECLARE_LIBBPF_OPTS(bpf_test_run_opts, syscall_topts, > + .ctx_in = &args, > + .ctx_size_in = sizeof(args), > + ); > LIBBPF_OPTS(bpf_test_run_opts, topts, > .data_in = &pkt_v4, > .data_size_in = sizeof(pkt_v4), > @@ -38,6 +50,22 @@ static void test_main(void) > ASSERT_OK(err, "bpf_prog_test_run(test_ref_btf_id)"); > ASSERT_EQ(topts.retval, 0, "test_ref_btf_id-retval"); > > + prog_fd = skel->progs.kfunc_syscall_test.prog_fd; > + err = bpf_prog_test_run_opts(prog_fd, &syscall_topts); > + ASSERT_OK(err, "bpf_prog_test_run(syscall_test)"); > + > + prog_fd = skel->progs.kfunc_syscall_test_fail.prog_fd; > + err = bpf_prog_test_run_opts(prog_fd, &syscall_topts); > + ASSERT_ERR(err, "bpf_prog_test_run(syscall_test_fail)"); It would be better to assert on the verifier error string, to make sure we continue actually testing the error we care about and not something else. > + > + syscall_topts.ctx_in = NULL; > + syscall_topts.ctx_size_in = 0; > + > + prog_fd = skel->progs.kfunc_syscall_test_null.prog_fd; > + err = bpf_prog_test_run_opts(prog_fd, &syscall_topts); > + ASSERT_OK(err, "bpf_prog_test_run(syscall_test_null)"); > + ASSERT_EQ(syscall_topts.retval, 0, "syscall_test_null-retval"); > + > kfunc_call_test_lskel__destroy(skel); > } > > diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c > index 5aecbb9fdc68..da7ae0ef9100 100644 > --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c > +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c > @@ -92,4 +92,40 @@ int kfunc_call_test_pass(struct __sk_buff *skb) > return 0; > } > > +struct syscall_test_args { > + __u8 data[16]; > + size_t size; > +}; > + > +SEC("syscall") > +int kfunc_syscall_test(struct syscall_test_args *args) > +{ > + const int size = args->size; > + > + if (size > sizeof(args->data)) > + return -7; /* -E2BIG */ > + > + bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(args->data)); > + bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(*args)); > + bpf_kfunc_call_test_mem_len_pass1(&args->data, size); > + > + return 0; > +} > + > +SEC("syscall") > +int kfunc_syscall_test_null(struct syscall_test_args *args) > +{ > + bpf_kfunc_call_test_mem_len_pass1(args, 0); > + Where is it testing 'NULL'? It is testing zero_size_allowed. > + return 0; > +} > + > +SEC("syscall") > +int kfunc_syscall_test_fail(struct syscall_test_args *args) > +{ > + bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(*args) + 1); > + > + return 0; > +} > + > char _license[] SEC("license") = "GPL"; > -- > 2.36.1 >
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. >
On Tue, Aug 30, 2022 at 7:29 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > 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. PTR_TO_CTX in the verifier always means valid pointer. All code paths in the verifier assumes that it's not NULL. Pointer to skb, to xdp, to pt_regs, etc. The syscall prog type is little bit special, since it makes sense not to pass any argument to such prog. So ctx_size_in == 0 is enforced after the verification: if (ctx_size_in < prog->aux->max_ctx_offset || ctx_size_in > U16_MAX) return -EINVAL; The verifier should be able to proceed assuming ctx != NULL and remember max max_ctx_offset. If max_ctx_offset == 4 and ctx_size_in == 0 then it doesn't matter whether the actual 'ctx' pointer is NULL or points to a valid memory. So it's ok for the verifier to assume ctx != NULL everywhere. Back to the issue at hand. With this patch the line: bpf_kfunc_call_test_mem_len_pass1(args, sizeof(*args)); will be seen as access_size == sizeof(*args), right? So this part: + if (access_size == 0) + return zero_size_allowed ? 0 : -EACCES; will be skipped and the newly added check_mem_access() will call check_ctx_access() which will call syscall_prog_is_valid_access() and it will say that any off < U16_MAX is fine and will simply record max max_ctx_offset. The ctx_size_in < prog->aux->max_ctx_offset check is done later. So when you're saying: "call of the program is actually failing with -EINVAL" that's the check you're referring to? If so, everything works as expected. The verifier thinks that bpf_kfunc_call_test_mem_len_pass1() can read that many bytes from args, so it has to reject running the loaded prog in bpf_prog_test_run_syscall(). So what are you trying to achieve ? Make the verifier understand that ctx can be NULL ? If so that is a probably huge undertaking. Something else?
On Wed, Aug 31, 2022 at 6:37 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Aug 30, 2022 at 7:29 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > 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. > > PTR_TO_CTX in the verifier always means valid pointer. > All code paths in the verifier assumes that it's not NULL. > Pointer to skb, to xdp, to pt_regs, etc. > The syscall prog type is little bit special, since it > makes sense not to pass any argument to such prog. > So ctx_size_in == 0 is enforced after the verification: > if (ctx_size_in < prog->aux->max_ctx_offset || > ctx_size_in > U16_MAX) > return -EINVAL; > The verifier should be able to proceed assuming ctx != NULL > and remember max max_ctx_offset. > If max_ctx_offset == 4 and ctx_size_in == 0 then > it doesn't matter whether the actual 'ctx' pointer is NULL > or points to a valid memory. > So it's ok for the verifier to assume ctx != NULL everywhere. Ok, thanks for the detailed explanation. > > Back to the issue at hand. > With this patch the line: > bpf_kfunc_call_test_mem_len_pass1(args, sizeof(*args)); > will be seen as access_size == sizeof(*args), right? > So this part: > + if (access_size == 0) > + return zero_size_allowed ? 0 : -EACCES; > > will be skipped and > the newly added check_mem_access() will call check_ctx_access() > which will call syscall_prog_is_valid_access() and it will say > that any off < U16_MAX is fine and will simply > record max max_ctx_offset. > The ctx_size_in < prog->aux->max_ctx_offset check is done later. Yep, this is correct and this is working now, with a proper error (and no, this is not the error I am trying to fix, see below): eBPF prog: ``` 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; } ``` before this patch (1/23): * with ctx not NULL: libbpf: prog 'kfunc_syscall_test_null_fail': BPF program load failed: Invalid argument R1 type=ctx expected=fp arg#0 arg#1 memory, len pair leads to invalid memory access => this is not correct, we expect the program to be loaded (and it is expected, this is the bug that is fixed) * Same result with ctx being NULL from the caller With just the hunk in kernel/bpf/verifier.c (so without touching max_ctx_offset: * with ctx not NULL: program is loaded, and executed correctly * with ctx being NULL: program is now loaded, but execution returns -EINVAL, as expected So this case is fully solved by just the hunk in verifier.c With the full patch: same results, with or without ctx being set to NULL, so no side effects. > > So when you're saying: > "call of the program is actually failing with -EINVAL" > that's the check you're referring to? No. I am referring to the following eBPF program: ``` SEC("syscall") int kfunc_syscall_test_null(struct syscall_test_args *args) { return 0; } ``` (no calls, just the declaration of a program) This one is supposed to be loaded and properly run whatever the context is, right? However, without the hunk in the btf.c file (max_ctx_offset), we have the following (ctx is set to NULL by the userspace): verify_success:FAIL:kfunc_syscall_test_null unexpected error: -22 (errno 22) The reason is that the verifier is calling btf_check_subprog_arg_match() on programs too, and considers that ctx is not NULL, and bumps the max_ctx_offset value. > > If so, everything works as expected. Not exactly, we can not call a syscall program with a null context without this hunk. > The verifier thinks that bpf_kfunc_call_test_mem_len_pass1() > can read that many bytes from args, > so it has to reject running the loaded prog in bpf_prog_test_run_syscall(). Yes, that part works. I am focusing on the program declaration. > > So what are you trying to achieve ? See above :) > Make the verifier understand that ctx can be NULL ? Nope. I am fine with the way it is. But any eBPF (sub)prog is checked against btf_check_subprog_arg_match(), which in turns marks all of these calls accessing the entire ctx, even if the ctx is null when that case is valid. > If so that is a probably huge undertaking. > Something else? > Hopefully this is clearer now. Cheers, Benjamin
On Fri, Sep 2, 2022 at 5:50 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, 1 Sept 2022 at 18:48, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > [...] > > If the above is correct, then yes, it would make sense to me to have 2 > > distinct functions: one to check for the args types only (does the > > function definition in the problem matches BTF), and one to check for > > its use. > > Behind the scenes, btf_check_subprog_arg_match() calls > > btf_check_func_arg_match() which is the one function with entangled > > arguments type checking and actually assessing that the values > > provided are correct. > > > > I can try to split that btf_check_func_arg_match() into 2 distinct > > functions, though I am not sure I'll get it right. > > FYI, I've already split them into separate functions in my tree > because it had become super ugly at this point with all the new > support and I refactored it to add the linked list helpers support > using kfuncs (which requires some special handling for the args), so I > think you can just leave it with a "processing_call" check in for your > series for now. > great, thanks a lot. Actually, writing the patch today with the "processing_call" was really easy now that I have turned the problem in my head a lot yesterday. I am about to send v10 with the reviews addressed. Cheers, Benjamin
Hi, here comes the v9 of the HID-BPF series. Again, for a full explanation of HID-BPF, please refer to the last patch in this series (23/23). This version sees some minor improvements compared to v7 and v8, only focusing on the reviews I got. (v8 was a single patch update) - patch 1/24 in v7 was dropped as it is already fixed upstream - patch 1/23 in v9 is now capable of handling all functions, not just kfuncs (tested with the selftests only) - some minor nits from Greg's review - a rebase on top of the current bpf-next tree as the kfunc definition changed (for the better). Cheers, Benjamin Benjamin Tissoires (23): bpf/verifier: allow all functions to read user provided context bpf/verifier: do not clear meta in check_mem_size selftests/bpf: add test for accessing ctx from syscall program type bpf/verifier: allow kfunc to return an allocated mem selftests/bpf: Add tests for kfunc returning a memory pointer bpf: prepare for more bpf syscall to be used from kernel and user space. libbpf: add map_get_fd_by_id and map_delete_elem in light skeleton HID: core: store the unique system identifier in hid_device HID: export hid_report_type to uapi HID: convert defines of HID class requests into a proper enum HID: Kconfig: split HID support and hid-core compilation HID: initial BPF implementation selftests/bpf: add tests for the HID-bpf initial implementation HID: bpf: allocate data memory for device_event BPF programs selftests/bpf/hid: add test to change the report size HID: bpf: introduce hid_hw_request() selftests/bpf: add tests for bpf_hid_hw_request HID: bpf: allow to change the report descriptor selftests/bpf: add report descriptor fixup tests selftests/bpf: Add a test for BPF_F_INSERT_HEAD samples/bpf: HID: add new hid_mouse example samples/bpf: HID: add Surface Dial example Documentation: add HID-BPF docs Documentation/hid/hid-bpf.rst | 512 +++++++++ Documentation/hid/index.rst | 1 + drivers/Makefile | 2 +- drivers/hid/Kconfig | 20 +- drivers/hid/Makefile | 2 + drivers/hid/bpf/Kconfig | 17 + drivers/hid/bpf/Makefile | 11 + drivers/hid/bpf/entrypoints/Makefile | 93 ++ drivers/hid/bpf/entrypoints/README | 4 + drivers/hid/bpf/entrypoints/entrypoints.bpf.c | 66 ++ .../hid/bpf/entrypoints/entrypoints.lskel.h | 682 ++++++++++++ drivers/hid/bpf/hid_bpf_dispatch.c | 526 ++++++++++ drivers/hid/bpf/hid_bpf_dispatch.h | 28 + drivers/hid/bpf/hid_bpf_jmp_table.c | 577 ++++++++++ drivers/hid/hid-core.c | 49 +- include/linux/bpf.h | 9 +- include/linux/btf.h | 10 + include/linux/hid.h | 38 +- include/linux/hid_bpf.h | 148 +++ include/uapi/linux/hid.h | 26 +- include/uapi/linux/hid_bpf.h | 25 + kernel/bpf/btf.c | 109 +- kernel/bpf/syscall.c | 10 +- kernel/bpf/verifier.c | 64 +- net/bpf/test_run.c | 21 + samples/bpf/.gitignore | 2 + samples/bpf/Makefile | 27 + samples/bpf/hid_mouse.bpf.c | 134 +++ samples/bpf/hid_mouse.c | 161 +++ samples/bpf/hid_surface_dial.bpf.c | 161 +++ samples/bpf/hid_surface_dial.c | 232 ++++ tools/include/uapi/linux/hid.h | 62 ++ tools/include/uapi/linux/hid_bpf.h | 25 + tools/lib/bpf/skel_internal.h | 23 + tools/testing/selftests/bpf/Makefile | 5 +- tools/testing/selftests/bpf/config | 3 + tools/testing/selftests/bpf/prog_tests/hid.c | 990 ++++++++++++++++++ .../selftests/bpf/prog_tests/kfunc_call.c | 76 ++ tools/testing/selftests/bpf/progs/hid.c | 206 ++++ .../selftests/bpf/progs/kfunc_call_test.c | 125 +++ 40 files changed, 5198 insertions(+), 84 deletions(-) create mode 100644 Documentation/hid/hid-bpf.rst create mode 100644 drivers/hid/bpf/Kconfig create mode 100644 drivers/hid/bpf/Makefile create mode 100644 drivers/hid/bpf/entrypoints/Makefile create mode 100644 drivers/hid/bpf/entrypoints/README create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.bpf.c create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.lskel.h create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.c create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.h create mode 100644 drivers/hid/bpf/hid_bpf_jmp_table.c create mode 100644 include/linux/hid_bpf.h create mode 100644 include/uapi/linux/hid_bpf.h create mode 100644 samples/bpf/hid_mouse.bpf.c create mode 100644 samples/bpf/hid_mouse.c create mode 100644 samples/bpf/hid_surface_dial.bpf.c create mode 100644 samples/bpf/hid_surface_dial.c create mode 100644 tools/include/uapi/linux/hid.h create mode 100644 tools/include/uapi/linux/hid_bpf.h create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c create mode 100644 tools/testing/selftests/bpf/progs/hid.c