Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1401545 > unrolled thread

[GIT PULL] EFI fix

Back to article view | Back to linux.kernel

Contents

  [GIT PULL] EFI fix Ingo Molnar <mingo@kernel.org> - 2016-05-16 16:50 +0200
    Re: [GIT PULL] EFI fix Linus Torvalds <torvalds@linux-foundation.org> - 2016-05-16 22:10 +0200
      Re: [GIT PULL] EFI fix Alex Thorlton <athorlton@sgi.com> - 2016-05-16 22:30 +0200
        Re: [GIT PULL] EFI fix Alex Thorlton <athorlton@sgi.com> - 2016-05-17 00:50 +0200
      [tip:x86/urgent] x86/efi: Fix 7-parameter efi_call()s tip-bot for Linus Torvalds <tipbot@zytor.com> - 2016-05-17 08:40 +0200
      Re: [GIT PULL] EFI fix Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-17 11:10 +0200
        Re: [GIT PULL] EFI fix Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-17 11:50 +0200
          Re: [GIT PULL] EFI fix Ingo Molnar <mingo@kernel.org> - 2016-05-17 12:30 +0200
            [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-17 16:50 +0200
              Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Linus Torvalds <torvalds@linux-foundation.org> - 2016-05-17 18:40 +0200
                Re: [PATCH] x86/asm/entry: fix stack return address retrieval in  thunk Steven Rostedt <rostedt@goodmis.org> - 2016-05-17 19:00 +0200
                  Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Linus Torvalds <torvalds@linux-foundation.org> - 2016-05-17 19:30 +0200
                  Re: [PATCH] x86/asm/entry: fix stack return address retrieval in  thunk Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-17 19:30 +0200
                [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-17 20:10 +0200
                  Re: [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk Linus Torvalds <torvalds@linux-foundation.org> - 2016-05-17 20:40 +0200
                  [tip:x86/urgent] x86/entry/64: Fix stack return address retrieval  in thunk tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2016-05-19 11:20 +0200
      Re: [GIT PULL] EFI fix Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-23 14:10 +0200
        Re: [GIT PULL] EFI fix Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 14:40 +0200
          Re: [GIT PULL] EFI fix Ingo Molnar <mingo@kernel.org> - 2016-05-24 11:10 +0200

#1401545 — [GIT PULL] EFI fix

Linus,

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.

 Thanks,

	Ingo

------------------>
Alex Thorlton (1):
      x86/efi: Fix 7th argument to efi_call()


 arch/x86/platform/efi/efi_stub_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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)

[toc] | [next] | [standalone]

#1401743

[Multipart message — attachments visible in raw view] — view raw

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.

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.

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?

                         Linus

[toc] | [prev] | [next] | [standalone]

#1401762

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

[toc] | [prev] | [next] | [standalone]

#1401800

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

[toc] | [prev] | [next] | [standalone]

#1402133 — [tip:x86/urgent] x86/efi: Fix 7-parameter efi_call()s

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)

[toc] | [prev] | [next] | [standalone]

#1402210

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.

[toc] | [prev] | [next] | [standalone]

#1402230

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.

[toc] | [prev] | [next] | [standalone]

#1402272

* 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1402456 — Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1402473 — Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1402487 — Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1402488 — Re: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1402511 — [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1402524 — Re: [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1403536 — [tip:x86/urgent] x86/entry/64: Fix stack return address retrieval in thunk

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

[toc] | [prev] | [next] | [standalone]

#1405309

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?

[toc] | [prev] | [next] | [standalone]

#1405325

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

[toc] | [prev] | [next] | [standalone]

#1405962

* 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web