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: Tue, 17 May 2022 00:01:32 -0700 From: Yonghong Song <yhs@...com> To: Eugene Syromiatnikov <esyr@...hat.com>, Jiri Olsa <jolsa@...nel.org>, Masami Hiramatsu <mhiramat@...nel.org>, Steven Rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...hat.com>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net> Cc: Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>, John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, netdev@...r.kernel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org Subject: Re: [PATCH bpf v2 1/4] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach On 5/16/22 4:04 PM, Eugene Syromiatnikov wrote: > Check that size would not overflow before calculation (and return > -EOVERFLOW if it will), to prevent potential out-of-bounds write > with the following copy_from_user. Add the same check > to kprobe_multi_resolve_syms in case it will be called from elsewhere > in the future. > > Fixes: 0dcac272540613d4 ("bpf: Add multi kprobe link") > Signed-off-by: Eugene Syromiatnikov <esyr@...hat.com> > --- > kernel/trace/bpf_trace.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index d8553f4..f1d4e68 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2352,12 +2352,15 @@ static int > kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt, > unsigned long *addrs) > { > - unsigned long addr, size; > + unsigned long addr, sym_size; > + u32 size; > const char __user **syms; > int err = -ENOMEM; > unsigned int i; > char *func; > > + if (check_mul_overflow(cnt, (u32)sizeof(*syms), &size)) > + return -EOVERFLOW; > size = cnt * sizeof(*syms); The statement 'size = cnt * sizeof(*syms)' can be removed right? Here, the 'size' will be computed properly with check_mul_overflow() if 0 is returned. > syms = kvzalloc(size, GFP_KERNEL); > if (!syms) > @@ -2382,9 +2385,9 @@ kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt, > addr = kallsyms_lookup_name(func); > if (!addr) > goto error; > - if (!kallsyms_lookup_size_offset(addr, &size, NULL)) > + if (!kallsyms_lookup_size_offset(addr, &sym_size, NULL)) > goto error; > - addr = ftrace_location_range(addr, addr + size - 1); > + addr = ftrace_location_range(addr, addr + sym_size - 1); > if (!addr) > goto error; > addrs[i] = addr; > @@ -2429,6 +2432,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > if (!cnt) > return -EINVAL; > > + if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size)) > + return -EOVERFLOW; > size = cnt * sizeof(*addrs); Same here, 'size = ...' is not needed. > addrs = kvmalloc(size, GFP_KERNEL); > if (!addrs)
Powered by blists - more mailing lists