* Re: [GIT PULL] EFI fix
2016-05-16 20:05 ` Linus Torvalds
@ 2016-05-16 20:23 ` Alex Thorlton
2016-05-16 22:40 ` Alex Thorlton
2016-05-17 6:30 ` [tip:x86/urgent] x86/efi: Fix 7-parameter efi_call()s tip-bot for Linus Torvalds
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Alex Thorlton @ 2016-05-16 20:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Alex Thorlton, Linux Kernel Mailing List,
Matt Fleming, Thomas Gleixner, H. Peter Anvin, Andrew Morton,
Peter Zijlstra, Borislav Petkov
On Mon, May 16, 2016 at 01:05:45PM -0700, Linus Torvalds wrote:
> On Mon, May 16, 2016 at 7:46 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Please pull the latest efi-urgent-for-linus git tree from:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-for-linus
> >
> > # HEAD: bea23c757f66d91dac8fdadd94da0cba6b0b66bc x86/efi: Fix 7th argument to efi_call()
> >
> > A leftover fix from the v4.6 cycle.
>
> I'm not pulling this. It seems to be completely broken unless I'm
> mis-reading things.
>
> > diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
> > index 92723aeae0f9..62938ffbb9f9 100644
> > --- a/arch/x86/platform/efi/efi_stub_64.S
> > +++ b/arch/x86/platform/efi/efi_stub_64.S
> > @@ -43,7 +43,7 @@ ENTRY(efi_call)
> > FRAME_BEGIN
> > SAVE_XMM
> > mov (%rsp), %rax
> > - mov 8(%rax), %rax
> > + mov 16(%rax), %rax
> > subq $48, %rsp
> > mov %r9, 32(%rsp)
> > mov %rax, 40(%rsp)
>
> This code is an unmitigated disaster. It makes no sense, but the
> reason I refuse to pull it is that it also seems to be buggy - with or
> without that patch.
>
> In particular,. the SAME_XMM code saves the old stack pointer, but
> that's just crazy. It saves the stack pointer *AFTER* we've done that
>
> FRAME_BEGIN
>
> which will have *changed* the stack pointer, depending on whether
> stack frames are enabled or not.
>
> So when the code then does
>
> mov (%rsp), %rax
>
> we now move that old stack pointer into %rax, but the offset off that
> stack pointer will depend on whether that FRAME_BEGIN saved off %rbp
> or not.
>
> So that whole 8-vs-16 offset confusion depends on the frame pointer!
> If frame pointers were enabled, it will be 16. If they weren't, it
> will be 8. That patch that changes it from 8 to 16 will just move the
> bug around. Before, it was correct when frame pointers were disabled
> and buggy otherwise. Now, it's correct if frame pointers are enabled,
> and buggy otherwise.
This makes sense. I missed the implication of the conditionally defined
FRAME_BEGIN being used at the beginning of this function. My fix works
on our machines, because we always have frame pointers enabled, but I do
see, now, how this effectively just moves the bug.
> I may be missing something, but I think that commit is pure garbage.
>
> I think the right fix is to just get rid of that silly conditional
> frame pointer thing, and always use frame pointers in this stub
> function. And then we don't need that (odd) load to get the old stack
> pointer into %rax - we can just use the frame pointer.
After having read your explanation, I agree. If we leave in the
conditional frame pointer chunk, we'll have to do some other trickery to
make sure that we get the offset for the 7th arg correct, which sounds
ugly. Your idea seems like a much cleaner fix.
> Something like the attached completely untested patch.
>
> But maybe I was missing something. Maybe my patch is crap and the
> patch above is right for some reason that completely evades me.
>
> Since this apparently only affects the SGI EFI stuff, can you please
> test this, Alex?
Everything discussed above makes sense to me, and the patch looks sane.
I will apply and test it today and let you know how it works.
Thanks for looking at this!
- Alex
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [GIT PULL] EFI fix
2016-05-16 20:23 ` Alex Thorlton
@ 2016-05-16 22:40 ` Alex Thorlton
0 siblings, 0 replies; 19+ messages in thread
From: Alex Thorlton @ 2016-05-16 22:40 UTC (permalink / raw)
To: Alex Thorlton
Cc: Linus Torvalds, Ingo Molnar, Linux Kernel Mailing List,
Matt Fleming, Thomas Gleixner, H. Peter Anvin, Andrew Morton,
Peter Zijlstra, Borislav Petkov
On Mon, May 16, 2016 at 03:23:51PM -0500, Alex Thorlton wrote:
> Everything discussed above makes sense to me, and the patch looks sane.
> I will apply and test it today and let you know how it works.
I applied this to the latest -tip kernel and tested on both real
hardware and in our simulator. Everything appears to be behaving as
expected.
I still need to work on the other patch in the patchset to get our
callbacks working with the new EFI page tables, but I believe we have a
clear path forward there, and that is (mostly) a separate issue.
Thanks again for catching this mistake, Linus!
- Alex
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:x86/urgent] x86/efi: Fix 7-parameter efi_call()s
2016-05-16 20:05 ` Linus Torvalds
2016-05-16 20:23 ` Alex Thorlton
@ 2016-05-17 6:30 ` tip-bot for Linus Torvalds
2016-05-17 9:04 ` [GIT PULL] EFI fix Matt Fleming
2016-05-23 12:08 ` [GIT PULL] EFI fix Matt Fleming
3 siblings, 0 replies; 19+ messages in thread
From: tip-bot for Linus Torvalds @ 2016-05-17 6:30 UTC (permalink / raw)
To: linux-tip-commits
Cc: bp, eranian, dvlasenk, vincent.weaver, linux-kernel, brgerst,
matt, a.p.zijlstra, peterz, hpa, jolsa, luto, athorlton, mingo,
acme, alexander.shishkin, tglx, torvalds, akpm
Commit-ID: 683ad8092cd262a02d01377dd17a29d492438b90
Gitweb: http://git.kernel.org/tip/683ad8092cd262a02d01377dd17a29d492438b90
Author: Linus Torvalds <torvalds@linux-foundation.org>
AuthorDate: Mon, 16 May 2016 13:05:45 -0700
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 May 2016 08:25:06 +0200
x86/efi: Fix 7-parameter efi_call()s
Alex Thorlton reported that the SGI/UV code crashes in the efi_call()
code when invoked with 7 parameters, due to:
mov (%rsp), %rax
mov 8(%rax), %rax
...
mov %rax, 40(%rsp)
Offset 8 is only true if CONFIG_FRAME_POINTERS is disabled,
with frame pointers enabled it should be 16.
Furthermore, the SAVE_XMM code saves the old stack pointer, but
that's just crazy. It saves the stack pointer *AFTER* we've done
the:
FRAME_BEGIN
... which will have *changed* the stack pointer, depending on whether
stack frames are enabled or not.
So when the code then does:
mov (%rsp), %rax
... we now move that old stack pointer into %rax, but the offset off that
stack pointer will depend on whether that FRAME_BEGIN saved off %rbp
or not.
So that whole 8-vs-16 offset confusion depends on the frame pointer!
If frame pointers were enabled, it will be 16. If they weren't, it
will be 8.
The right fix is to just get rid of that silly conditional frame
pointer thing, and always use frame pointers in this stub function.
And then we don't need that (odd) load to get the old stack
pointer into %rax - we can just use the frame pointer.
Reported-by: Alex Thorlton <athorlton@sgi.com>
Tested-by: Alex Thorlton <athorlton@sgi.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Link: http://lkml.kernel.org/r/CA%2B55aFzBS2v%3DWnEH83cUDg7XkOremFqJ30BJwF40dCYjReBkUQ@mail.gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/platform/efi/efi_stub_64.S | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
index 92723ae..cd95075 100644
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -11,7 +11,6 @@
#include <asm/msr.h>
#include <asm/processor-flags.h>
#include <asm/page_types.h>
-#include <asm/frame.h>
#define SAVE_XMM \
mov %rsp, %rax; \
@@ -40,10 +39,10 @@
mov (%rsp), %rsp
ENTRY(efi_call)
- FRAME_BEGIN
+ pushq %rbp
+ movq %rsp, %rbp
SAVE_XMM
- mov (%rsp), %rax
- mov 8(%rax), %rax
+ mov 16(%rbp), %rax
subq $48, %rsp
mov %r9, 32(%rsp)
mov %rax, 40(%rsp)
@@ -53,6 +52,6 @@ ENTRY(efi_call)
call *%rdi
addq $48, %rsp
RESTORE_XMM
- FRAME_END
+ popq %rbp
ret
ENDPROC(efi_call)
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [GIT PULL] EFI fix
2016-05-16 20:05 ` Linus Torvalds
2016-05-16 20:23 ` Alex Thorlton
2016-05-17 6:30 ` [tip:x86/urgent] x86/efi: Fix 7-parameter efi_call()s tip-bot for Linus Torvalds
@ 2016-05-17 9:04 ` Matt Fleming
2016-05-17 9:46 ` Matt Fleming
2016-05-23 12:08 ` [GIT PULL] EFI fix Matt Fleming
3 siblings, 1 reply; 19+ messages in thread
From: Matt Fleming @ 2016-05-17 9:04 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Alex Thorlton, Linux Kernel Mailing List,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Peter Zijlstra,
Borislav Petkov, Josh Poimboeuf
On Mon, 16 May, at 01:05:45PM, Linus Torvalds wrote:
>
> So that whole 8-vs-16 offset confusion depends on the frame pointer!
> If frame pointers were enabled, it will be 16. If they weren't, it
> will be 8. That patch that changes it from 8 to 16 will just move the
> bug around. Before, it was correct when frame pointers were disabled
> and buggy otherwise. Now, it's correct if frame pointers are enabled,
> and buggy otherwise.
Urgh, right.
We didn't always use frame pointers in efi_call(), they were
introduced in commit 779c433b8ea5c ("x86/asm/efi: Create a stack frame
in efi_call()") earlier this year to stop objtool complaining.
I admit I totally missed the part about pulling arguments off the
stack when I reviewed that patch.
> I may be missing something, but I think that commit is pure garbage.
You're correct.
> I think the right fix is to just get rid of that silly conditional
> frame pointer thing, and always use frame pointers in this stub
> function. And then we don't need that (odd) load to get the old stack
> pointer into %rax - we can just use the frame pointer.
>
> Something like the attached completely untested patch.
Looks good to me, but I haven't tested it.
Now I'm wondering whether other users of FRAME_BEGIN/FRAME_END make
this same mistake. Coccinelle might be able to detect it perhaps.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [GIT PULL] EFI fix
2016-05-17 9:04 ` [GIT PULL] EFI fix Matt Fleming
@ 2016-05-17 9:46 ` Matt Fleming
2016-05-17 10:20 ` Ingo Molnar
0 siblings, 1 reply; 19+ messages in thread
From: Matt Fleming @ 2016-05-17 9:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Alex Thorlton, Linux Kernel Mailing List,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Peter Zijlstra,
Borislav Petkov, Josh Poimboeuf
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,
/* 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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [GIT PULL] EFI fix
2016-05-17 9:46 ` Matt Fleming
@ 2016-05-17 10:20 ` Ingo Molnar
2016-05-17 14:43 ` [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Josh Poimboeuf
0 siblings, 1 reply; 19+ messages in thread
From: Ingo Molnar @ 2016-05-17 10:20 UTC (permalink / raw)
To: Matt Fleming
Cc: Linus Torvalds, Alex Thorlton, Linux Kernel Mailing List,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Peter Zijlstra,
Borislav Petkov, Josh Poimboeuf, Andy Lutomirski, Steven Rostedt,
Denys Vlasenko
* Matt Fleming <matt@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.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 10:20 ` Ingo Molnar
@ 2016-05-17 14:43 ` Josh Poimboeuf
2016-05-17 16:31 ` Linus Torvalds
0 siblings, 1 reply; 19+ messages in thread
From: Josh Poimboeuf @ 2016-05-17 14:43 UTC (permalink / raw)
To: Ingo Molnar
Cc: Matt Fleming, Linus Torvalds, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Steven Rostedt, Denys Vlasenko
On Tue, May 17, 2016 at 12:20:49PM +0200, Ingo Molnar wrote:
>
> * Matt Fleming <matt@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 <jpoimboe@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 <matt@codeblueprint.co.uk>
Signed-off-by: Josh Poimboeuf <jpoimboe@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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 14:43 ` [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Josh Poimboeuf
@ 2016-05-17 16:31 ` Linus Torvalds
2016-05-17 16:51 ` Steven Rostedt
2016-05-17 18:06 ` [PATCH v2] " Josh Poimboeuf
0 siblings, 2 replies; 19+ messages in thread
From: Linus Torvalds @ 2016-05-17 16:31 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ingo Molnar, Matt Fleming, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Steven Rostedt, Denys Vlasenko
On Tue, May 17, 2016 at 7:43 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> 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 */
I don't hate this patch, but quite frankly, as with the other case,
I'd just make the frame pointer be unconditional in this case.
If we push nine other registers, the frame pointer setup code is *not*
going to matter.
The reason to avoid frame pointers in code generation is two-fold:
1) for small leaf functions, it often ends up dominating
2) it removes a register that is otherwise usable, which can be
particularly bad on 32-bit x86 due to the much more limited number of
registers (and was apparently really noticeable on the older on-order
atom cores)
and in this case neither of them is really an issue.
So I would suggest that any case that actually depends on a frame
access just make the frame pointer not just unconditional, but
_explicit_.
So not just avoiding the macro because it's conditional, but write out
the sequence to actually set up the frame, and then use
- movq 9*8(%rsp), %rdi
+ movq 8(%rbp), %rdi # return address
to entirely avoid all kind of "how many registers have we pushed" math.
Considering that we got this wrong in two places, it's clearly too
subtle for our little brains as-is.
Linus
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 16:31 ` Linus Torvalds
@ 2016-05-17 16:51 ` Steven Rostedt
2016-05-17 17:21 ` Linus Torvalds
2016-05-17 17:25 ` Josh Poimboeuf
2016-05-17 18:06 ` [PATCH v2] " Josh Poimboeuf
1 sibling, 2 replies; 19+ messages in thread
From: Steven Rostedt @ 2016-05-17 16:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: Josh Poimboeuf, Ingo Molnar, Matt Fleming, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Denys Vlasenko
On Tue, 17 May 2016 09:31:12 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Considering that we got this wrong in two places, it's clearly too
> subtle for our little brains as-is.
And did we only get this wrong in two places? That is, do we really
know how little our brains really are?
-- Steve
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 16:51 ` Steven Rostedt
@ 2016-05-17 17:21 ` Linus Torvalds
2016-05-17 17:25 ` Josh Poimboeuf
1 sibling, 0 replies; 19+ messages in thread
From: Linus Torvalds @ 2016-05-17 17:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: Josh Poimboeuf, Ingo Molnar, Matt Fleming, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Denys Vlasenko
On Tue, May 17, 2016 at 9:51 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> And did we only get this wrong in two places? That is, do we really
> know how little our brains really are?
Doing some grepping seems to imply that was it.
There's a fair number of cases in
arch/x86/crypto/aesni-intel_asm.S
but they actually use the proper FRAME_OFFSET.
Here's my grep pattern:
git grep -Elw 'FRAME_((BEGIN)|(END))' |
xargs grep --color=always -E
'(FRAME_((BEGIN)|(END)))|(\(.*%[er]sp.*\))' |
less -SFRX
I'm not going to guarantee anything, but both
arch/x86/entry/thunk_64.S and arch/x86/platform/efi/efi_stub_64.S
stood out with this grep as having stack pointer accesses between a
FRAME_BEGIN and FRAME_END, so at least that pattern ends up finding
the two known problem cases.
Yeah, I know, I should have used 'awk' for this. Sue me. It's been too
long since I did awk state machines. There's a reason there's a "git
grep" but not a "git awk" command.
Linus
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 16:51 ` Steven Rostedt
2016-05-17 17:21 ` Linus Torvalds
@ 2016-05-17 17:25 ` Josh Poimboeuf
1 sibling, 0 replies; 19+ messages in thread
From: Josh Poimboeuf @ 2016-05-17 17:25 UTC (permalink / raw)
To: Steven Rostedt
Cc: Linus Torvalds, Ingo Molnar, Matt Fleming, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Denys Vlasenko
On Tue, May 17, 2016 at 12:51:41PM -0400, Steven Rostedt wrote:
> On Tue, 17 May 2016 09:31:12 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > Considering that we got this wrong in two places, it's clearly too
> > subtle for our little brains as-is.
>
> And did we only get this wrong in two places? That is, do we really
> know how little our brains really are?
I've looked again at all the other users of FRAME_BEGIN, and this
pattern of a callee attempting to access the caller's stack is pretty
rare in x86_64 (which is the main user of the macro). My little brain
is telling me there are no more bugs.
--
Josh
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 16:31 ` Linus Torvalds
2016-05-17 16:51 ` Steven Rostedt
@ 2016-05-17 18:06 ` Josh Poimboeuf
2016-05-17 18:33 ` Linus Torvalds
2016-05-19 9:12 ` [tip:x86/urgent] x86/entry/64: Fix " tip-bot for Josh Poimboeuf
1 sibling, 2 replies; 19+ messages in thread
From: Josh Poimboeuf @ 2016-05-17 18:06 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Matt Fleming, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Steven Rostedt, Denys Vlasenko
On Tue, May 17, 2016 at 09:31:12AM -0700, Linus Torvalds wrote:
> On Tue, May 17, 2016 at 7:43 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > 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 */
>
> I don't hate this patch, but quite frankly, as with the other case,
> I'd just make the frame pointer be unconditional in this case.
>
> If we push nine other registers, the frame pointer setup code is *not*
> going to matter.
>
> The reason to avoid frame pointers in code generation is two-fold:
>
> 1) for small leaf functions, it often ends up dominating
>
> 2) it removes a register that is otherwise usable, which can be
> particularly bad on 32-bit x86 due to the much more limited number of
> registers (and was apparently really noticeable on the older on-order
> atom cores)
>
> and in this case neither of them is really an issue.
>
> So I would suggest that any case that actually depends on a frame
> access just make the frame pointer not just unconditional, but
> _explicit_.
>
> So not just avoiding the macro because it's conditional, but write out
> the sequence to actually set up the frame, and then use
>
> - movq 9*8(%rsp), %rdi
> + movq 8(%rbp), %rdi # return address
>
> to entirely avoid all kind of "how many registers have we pushed" math.
>
> Considering that we got this wrong in two places, it's clearly too
> subtle for our little brains as-is.
Makes sense, thanks. Here's v2:
---
From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk
With CONFIG_FRAME_POINTER enabled, a 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 two functions
which trace the enabling and disabling of interrupts, so this bug can
result in bad debug or tracing information with CONFIG_IRQSOFF_TRACER or
CONFIG_PROVE_LOCKING.
Fixes: 058fb73274f9 ("x86/asm/entry: Create stack frames in thunk functions")
Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/entry/thunk_64.S | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index 98df1fa..027aec4 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -8,16 +8,15 @@
#include <linux/linkage.h>
#include "calling.h"
#include <asm/asm.h>
-#include <asm/frame.h>
/* 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
+ pushq %rbp
+ movq %rsp, %rbp
- /* this one pushes 9 elems, the next one would be %rIP */
pushq %rdi
pushq %rsi
pushq %rdx
@@ -29,8 +28,8 @@
pushq %r11
.if \put_ret_addr_in_rdi
- /* 9*8(%rsp) is return addr on stack */
- movq 9*8(%rsp), %rdi
+ /* 8(%rbp) is return addr on stack */
+ movq 8(%rbp), %rdi
.endif
call \func
@@ -65,7 +64,7 @@ restore:
popq %rdx
popq %rsi
popq %rdi
- FRAME_END
+ popq %rbp
ret
_ASM_NOKPROBE(restore)
#endif
--
2.4.11
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk
2016-05-17 18:06 ` [PATCH v2] " Josh Poimboeuf
@ 2016-05-17 18:33 ` Linus Torvalds
2016-05-19 9:12 ` [tip:x86/urgent] x86/entry/64: Fix " tip-bot for Josh Poimboeuf
1 sibling, 0 replies; 19+ messages in thread
From: Linus Torvalds @ 2016-05-17 18:33 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Ingo Molnar, Matt Fleming, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov, Andy Lutomirski,
Steven Rostedt, Denys Vlasenko
On Tue, May 17, 2016 at 11:06 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Makes sense, thanks. Here's v2:
Yes, this looks much nicer to me. Thanks.
Ingo, feel free to add my "Acked-by" or whatever.
Linus
^ permalink raw reply [flat|nested] 19+ messages in thread* [tip:x86/urgent] x86/entry/64: Fix stack return address retrieval in thunk
2016-05-17 18:06 ` [PATCH v2] " Josh Poimboeuf
2016-05-17 18:33 ` Linus Torvalds
@ 2016-05-19 9:12 ` tip-bot for Josh Poimboeuf
1 sibling, 0 replies; 19+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2016-05-19 9:12 UTC (permalink / raw)
To: linux-tip-commits
Cc: hpa, mingo, dvlasenk, brgerst, torvalds, a.p.zijlstra, jpoimboe,
matt, bp, akpm, peterz, tglx, luto, rostedt, athorlton,
linux-kernel
Commit-ID: d4bf7078c43e11097e0d6f04d3fb999bf92c4fb0
Gitweb: http://git.kernel.org/tip/d4bf7078c43e11097e0d6f04d3fb999bf92c4fb0
Author: Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Tue, 17 May 2016 13:06:06 -0500
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 19 May 2016 09:12:34 +0200
x86/entry/64: Fix stack return address retrieval in thunk
With CONFIG_FRAME_POINTER enabled, a 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 two functions
which trace the enabling and disabling of interrupts, so this bug can
result in bad debug or tracing information with CONFIG_IRQSOFF_TRACER or
CONFIG_PROVE_LOCKING.
Fix this by implementing the suggestion of Linus: explicitly push
the frame pointer all the time and constify the stack offsets that
way. This is both correct and easier to read.
Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
[ Extended the changelog a bit. ]
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 058fb73274f9 ("x86/asm/entry: Create stack frames in thunk functions")
Link: http://lkml.kernel.org/r/20160517180606.v5o7wcgdni7443ol@treble
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/entry/thunk_64.S | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index 98df1fa..027aec4 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -8,16 +8,15 @@
#include <linux/linkage.h>
#include "calling.h"
#include <asm/asm.h>
-#include <asm/frame.h>
/* 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
+ pushq %rbp
+ movq %rsp, %rbp
- /* this one pushes 9 elems, the next one would be %rIP */
pushq %rdi
pushq %rsi
pushq %rdx
@@ -29,8 +28,8 @@
pushq %r11
.if \put_ret_addr_in_rdi
- /* 9*8(%rsp) is return addr on stack */
- movq 9*8(%rsp), %rdi
+ /* 8(%rbp) is return addr on stack */
+ movq 8(%rbp), %rdi
.endif
call \func
@@ -65,7 +64,7 @@ restore:
popq %rdx
popq %rsi
popq %rdi
- FRAME_END
+ popq %rbp
ret
_ASM_NOKPROBE(restore)
#endif
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [GIT PULL] EFI fix
2016-05-16 20:05 ` Linus Torvalds
` (2 preceding siblings ...)
2016-05-17 9:04 ` [GIT PULL] EFI fix Matt Fleming
@ 2016-05-23 12:08 ` Matt Fleming
2016-05-23 12:33 ` Josh Poimboeuf
3 siblings, 1 reply; 19+ messages in thread
From: Matt Fleming @ 2016-05-23 12:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Alex Thorlton, Linux Kernel Mailing List,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Peter Zijlstra,
Borislav Petkov, Josh Poimboeuf
On Mon, 16 May, at 01:05:45PM, Linus Torvalds wrote:
>
> I think the right fix is to just get rid of that silly conditional
> frame pointer thing, and always use frame pointers in this stub
> function. And then we don't need that (odd) load to get the old stack
> pointer into %rax - we can just use the frame pointer.
>
> Something like the attached completely untested patch.
Linus, are you going to apply your patch directly or would you prefer
someone to send a pull request which also includes Josh's patch for
arch/x86/entry/thunk_64.S?
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [GIT PULL] EFI fix
2016-05-23 12:08 ` [GIT PULL] EFI fix Matt Fleming
@ 2016-05-23 12:33 ` Josh Poimboeuf
2016-05-24 9:03 ` Ingo Molnar
0 siblings, 1 reply; 19+ messages in thread
From: Josh Poimboeuf @ 2016-05-23 12:33 UTC (permalink / raw)
To: Matt Fleming
Cc: Linus Torvalds, Ingo Molnar, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov
On Mon, May 23, 2016 at 01:08:01PM +0100, Matt Fleming wrote:
> On Mon, 16 May, at 01:05:45PM, Linus Torvalds wrote:
> >
> > I think the right fix is to just get rid of that silly conditional
> > frame pointer thing, and always use frame pointers in this stub
> > function. And then we don't need that (odd) load to get the old stack
> > pointer into %rax - we can just use the frame pointer.
> >
> > Something like the attached completely untested patch.
>
> Linus, are you going to apply your patch directly or would you prefer
> someone to send a pull request which also includes Josh's patch for
> arch/x86/entry/thunk_64.S?
Ingo already merged both patches into tip:x86/urgent, so I'd presume
he'll be sending a pull request for them soon.
--
Josh
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [GIT PULL] EFI fix
2016-05-23 12:33 ` Josh Poimboeuf
@ 2016-05-24 9:03 ` Ingo Molnar
0 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2016-05-24 9:03 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Matt Fleming, Linus Torvalds, Alex Thorlton,
Linux Kernel Mailing List, Thomas Gleixner, H. Peter Anvin,
Andrew Morton, Peter Zijlstra, Borislav Petkov
* Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, May 23, 2016 at 01:08:01PM +0100, Matt Fleming wrote:
> > On Mon, 16 May, at 01:05:45PM, Linus Torvalds wrote:
> > >
> > > I think the right fix is to just get rid of that silly conditional
> > > frame pointer thing, and always use frame pointers in this stub
> > > function. And then we don't need that (odd) load to get the old stack
> > > pointer into %rax - we can just use the frame pointer.
> > >
> > > Something like the attached completely untested patch.
> >
> > Linus, are you going to apply your patch directly or would you prefer
> > someone to send a pull request which also includes Josh's patch for
> > arch/x86/entry/thunk_64.S?
>
> Ingo already merged both patches into tip:x86/urgent, so I'd presume
> he'll be sending a pull request for them soon.
Correct, I plan to send them later today.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 19+ messages in thread