Re: [PATCH bpf] bpf: Add missing access_ok call to copy_user_syms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `access_ok()` checks for user pointers `uoffsets`, `uref_ctr_offsets`, and `ucookies` before using `__get_user()` in `bpf_uprobe_multi_link_attach`.
--

commit cd20e04dceb618df71198c96a9e801e033dcb0a0
Author: Jiri Olsa <jolsa@kernel.org>
bpf: Add missing access_ok call to copy_user_syms

The patch adds a missing access_ok() check for the user pointer array usyms
in copy_user_syms() before accessing it via __get_user(). It also cleans
up the error path by returning -ENOMEM directly upon memory allocation failure
instead of using a goto.

Link: https://lore.kernel.org/bpf/20260611115503.AC16D1F00893@smtp.kernel.org/

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a02bd258677ee..c274109d020f3 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2331,9 +2331,12 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>  	int err = -ENOMEM;
>  	unsigned int i;
>  
> +	if (!access_ok(usyms, cnt * sizeof(*usyms)))
> +		return -EFAULT;
> +
>  	syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL);
>  	if (!syms)
> -		goto error;
> +		return -ENOMEM;
>  
>  	buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL);
>  	if (!buf)
> @@ -2358,10 +2361,8 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32
>  	return 0;
>  
>  error:
> -	if (err) {
> -		kvfree(syms);
> -		kvfree(buf);
> -	}
> +	kvfree(syms);
> +	kvfree(buf);
>  	return err;
>  }

[Severity: High]
This is a pre-existing issue, but while reviewing this patch I noticed a
similar pattern in bpf_uprobe_multi_link_attach() within the same file.

kernel/trace/bpf_trace.c:bpf_uprobe_multi_link_attach() {
    ...
	for (i = 0; i < cnt; i++) {
		if (__get_user(uprobes[i].offset, uoffsets + i)) {
			err = -EFAULT;
			goto error_free;
		}
		if (uprobes[i].offset < 0) {
			err = -EINVAL;
			goto error_free;
		}
		if (uref_ctr_offsets && __get_user(uprobes[i].ref_ctr_offset, uref_ctr_offsets + i)) {
			err = -EFAULT;
			goto error_free;
		}
		if (ucookies && __get_user(uprobes[i].cookie, ucookies + i)) {
			err = -EFAULT;
			goto error_free;
		}
    ...
}

Are the user-provided array pointers uoffsets, uref_ctr_offsets, and ucookies
missing overarching access_ok() validations before being accessed in this loop?

Since __get_user() explicitly bypasses the access_ok() validation performed by
get_user(), reading these pointers without a prior check could potentially allow
an attacker to pass arbitrary kernel memory addresses and trigger unauthorized
reads or crashes.

Does this function need the same fix as copy_user_syms()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616083056.405652-1-jolsa@kernel.org?part=1