| Message ID | 20220712145850.599666-3-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On 7/12/22 7:58 AM, Benjamin Tissoires 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> Acked-by: Yonghong Song <yhs@fb.com>
On Tue, 12 Jul 2022 at 17:02, 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> > > --- > > new in v6 > --- > kernel/bpf/verifier.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 328cfab3af60..f6af57a84247 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,19 @@ 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 access_t = meta->raw_mode ? BPF_WRITE : BPF_READ; small nit: _t suffix is used for types, so you could probably rename this. maybe atype? > + > + return check_mem_access(env, env->insn_idx, regno, access_size, BPF_B, > + access_t, -1, false); If I read the code correctly, this makes the max_ctx_offset of prog access_size + 1 (off + size_to_bytes(BPF_B)), which is 1 more than the actual size being accessed. This also messes up check_helper_mem_access when it allows NULL, 0 pair to pass (because check is against actual size + 1). We do allow passing NULL when size is 0 for kfuncs (see zero_size_allowed is true in check_mem_size_reg), so your hid_hw_request function is missing that NULL check for buf too. In the selftest that checks for failure in loading + bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(*args) + 1); so it will still fail with just sizeof(*args). Also please add coverage for this case in the next version. > + } > + > + 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 +5350,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 Sat, Jul 16, 2022 at 9:48 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Tue, 12 Jul 2022 at 17:02, 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> > > > > --- > > > > new in v6 > > --- > > kernel/bpf/verifier.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 328cfab3af60..f6af57a84247 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,19 @@ 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 access_t = meta->raw_mode ? BPF_WRITE : BPF_READ; > > small nit: _t suffix is used for types, so you could probably rename > this. maybe atype? Ack, fixed locally. > > > + > > + return check_mem_access(env, env->insn_idx, regno, access_size, BPF_B, > > + access_t, -1, false); > > If I read the code correctly, this makes the max_ctx_offset of prog > access_size + 1 (off + size_to_bytes(BPF_B)), which is 1 more than the > actual size being accessed. Oh, correct. I am mixing offset and access_size, which creates this :( > > This also messes up check_helper_mem_access when it allows NULL, 0 > pair to pass (because check is against actual size + 1). We do allow > passing NULL when size is 0 for kfuncs (see zero_size_allowed is true I am a little bit confused by how check_mem_size_reg() treats the case when reg->umin_value == 0. What does it mean to call check_helper_mem_access() with a 0 size if we have zero_size_allowed? Can I just have in the PTR_TO_CTX case: "if (access_size == 0) return zero_size_allowed ? 0 : -EINVAL;" or should I only allow the call if the ptr in the register is null? > in check_mem_size_reg), so your hid_hw_request function is missing > that NULL check for buf too. Actually, in hid_hw_request() we ensure buf__sz is greater than 1, so buf can not be null. But I agree it doesn't hurt to have that extra check to be sure (we are called from a syscall program, so not time sensitive). > > In the selftest that checks for failure in loading > + bpf_kfunc_call_test_mem_len_pass1(&args->data, sizeof(*args) + 1); > so it will still fail with just sizeof(*args). Good point. > > Also please add coverage for this case in the next version. I added both (NULL, 0) and (&args->data, sizeof(*args)) as passing tests locally. And thanks for the review! Cheers, Benjamin > > > + } > > + > > + 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 +5350,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 > > >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 328cfab3af60..f6af57a84247 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,19 @@ 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 access_t = meta->raw_mode ? BPF_WRITE : BPF_READ; + + return check_mem_access(env, env->insn_idx, regno, access_size, BPF_B, + access_t, -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 +5350,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> --- new in v6 --- kernel/bpf/verifier.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)