[PATCH] x86/ftrace: Fix ebp in ftrace_regs_caller that screws up unwinder

[PATCH] x86/ftrace: Fix ebp in ftrace_regs_caller that screws up unwinder

[Steven Rostedt (VMware)]

From: Steven Rostedt (VMware) <rostedt@goodmis.org>

Fengguang Wu's zero day bot triggered a stack unwinder dump. This can
be easily triggered when CONFIG_FRAME_POINTERS is enabled and -mfentry
is in use on x86_32.

 ># cd /sys/kernel/debug/tracing
 ># echo 'p:schedule schedule' > kprobe_events
 ># echo stacktrace > events/kprobes/schedule/trigger

This is because the code that implemented fentry in the
ftrace_regs_caller tried to use the least amount of #ifdefs, and
modified ebp when CC_USE_FENTRY was defined to point to the parent ip
as it does when CC_USE_FENTRY is not defined. But when
CONFIG_FRAME_POINTERS is set, it corrupts the ebp register for this
frame while doing the tracing.

NOTE, it does not corrupt ebp in any other way. It is just a bad
frame pointer when calling into the tracing infrastructure. The original
ebp is restored before returning from the fentry call. But if a stack
trace is performed inside the tracing, the unwinder will notice the bad
ebp.

Instead of toying with ebp with CC_USING_FENTRY, just slap the parent
ip into the second parameter (%edx), and have an #else that does it the
original way.

The unwinder will unfortunately miss the function being traced, as the
stack frame is not set up yet for it, as it is for x86_64. But fixing
that is a bit more complex and did not work before anyway.

This has been tested with and without FRAME_POINTERS being set while
using -mfentry, as well as using an older compiler that uses mcount.

