lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 16 Jul 2022 21:47:34 +0200 From: Kumar Kartikeya Dwivedi <memxor@...il.com> To: Benjamin Tissoires <benjamin.tissoires@...hat.com> Cc: Greg KH <gregkh@...uxfoundation.org>, Jiri Kosina <jikos@...nel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>, John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>, Dave Marchevsky <davemarchevsky@...com>, Joe Stringer <joe@...ium.io>, Jonathan Corbet <corbet@....net>, Tero Kristo <tero.kristo@...ux.intel.com>, linux-kernel@...r.kernel.org, linux-input@...r.kernel.org, netdev@...r.kernel.org, bpf@...r.kernel.org, linux-kselftest@...r.kernel.org, linux-doc@...r.kernel.org Subject: Re: [PATCH bpf-next v6 02/23] bpf/verifier: allow kfunc to read user provided context On Tue, 12 Jul 2022 at 17:02, Benjamin Tissoires <benjamin.tissoires@...hat.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@...hat.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 >
Powered by blists - more mailing lists