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

Groups > linux.kernel > #1401743

Re: [GIT PULL] EFI fix

From Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups linux.kernel
Subject Re: [GIT PULL] EFI fix
Date 2016-05-16 22:10 +0200
Message-ID <rzwZY-3eP-9@gated-at.bofh.it> (permalink)
References <rzs0h-8f4-1@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

[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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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

csiph-web