linux-kernel - Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack dump reliability issues

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]

Date:	Mon, 1 Aug 2016 10:36:33 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Brian Gerst <brgerst@...il.com>,
	Kees Cook <keescook@...omium.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Byungchul Park <byungchul.park@....com>
Subject: Re: [PATCH 05/19] x86/dumpstack: fix function graph tracing stack
 dump reliability issues

On Mon, Aug 01, 2016 at 10:28:21AM -0400, Steven Rostedt wrote:
> On Sat, 30 Jul 2016 08:51:25 -0500
> Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> 
> > On Fri, Jul 29, 2016 at 10:20:36PM -0400, Steven Rostedt wrote:
> > > On Fri, 29 Jul 2016 19:50:59 -0500
> > > Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > >   
> > > > BTW, it would be really nice if ftrace_graph_ret_addr() were idempotent
> > > > so we could get the "real" return address without having to pass in a
> > > > state variable.
> > > > 
> > > > For example we could add an "unsigned long *retp" pointer to
> > > > ftrace_ret_stack, which points to the return address on the stack.  Then
> > > > we could get rid of the index state variable in ftrace_graph_ret_addr,
> > > > and also then there would never be a chance of the stack dump getting
> > > > out of sync with the ret_stack.
> > > > 
> > > > What do you think?
> > > >   
> > > 
> > > I don't want to extend ret_stack as that is allocated 50 of these
> > > structures for every task. That said, we have the "fp" field that's
> > > used to check for frame pointer corruption when mcount is used. With
> > > CC_USING_FENTRY, that field is ignored. Perhaps we could overload that
> > > field for this.  
> > 
> > In that case, I guess we would need two versions of
> > ftrace_graph_ret_addr(), with the current implementation still needed
> > for mcount+HAVE_FUNCTION_GRAPH_FP_TEST.
> 
> How hard would it be in that case?

Well, it would be easy enough, but then the caller would still need to
pass in the state variable.  So maybe it's not worth the trouble.

> > Or would you want to get rid of HAVE_FUNCTION_GRAPH_FP_TEST for x86?
> 
> No, because there's gcc versions that we still support that mess up
> mcount, and could still cause issues with function graph.
> 
> 
> > 
> > BTW, on a different note, should I put ftrace_graph_ret_addr() to
> > kernel/trace/trace_functions_graph.c so other arches can use it?
> > 
> 
> I guess you could. There doesn't seem to be any x86 specific code in
> that right?

Right.  And I noticed that several arches implement this same
functionality in a slightly different way to fit with their stack dump
code.  So it would be nice to make it common.

-- 
Josh