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] [day] [month] [year] [list]

Message-ID: <CAEf4BzYja7imAz-kW+6WF03dQGkwfugw79p6reVmHbc8BTFOjw@mail.gmail.com>
Date: Wed, 3 Jul 2024 16:26:36 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: Andrii Nakryiko <andrii@...nel.org>, 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 Wed, Jul 3, 2024 at 3:39 PM Josh Poimboeuf <jpoimboe@...nel.org> wrote:
>
> 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()?

Ah, it's __get_user vs get_user quirk, right? Should I just use
get_user() here? It seems like existing code is trying to avoid two
__access_ok() checks for two fields of stack_frame, but here we don't
have that optimization opportunity anyways.

>
> 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:

Sure, can do.

>
>         ...
>         pagefault_disable();

But I'd leave pagefault_disable() outside of that function, because
caller has to do it either way.

>
>         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.

I'm not sure about "at least several days", tbh. I generally try to
not post more often than once a day, and that only if I received some
meaningful reviewing feedback (like in your case). I do wait a few
days for reviews before pinging the mailing list again, though.

Would I get this feedback if I haven't posted v3? Or we'd just be
delaying the inevitable for a few more idle days? This particular
change (in it's initial version before yours and recent Peter's
comments) has been sitting under review since May 8th ([0], and then
posted without changes on May 21st, [1]), so I'm not exactly rushing
the things here.

Either way, I won't get to this until next week, so I won't spam you
too much anymore, sorry.

  [0] https://lore.kernel.org/linux-trace-kernel/20240508212605.4012172-4-andrii@kernel.org/
  [1] https://lore.kernel.org/linux-trace-kernel/20240522013845.1631305-4-andrii@kernel.org/

>
> --
> Josh