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@redhat.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)