On Tue, May 17, 2016 at 12:20:49PM +0200, Ingo Molnar wrote:
>
> * Matt Fleming <m...@codeblueprint.co.uk> 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 <jpoim...@redhat.com>
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 <m...@codeblueprint.co.uk>
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
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
