On Tue, Jul 02, 2024 at 09:02:03PM -0700, Andrii Nakryiko wrote:
> @@ -2833,6 +2858,18 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry_ctx *ent
>
> fp = compat_ptr(ss_base + regs->bp);
> pagefault_disable();
> +
> +#ifdef CONFIG_UPROBES
> + /* see perf_callchain_user() below for why we do this */
> + if (current->utask) {
> + u32 ret_addr;
> +
> + if (is_uprobe_at_func_entry(regs, current->utask->auprobe) &&
> + !__get_user(ret_addr, (const u32 __user *)regs->sp))
Shouldn't the regs->sp value be checked with __access_ok() before
calling __get_user()?
Also, instead of littering functions with ifdefs it would be better to
abstract this out into a separate function which has an "always return
false" version for !CONFIG_UPROBES. Then the above could be simplified to
something like:
...
pagefault_disable();
if (is_uprobe_at_func_entry(regs, current) &&
__access_ok(regs->sp, 4) &&
!__get_user(ret_addr, (const u32 __user *)regs->sp))
perf_callchain_store(entry, ret_addr);
...
Also it's good practice to wait at least several days before posting new
versions to avoid spamming reviewers and to give them time to digest
what you've already sent.
--
Josh
