linux-kernel - Re: [PATCH v12 02/14] unwind_user: Add frame pointer support

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: <20250701081443.05db9bdd@gandalf.local.home>
Date: Tue, 1 Jul 2025 08:14:43 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Florian Weimer <fweimer@...hat.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 bpf@...r.kernel.org, x86@...nel.org, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Josh Poimboeuf <jpoimboe@...nel.org>, Peter Zijlstra
 <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, Jiri Olsa
 <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Thomas Gleixner
 <tglx@...utronix.de>, Andrii Nakryiko <andrii@...nel.org>, Indu Bhagat
 <indu.bhagat@...cle.com>, "Jose E. Marchesi" <jemarch@....org>, Beau
 Belgrave <beaub@...ux.microsoft.com>, Jens Remus <jremus@...ux.ibm.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH v12 02/14] unwind_user: Add frame pointer support

On Tue, 01 Jul 2025 06:46:14 +0200
Florian Weimer <fweimer@...hat.com> wrote:

> * Linus Torvalds:
> 
> > On Mon, 30 Jun 2025 at 17:54, Steven Rostedt <rostedt@...dmis.org> wrote:  
> >>
> >> +       /* stack going in wrong direction? */
> >> +       if (cfa <= state->sp)
> >> +               goto done;  
> >
> > I suspect this should do a lot more testing.
> >  
> >> +       /* Find the Return Address (RA) */
> >> +       if (get_user(ra, (unsigned long *)(cfa + frame->ra_off)))
> >> +               goto done;
> >> +
> >> +       if (frame->fp_off && get_user(fp, (unsigned long __user *)(cfa + frame->fp_off)))
> >> +               goto done;  
> >
> > .. and this should check the frame for validity too.  At a minimum it
> > should be properly aligned, but things like "it had better be above
> > the current frame" to avoid having some loop would seem to be a good
> > idea.  
> 
> I don't think SFrame as-is requires stacks to be contiguous.  Maybe
> there could be a per-frame flag that indicates whether a stack switch is
> expected?

Looking at the current code of perf, it appears to only check that the
address is valid to read from user space. Perhaps that's the only check
needed here too?

Now this loop will not go into an infinite loop as the code has:

	for_each_user_frame(&state) {
		trace->entries[trace->nr++] = state.ip;
		if (trace->nr >= max_entries)
			break;
	}

Where

#define for_each_user_frame(state) \
	for (unwind_user_start((state)); !(state)->done; unwind_user_next((state)))

It will stop at "max_entries" even if the user space tries to make it go
forever. max_entries is either 511 (on 64 bit) or 1023 (on 32 bit), as it
is defined as:

/* Make the cache fit in a 4K page */
#define UNWIND_MAX_ENTRIES					\
	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))

Now, perhaps we need to verify that the cfa is indeed canonical, but what
other test do we have perform?

In the future, this code will also be handling JIT and possibly interpreted
code. How much do we really want to limit the stack walking due to checks?

-- Steve