Re: [GIT PULL] EFI fix

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 843 bytes --]

 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 92723aeae0f9..0a995aaddcfc 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)