Reported-by: kernel test robot <fengguang.wu@intel.com>
Analyzed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lists.01.org/pipermail/lkp/2017-April/006165.html
Fixes: 644e0e8dc76b ("x86/ftrace: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 07f4035..722a145 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -128,14 +128,14 @@ ENTRY(ftrace_regs_caller)
 	pushl	%edx
 	pushl	%ecx
 	pushl	%ebx
-#ifdef CC_USING_FENTRY
-	/* Load 4 off of the parent ip addr into ebp */
-	lea	14*4(%esp), %ebp
-#endif
 
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
+#ifdef CC_USING_FENTRY
+	movl	15*4(%esp), %edx		/* Load parent ip (2nd parameter) */
+#else
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
+#endif
 	movl	function_trace_op, %ecx		/* Save ftrace_pos in 3rd parameter */
 	pushl	%esp				/* Save pt_regs as 4th parameter */
 

Re: [PATCH] x86/ftrace: Fix ebp in ftrace_regs_caller that screws up unwinder

[Josh Poimboeuf]

On Thu, Apr 20, 2017 at 05:22:36PM -0400, Steven Rostedt (VMware) wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> Fengguang Wu's zero day bot triggered a stack unwinder dump. This can
> be easily triggered when CONFIG_FRAME_POINTERS is enabled and -mfentry
> is in use on x86_32.
> 
>  ># cd /sys/kernel/debug/tracing
>  ># echo 'p:schedule schedule' > kprobe_events
>  ># echo stacktrace > events/kprobes/schedule/trigger
> 
> This is because the code that implemented fentry in the
> ftrace_regs_caller tried to use the least amount of #ifdefs, and
> modified ebp when CC_USE_FENTRY was defined to point to the parent ip
> as it does when CC_USE_FENTRY is not defined. But when
> CONFIG_FRAME_POINTERS is set, it corrupts the ebp register for this
> frame while doing the tracing.
> 
> NOTE, it does not corrupt ebp in any other way. It is just a bad
> frame pointer when calling into the tracing infrastructure. The original
> ebp is restored before returning from the fentry call. But if a stack
> trace is performed inside the tracing, the unwinder will notice the bad
> ebp.
> 
> Instead of toying with ebp with CC_USING_FENTRY, just slap the parent
> ip into the second parameter (%edx), and have an #else that does it the
> original way.
> 
> The unwinder will unfortunately miss the function being traced, as the
> stack frame is not set up yet for it, as it is for x86_64. But fixing
> that is a bit more complex and did not work before anyway.
> 
> This has been tested with and without FRAME_POINTERS being set while
> using -mfentry, as well as using an older compiler that uses mcount.
> 
> Reported-by: kernel test robot <fengguang.wu@intel.com>
> Analyzed-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Link: https://lists.01.org/pipermail/lkp/2017-April/006165.html
> Fixes: 644e0e8dc76b ("x86/ftrace: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

It's actually CONFIG_FRAME_POINTER (no 'S').  Otherwise,

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[tip:x86/asm] x86/ftrace: Fix ebp in ftrace_regs_caller that screws up unwinder

[tip-bot for Steven Rostedt (VMware)]

Commit-ID:  dc912c303517b01960dcee6875a78b2999f7c098
Gitweb:     http://git.kernel.org/tip/dc912c303517b01960dcee6875a78b2999f7c098
Author:     Steven Rostedt (VMware) <rostedt@goodmis.org>
AuthorDate: Thu, 20 Apr 2017 17:22:36 -0400
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 21 Apr 2017 09:48:16 +0200

x86/ftrace: Fix ebp in ftrace_regs_caller that screws up unwinder

Fengguang Wu's zero day bot triggered a stack unwinder dump. This can
be easily triggered when CONFIG_FRAME_POINTERS is enabled and -mfentry
is in use on x86_32.

 ># cd /sys/kernel/debug/tracing
 ># echo 'p:schedule schedule' > kprobe_events
 ># echo stacktrace > events/kprobes/schedule/trigger

This is because the code that implemented fentry in the ftrace_regs_caller
tried to use the least amount of #ifdefs, and modified ebp when
CC_USE_FENTRY was defined to point to the parent ip as it does when
CC_USE_FENTRY is not defined. But when CONFIG_FRAME_POINTERS is set, it
corrupts the ebp register for this frame while doing the tracing.

NOTE, it does not corrupt ebp in any other way. It is just a bad frame
pointer when calling into the tracing infrastructure. The original ebp is
restored before returning from the fentry call. But if a stack trace is
performed inside the tracing, the unwinder will notice the bad ebp.

Instead of toying with ebp with CC_USING_FENTRY, just slap the parent ip
into the second parameter (%edx), and have an #else that does it the
original way.

The unwinder will unfortunately miss the function being traced, as the
stack frame is not set up yet for it, as it is for x86_64. But fixing that
is a bit more complex and did not work before anyway.

This has been tested with and without FRAME_POINTERS being set while using
-mfentry, as well as using an older compiler that uses mcount.

Analyzed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Fixes: 644e0e8dc76b ("x86/ftrace: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set")
Reported-by: kernel test robot <fengguang.wu@intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lists.01.org/pipermail/lkp/2017-April/006165.html
Link: http://lkml.kernel.org/r/20170420172236.7af7f6e5@gandalf.local.home
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/kernel/ftrace_32.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 07f4035..722a145 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -128,14 +128,14 @@ ENTRY(ftrace_regs_caller)
 	pushl	%edx
 	pushl	%ecx
 	pushl	%ebx
-#ifdef CC_USING_FENTRY
-	/* Load 4 off of the parent ip addr into ebp */
-	lea	14*4(%esp), %ebp
-#endif
 
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
+#ifdef CC_USING_FENTRY
+	movl	15*4(%esp), %edx		/* Load parent ip (2nd parameter) */
+#else
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
+#endif
 	movl	function_trace_op, %ecx		/* Save ftrace_pos in 3rd parameter */
 	pushl	%esp				/* Save pt_regs as 4th parameter */