| Message ID | 20220721153625.1282007-3-benjamin.tissoires@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On Thu, 21 Jul 2022 at 17:36, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > When a kfunc was trying to access data from context in a syscall eBPF > program, the verifier was rejecting the call. > 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 and allow such situation to happen. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > LGTM, with just a couple more nits. Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > 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/verifier.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 7c1e056624f9..d5fe7e618c52 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -248,6 +248,7 @@ struct bpf_call_arg_meta { > struct bpf_map *map_ptr; > bool raw_mode; > bool pkt_access; > + bool is_kfunc; > u8 release_regno; > int regno; > int access_size; > @@ -5170,6 +5171,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, > struct bpf_call_arg_meta *meta) > { > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > u32 *max_access; > > switch (base_type(reg->type)) { > @@ -5223,6 +5225,24 @@ 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 of a kfunc called in a program of type SYSCALL, the context is > + * user supplied, so not computed statically. > + * Dynamically check it now > + */ > + if (prog_type == BPF_PROG_TYPE_SYSCALL && meta && meta->is_kfunc) { > + enum bpf_access_type atype = meta->raw_mode ? BPF_WRITE : BPF_READ; > + int offset = access_size - 1; > + > + /* Allow zero-byte read from NULL or PTR_TO_CTX */ This will not be handling the case for NULL, only for kfunc(ptr_to_ctx, 0) A null pointer has its reg->type as scalar, so it will be handled by the default case. > + if (access_size == 0) > + return zero_size_allowed ? 0 : -EINVAL; We should use -EACCES, just to be consistent. > + > + return check_mem_access(env, env->insn_idx, regno, offset, BPF_B, > + atype, -1, false); > + } > + > + fallthrough; > default: /* scalar_value or invalid ptr */ > /* Allow zero-byte read from NULL, regardless of pointer type */ > if (zero_size_allowed && access_size == 0 && > @@ -5335,6 +5355,7 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state > WARN_ON_ONCE(regno < BPF_REG_2 || regno > BPF_REG_5); > > memset(&meta, 0, sizeof(meta)); > + meta.is_kfunc = true; > > if (may_be_null) { > saved_reg = *mem_reg; > -- > 2.36.1 >
On Thu, Jul 21, 2022 at 10:15 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, 21 Jul 2022 at 17:36, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > When a kfunc was trying to access data from context in a syscall eBPF > > program, the verifier was rejecting the call. > > 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 and allow such situation to happen. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > --- > > > > LGTM, with just a couple more nits. > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Thanks a lot for the quick review. Sending the new version of just this patch now. Cheers, Benjamin > > > 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/verifier.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 7c1e056624f9..d5fe7e618c52 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -248,6 +248,7 @@ struct bpf_call_arg_meta { > > struct bpf_map *map_ptr; > > bool raw_mode; > > bool pkt_access; > > + bool is_kfunc; > > u8 release_regno; > > int regno; > > int access_size; > > @@ -5170,6 +5171,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, > > struct bpf_call_arg_meta *meta) > > { > > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > > + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > u32 *max_access; > > > > switch (base_type(reg->type)) { > > @@ -5223,6 +5225,24 @@ 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 of a kfunc called in a program of type SYSCALL, the context is > > + * user supplied, so not computed statically. > > + * Dynamically check it now > > + */ > > + if (prog_type == BPF_PROG_TYPE_SYSCALL && meta && meta->is_kfunc) { > > + enum bpf_access_type atype = meta->raw_mode ? BPF_WRITE : BPF_READ; > > + int offset = access_size - 1; > > + > > + /* Allow zero-byte read from NULL or PTR_TO_CTX */ > > This will not be handling the case for NULL, only for kfunc(ptr_to_ctx, 0) > A null pointer has its reg->type as scalar, so it will be handled by > the default case. > > > + if (access_size == 0) > > + return zero_size_allowed ? 0 : -EINVAL; > > We should use -EACCES, just to be consistent. > > > + > > + return check_mem_access(env, env->insn_idx, regno, offset, BPF_B, > > + atype, -1, false); > > + } > > + > > + fallthrough; > > default: /* scalar_value or invalid ptr */ > > /* Allow zero-byte read from NULL, regardless of pointer type */ > > if (zero_size_allowed && access_size == 0 && > > @@ -5335,6 +5355,7 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state > > WARN_ON_ONCE(regno < BPF_REG_2 || regno > BPF_REG_5); > > > > memset(&meta, 0, sizeof(meta)); > > + meta.is_kfunc = true; > > > > if (may_be_null) { > > saved_reg = *mem_reg; > > -- > > 2.36.1 > > >
On Fri, Jul 22, 2022 at 1:46 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > When a kfunc was trying to access data from context in a syscall eBPF > program, the verifier was rejecting the call. > 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 and allow such situation to happen. > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > 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/verifier.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 7c1e056624f9..c807c5d7085a 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -248,6 +248,7 @@ struct bpf_call_arg_meta { > struct bpf_map *map_ptr; > bool raw_mode; > bool pkt_access; > + bool is_kfunc; > u8 release_regno; > int regno; > int access_size; > @@ -5170,6 +5171,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, > struct bpf_call_arg_meta *meta) > { > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > u32 *max_access; > > switch (base_type(reg->type)) { > @@ -5223,6 +5225,24 @@ 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 of a kfunc called in a program of type SYSCALL, the context is > + * user supplied, so not computed statically. > + * Dynamically check it now > + */ > + if (prog_type == BPF_PROG_TYPE_SYSCALL && meta && meta->is_kfunc) { prog_type check looks a bit odd here. Can we generalize with if (!env->ops->convert_ctx_access In other words any program type that doesn't have ctx rewrites can use helpers to access ctx fields ? Also why kfunc only? It looks safe to allow normal helpers as well.
On Fri, Jul 22, 2022 at 6:16 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Jul 22, 2022 at 1:46 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > When a kfunc was trying to access data from context in a syscall eBPF > > program, the verifier was rejecting the call. > > 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 and allow such situation to happen. > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > --- > > > > 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/verifier.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 7c1e056624f9..c807c5d7085a 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -248,6 +248,7 @@ struct bpf_call_arg_meta { > > struct bpf_map *map_ptr; > > bool raw_mode; > > bool pkt_access; > > + bool is_kfunc; > > u8 release_regno; > > int regno; > > int access_size; > > @@ -5170,6 +5171,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, > > struct bpf_call_arg_meta *meta) > > { > > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > > + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > u32 *max_access; > > > > switch (base_type(reg->type)) { > > @@ -5223,6 +5225,24 @@ 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 of a kfunc called in a program of type SYSCALL, the context is > > + * user supplied, so not computed statically. > > + * Dynamically check it now > > + */ > > + if (prog_type == BPF_PROG_TYPE_SYSCALL && meta && meta->is_kfunc) { > > prog_type check looks a bit odd here. > Can we generalize with > if (!env->ops->convert_ctx_access Yep, seems to be working fine for my use case and the test cases I have in this series. > > In other words any program type that doesn't have ctx rewrites can > use helpers to access ctx fields ? > > Also why kfunc only? > It looks safe to allow normal helpers as well. Well, not sure what is happening here, but if I remove the check for kfunc, the test for PTR_TO_CTX == NULL and size == 0 gives me a -EINVAL. The original reason for kfunc only was because I wanted to scope the changes to something I can control, but now I am completely out of ideas on why the NULL test fails if it enters the if branch. Unfortunately I won't have a lot of time this week to tackle this (I am on holiday with my family), and next will be tough too (at home but doing renovations). I can send the fixup to remove the prog_type check as I just made sure it works with the selftests. But I won't be able to dig further why it fails without the kfunc check, because not enough time and concentration. Cheers, Benjamin
On 7/25/22 18:36, Benjamin Tissoires wrote: > On Fri, Jul 22, 2022 at 6:16 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> On Fri, Jul 22, 2022 at 1:46 AM Benjamin Tissoires >> <benjamin.tissoires@redhat.com> wrote: >>> >>> When a kfunc was trying to access data from context in a syscall eBPF >>> program, the verifier was rejecting the call. >>> 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 and allow such situation to happen. >>> >>> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> >>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >>> >>> --- >>> >>> 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/verifier.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index 7c1e056624f9..c807c5d7085a 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -248,6 +248,7 @@ struct bpf_call_arg_meta { >>> struct bpf_map *map_ptr; >>> bool raw_mode; >>> bool pkt_access; >>> + bool is_kfunc; >>> u8 release_regno; >>> int regno; >>> int access_size; >>> @@ -5170,6 +5171,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, >>> struct bpf_call_arg_meta *meta) >>> { >>> struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; >>> + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >>> u32 *max_access; >>> >>> switch (base_type(reg->type)) { >>> @@ -5223,6 +5225,24 @@ 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 of a kfunc called in a program of type SYSCALL, the context is >>> + * user supplied, so not computed statically. >>> + * Dynamically check it now >>> + */ >>> + if (prog_type == BPF_PROG_TYPE_SYSCALL && meta && meta->is_kfunc) { >> >> prog_type check looks a bit odd here. >> Can we generalize with >> if (!env->ops->convert_ctx_access > > Yep, seems to be working fine for my use case and the test cases I > have in this series. > >> >> In other words any program type that doesn't have ctx rewrites can >> use helpers to access ctx fields ? >> >> Also why kfunc only? >> It looks safe to allow normal helpers as well. > > Well, not sure what is happening here, but if I remove the check for > kfunc, the test for PTR_TO_CTX == NULL and size == 0 gives me a > -EINVAL. I finally managed to track down the issue. The reason was that if we now call check_mem_access for every function check, but also subprogs. And so we ensure that a subprog can access context. This is all fine, but that test now tags the subprog accessing the context, even if it is actually null and not accessing it in the code. So to restore the previous behavior, I am storing env->prog->aux->max_ctx_offset in btf_check_subprog_arg_match() and restore it after the call to check for the arguments. See the v9 for the detail in the code. Cheers, Benjamin > > The original reason for kfunc only was because I wanted to scope the > changes to something I can control, but now I am completely out of > ideas on why the NULL test fails if it enters the if branch. > > Unfortunately I won't have a lot of time this week to tackle this (I > am on holiday with my family), and next will be tough too (at home but > doing renovations). > > I can send the fixup to remove the prog_type check as I just made sure > it works with the selftests. But I won't be able to dig further why it > fails without the kfunc check, because not enough time and > concentration. > > Cheers, > Benjamin
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7c1e056624f9..d5fe7e618c52 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -248,6 +248,7 @@ struct bpf_call_arg_meta { struct bpf_map *map_ptr; bool raw_mode; bool pkt_access; + bool is_kfunc; u8 release_regno; int regno; int access_size; @@ -5170,6 +5171,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, struct bpf_call_arg_meta *meta) { struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); u32 *max_access; switch (base_type(reg->type)) { @@ -5223,6 +5225,24 @@ 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 of a kfunc called in a program of type SYSCALL, the context is + * user supplied, so not computed statically. + * Dynamically check it now + */ + if (prog_type == BPF_PROG_TYPE_SYSCALL && meta && meta->is_kfunc) { + enum bpf_access_type atype = meta->raw_mode ? BPF_WRITE : BPF_READ; + int offset = access_size - 1; + + /* Allow zero-byte read from NULL or PTR_TO_CTX */ + if (access_size == 0) + return zero_size_allowed ? 0 : -EINVAL; + + return check_mem_access(env, env->insn_idx, regno, offset, BPF_B, + atype, -1, false); + } + + fallthrough; default: /* scalar_value or invalid ptr */ /* Allow zero-byte read from NULL, regardless of pointer type */ if (zero_size_allowed && access_size == 0 && @@ -5335,6 +5355,7 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state WARN_ON_ONCE(regno < BPF_REG_2 || regno > BPF_REG_5); memset(&meta, 0, sizeof(meta)); + meta.is_kfunc = true; if (may_be_null) { saved_reg = *mem_reg;
When a kfunc was trying to access data from context in a syscall eBPF program, the verifier was rejecting the call. 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 and allow such situation to happen. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- 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/verifier.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)