EFI fix

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.

Quoted 8 lines · Ingo Molnar

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.

arch/x86/platform/efi/efi_stub_64.S ENTRY(efi_call) · line 49

Quoted 13 lines

arch/x86/platform/efi/efi_stub_64.S +1 -1

index 92723aeae0f9..62938ffbb9f9 100644
@@ -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?

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.

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.

Now I'm wondering whether other users of FRAME_BEGIN/FRAME_END make this same mistake. Coccinelle might be able to detect it perhaps.

Quoted 3 lines · Matt Fleming

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.

Quoted 7 lines · Matt Fleming

Quoted 3 lines · Matt Fleming

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!

Quoted 23 lines

/* 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

Quoted 2 lines

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.

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?

Quoted 2 lines · On Tue, 17 May 2016 09:31:12 -0700 Linus Torvalds…

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?

arch/x86/entry/thunk_64.S line 21

Quoted 13 lines · Josh Poimboeuf

index 98df1fa..dae7ca0 100644

arch/x86/entry/thunk_64.S +2 -1

@@ -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

(quoted diff) +1 -1

-       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:

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.

Quoted 6 lines · Linus Torvalds

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.

Quoted 1 line

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?

Quoted 7 lines · Matt Fleming

Quoted 6 lines · Linus Torvalds

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.

Quoted 5 lines

Quoted 1 line

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.