[PATCH] x86/asm/entry: fix stack return address retrieval in thunk

[Josh Poimboeuf]

On Tue, May 17, 2016 at 12:20:49PM +0200, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Tue, 17 May, at 10:04:34AM, Matt Fleming wrote:
> > > 
> > > Now I'm wondering whether other users of FRAME_BEGIN/FRAME_END make
> > > this same mistake. Coccinelle might be able to detect it perhaps.
> > 
> > A quick bit of sed turned up the code in arch/x86/entry/entry_64.S,
> > which looks to suffer from the same bug,
> 
> That would be arch/x86/entry/thunk_64.S, but yeah, good find!
> 
> >         /* rdi: arg1 ... normal C conventions. rax is saved/restored. */
> >         .macro THUNK name, func, put_ret_addr_in_rdi=0
> >         .globl \name
> >         .type \name, @function
> > \name:
> >         FRAME_BEGIN
> > 
> >         /* this one pushes 9 elems, the next one would be %rIP */
> >         pushq %rdi
> >         pushq %rsi
> >         pushq %rdx
> >         pushq %rcx
> >         pushq %rax
> >         pushq %r8
> >         pushq %r9
> >         pushq %r10
> >         pushq %r11
> > 
> >         .if \put_ret_addr_in_rdi
> >         /* 9*8(%rsp) is return addr on stack */
> >         movq 9*8(%rsp), %rdi
> >         .endif
> > 
> > With CONFIG_FRAME_POINTER=y 9*8(%rsp) is actually the value of %rbp on
> > entry, not the return address.
> 
> This seems to affect:
> 
> #ifdef CONFIG_TRACE_IRQFLAGS
>         THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
>         THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
> #endif
> 
> and with TRACE_IRQFLAGS we already in most times force frame pointers, such as 
> when LOCKDEP is enabled:
> 
>   config LOCKDEP
>         bool
>         depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
>   ...
>         select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !SCORE
> 
> Also, unlike the efi_call() case, this thunk_64.S bug affects the quality of 
> debug/tracing information, not runtime correctness per se.
> 
> still all that is by accident, not by design - this bug should be fixed too.

---

From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
Subject: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk

With CONFIG_FRAME_POINTER enabled, the thunk can pass a bad return
address value to the called function.  '9*8(%rsp)' actually gets the
frame pointer, not the return address.

The only users of the 'put_ret_addr_in_rdi' option are some functions
which trace the enabling and disabling of interrupts, so this bug can
result in bad debug/tracing information.

Fixes: 058fb73274f9 ("x86/asm/entry: Create stack frames in thunk functions")
Reported-by: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
---
 arch/x86/entry/thunk_64.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index 98df1fa..dae7ca0 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -15,9 +15,10 @@
 	.globl \name
 	.type \name, @function
 \name:
+	/* push 1 register if frame pointers are enabled */
 	FRAME_BEGIN
 
-	/* this one pushes 9 elems, the next one would be %rIP */
+	/* push 9 registers */
 	pushq %rdi
 	pushq %rsi
 	pushq %rdx
@@ -29,8 +30,8 @@
 	pushq %r11
 
 	.if \put_ret_addr_in_rdi
-	/* 9*8(%rsp) is return addr on stack */
-	movq 9*8(%rsp), %rdi
+	/* (9*8)+FRAME_OFFSET(%rsp) is return addr on stack */
+	movq (9*8)+FRAME_OFFSET(%rsp), %rdi
 	.endif
 
 	call \func
-- 
2.4.11