linux-kernel - Re: [PATCH v3] perf,x86: avoid missing caller address in stack traces captured in uprobe

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240703223927.zby4glzbngjqxemd@treble>
Date: Wed, 3 Jul 2024 15:39:27 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Andrii Nakryiko <andrii@...nel.org>
Cc: linux-trace-kernel@...r.kernel.org, peterz@...radead.org,
	rostedt@...dmis.org, mhiramat@...nel.org, x86@...nel.org,
	mingo@...hat.com, tglx@...utronix.de, linux-kernel@...r.kernel.org,
	bpf@...r.kernel.org, rihams@...com,
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v3] perf,x86: avoid missing caller address in stack
 traces captured in uprobe

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