[GIT PULL] EFI fix

[GIT PULL] EFI fix

[Ingo Molnar]

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)

Re: [GIT PULL] EFI fix

[Linus Torvalds]

[-- 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)

Re: [GIT PULL] EFI fix

[Alex Thorlton]

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

Re: [GIT PULL] EFI fix

[Alex Thorlton]

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

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

[tip-bot for Linus Torvalds]

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)

Re: [GIT PULL] EFI fix

[Matt Fleming]

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.

Re: [GIT PULL] EFI fix

[Matt Fleming]

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.

Re: [GIT PULL] EFI fix

[Ingo Molnar]

* 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

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

[Josh Poimboeuf]

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

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

[Linus Torvalds]

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

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

[Steven Rostedt]

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

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

[Linus Torvalds]

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

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

[Josh Poimboeuf]

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

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

[Josh Poimboeuf]

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

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

[Linus Torvalds]

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

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

[tip-bot for Josh Poimboeuf]

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

Re: [GIT PULL] EFI fix

[Matt Fleming]

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?

Re: [GIT PULL] EFI fix

[Josh Poimboeuf]

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

Re: [GIT PULL] EFI fix

[Ingo Molnar]

* 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

Re: [GIT PULL] EFI fix

[pr-tracker-bot]

The pull request you sent on Sat, 18 May 2019 11:17:26 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/39feaa3ff4453594297574e116a55bd6d5371f37

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

[GIT PULL] EFI fix

[Ingo Molnar]

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: f8585539df0a1527c78b5d760665c89fe1c105a9 fbdev/efifb: Ignore framebuffer memmap entries that lack any memory types

Fix an EFI-fb regression that affects certain x86 systems.

 Thanks,

	Ingo

------------------>
Ard Biesheuvel (1):
      fbdev/efifb: Ignore framebuffer memmap entries that lack any memory types


 drivers/video/fbdev/efifb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 9e529cc2b4ff..9f39f0c360e0 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -477,8 +477,12 @@ static int efifb_probe(struct platform_device *dev)
 		 * If the UEFI memory map covers the efifb region, we may only
 		 * remap it using the attributes the memory map prescribes.
 		 */
-		mem_flags |= EFI_MEMORY_WT | EFI_MEMORY_WB;
-		mem_flags &= md.attribute;
+		md.attribute &= EFI_MEMORY_UC | EFI_MEMORY_WC |
+				EFI_MEMORY_WT | EFI_MEMORY_WB;
+		if (md.attribute) {
+			mem_flags |= EFI_MEMORY_WT | EFI_MEMORY_WB;
+			mem_flags &= md.attribute;
+		}
 	}
 	if (mem_flags & EFI_MEMORY_WC)
 		info->screen_base = ioremap_wc(efifb_fix.smem_start,

Re: [GIT PULL] EFI fix

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Jan 10, 2019 at 11:46 PM Ingo Molnar <mingo@kernel.org> wrote:
> >
> > A single fix that adds an annotation to resolve a kmemleak false
> > positive.
> 
> This one is apparently obviated by commit 80424b02d42b ("efi: Reduce
> the amount of memblock reservations for persistent allocations")

... and it turns out I mis-merged it in tip:master not realizing this 
connection (hence the pull request) - so good thing that Ard warned about 
this and you double checked it!

Thanks,

	Ingo

Re: [GIT PULL] EFI fix

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Jan 11, 2019 at 6:22 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > I was hoping we could merge this patch (so we can backport it), but
> > resolve the conflict by dropping the kmemleak_ignore() again [..]
> 
> Well, we'd drop the new #include line also, since it would be
> pointless without the kmemleak_ignore().
> 
> End result: there would be nothing left. Better not to merge it at all.

Indeed!

> It's easy enough to backport, and just say "done differently upstream
> in commit 80424b02d42b ("efi: Reduce the amount of memblock
> reservations for persistent allocations").
> 
> The stable tree doesn't require that the *same* commits be upstream,
> it only requires that the fixes be upstream and Greg&al want a pointer
> to the upstream fix just so that they know they're not fixing
> something that might still be broken upstream.
> 
> See for example (just random googling)
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=37435f7e80ef9adc32a69013c18f135e3f434244
> 
> which shows that "fixed differently upstream" case and points to why.

Thanks - I'm dropping the commit from efi/urgent.

	Ingo

Re: [GIT PULL] EFI fix

[Linus Torvalds]

On Fri, Jan 11, 2019 at 6:22 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> I was hoping we could merge this patch (so we can backport it), but
> resolve the conflict by dropping the kmemleak_ignore() again [..]

Well, we'd drop the new #include line also, since it would be
pointless without the kmemleak_ignore().

End result: there would be nothing left. Better not to merge it at all.

It's easy enough to backport, and just say "done differently upstream
in commit 80424b02d42b ("efi: Reduce the amount of memblock
reservations for persistent allocations").

The stable tree doesn't require that the *same* commits be upstream,
it only requires that the fixes be upstream and Greg&al want a pointer
to the upstream fix just so that they know they're not fixing
something that might still be broken upstream.

See for example (just random googling)

   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=37435f7e80ef9adc32a69013c18f135e3f434244

which shows that "fixed differently upstream" case and points to why.

                  Linus

Re: [GIT PULL] EFI fix

[Linus Torvalds]

On Thu, Jan 10, 2019 at 11:46 PM Ingo Molnar <mingo@kernel.org> wrote:
>
> A single fix that adds an annotation to resolve a kmemleak false
> positive.

This one is apparently obviated by commit 80424b02d42b ("efi: Reduce
the amount of memblock reservations for persistent allocations")

               Linus

Re: [GIT PULL] EFI fix

[Ard Biesheuvel]

On Fri, 11 Jan 2019 at 08:46, Ingo Molnar <mingo@kernel.org> wrote:
>
> 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: b12f5440d8ca02e8f9ab4f1461f9214295cc4f66 Merge branch 'linus' into efi/urgent, to resolve conflict
>
> A single fix that adds an annotation to resolve a kmemleak false
> positive.
>
>  Thanks,
>
>         Ingo
>
> ------------------>
> Qian Cai (1):
>       efi: Let kmemleak ignore false positives
>
>
>  drivers/firmware/efi/efi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4c46ff6f2242..7ac09dd8f268 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -31,6 +31,7 @@
>  #include <linux/acpi.h>
>  #include <linux/ucs2_string.h>
>  #include <linux/memblock.h>
> +#include <linux/kmemleak.h>
>
>  #include <asm/early_ioremap.h>
>
> @@ -1026,6 +1027,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
>         if (!rsv)
>                 return -ENOMEM;
>
> +       kmemleak_ignore(rsv);
> +
>         rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
>         atomic_set(&rsv->count, 1);
>         rsv->entry[0].base = addr;

I was hoping we could merge this patch (so we can backport it), but
resolve the conflict by dropping the kmemleak_ignore() again, since it
will now complain since the kmalloc() has been replaced with a
__get_free_page() in the mean time (if that makes sense)

[GIT PULL] EFI fix

[Ingo Molnar]

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: b12f5440d8ca02e8f9ab4f1461f9214295cc4f66 Merge branch 'linus' into efi/urgent, to resolve conflict

A single fix that adds an annotation to resolve a kmemleak false 
positive.

 Thanks,

	Ingo

------------------>
Qian Cai (1):
      efi: Let kmemleak ignore false positives


 drivers/firmware/efi/efi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4c46ff6f2242..7ac09dd8f268 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -31,6 +31,7 @@
 #include <linux/acpi.h>
 #include <linux/ucs2_string.h>
 #include <linux/memblock.h>
+#include <linux/kmemleak.h>
 
 #include <asm/early_ioremap.h>
 
@@ -1026,6 +1027,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 	if (!rsv)
 		return -ENOMEM;
 
+	kmemleak_ignore(rsv);
+
 	rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
 	atomic_set(&rsv->count, 1);
 	rsv->entry[0].base = addr;

Re: [GIT PULL] EFI fix

[pr-tracker-bot]

The pull request you sent on Fri, 30 Nov 2018 07:21:02 +0100:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8d9f412d51b84eafd2253a82120e218ddc53e721

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

[GIT PULL] EFI fix

[Ingo Molnar]

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: 976b489120cdab2b1b3a41ffa14661db43d58190 efi: Prevent GICv3 WARN() by mapping the memreserve table before first use

An arm64 warning fix.

 Thanks,

	Ingo

------------------>
Ard Biesheuvel (1):
      efi: Prevent GICv3 WARN() by mapping the memreserve table before first use


 drivers/firmware/efi/efi.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index fad7c62cfc0e..415849bab233 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -969,13 +969,33 @@ bool efi_is_table_address(unsigned long phys_addr)
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
 static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
 
-int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
+static int __init efi_memreserve_map_root(void)
+{
+	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
+		return -ENODEV;
+
+	efi_memreserve_root = memremap(efi.mem_reserve,
+				       sizeof(*efi_memreserve_root),
+				       MEMREMAP_WB);
+	if (WARN_ON_ONCE(!efi_memreserve_root))
+		return -ENOMEM;
+	return 0;
+}
+
+int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
 	struct linux_efi_memreserve *rsv;
+	int rc;
 
-	if (!efi_memreserve_root)
+	if (efi_memreserve_root == (void *)ULONG_MAX)
 		return -ENODEV;
 
+	if (!efi_memreserve_root) {
+		rc = efi_memreserve_map_root();
+		if (rc)
+			return rc;
+	}
+
 	rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
 	if (!rsv)
 		return -ENOMEM;
@@ -993,14 +1013,10 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 
 static int __init efi_memreserve_root_init(void)
 {
-	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
-		return -ENODEV;
-
-	efi_memreserve_root = memremap(efi.mem_reserve,
-				       sizeof(*efi_memreserve_root),
-				       MEMREMAP_WB);
-	if (!efi_memreserve_root)
-		return -ENOMEM;
+	if (efi_memreserve_root)
+		return 0;
+	if (efi_memreserve_map_root())
+		efi_memreserve_root = (void *)ULONG_MAX;
 	return 0;
 }
 early_initcall(efi_memreserve_root_init);

[GIT PULL] EFI fix

[Ingo Molnar]

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: 9b788f32bee6b0b293a4bdfca4ad4bb0206407fb x86/efi: Access EFI MMIO data as unencrypted when SEV is active

An UEFI variables fix for SEV guests.

 Thanks,

	Ingo

------------------>
Brijesh Singh (1):
      x86/efi: Access EFI MMIO data as unencrypted when SEV is active


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

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 77873ce700ae..5f2eb3231607 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -417,7 +417,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va)
 	if (!(md->attribute & EFI_MEMORY_WB))
 		flags |= _PAGE_PCD;
 
-	if (sev_active())
+	if (sev_active() && md->type != EFI_MEMORY_MAPPED_IO)
 		flags |= _PAGE_ENC;
 
 	pfn = md->phys_addr >> PAGE_SHIFT;

[GIT PULL] EFI fix

[Ingo Molnar]

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: e296701800f30d260a66f8aa1971b5b1bc3d2f81 efi/x86: Fix mixed mode reboot loop by removing pointless call to PciIo->Attributes()

Fix a UEFI mixed mode (64-bit kernel on 32-bit UEFI) reboot loop regression.

 Thanks,

	Ingo

------------------>
Ard Biesheuvel (1):
      efi/x86: Fix mixed mode reboot loop by removing pointless call to PciIo->Attributes()


 arch/x86/boot/compressed/eboot.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index e57665b4ba1c..e98522ea6f09 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -114,18 +114,12 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
 	struct pci_setup_rom *rom = NULL;
 	efi_status_t status;
 	unsigned long size;
-	uint64_t attributes, romsize;
+	uint64_t romsize;
 	void *romimage;
 
-	status = efi_call_proto(efi_pci_io_protocol, attributes, pci,
-				EfiPciIoAttributeOperationGet, 0ULL,
-				&attributes);
-	if (status != EFI_SUCCESS)
-		return status;
-
 	/*
-	 * Some firmware images contain EFI function pointers at the place where the
-	 * romimage and romsize fields are supposed to be. Typically the EFI
+	 * Some firmware images contain EFI function pointers at the place where
+	 * the romimage and romsize fields are supposed to be. Typically the EFI
 	 * code is mapped at high addresses, translating to an unrealistically
 	 * large romsize. The UEFI spec limits the size of option ROMs to 16
 	 * MiB so we reject any ROMs over 16 MiB in size to catch this.

[GIT PULL] EFI fix

[Ingo Molnar]

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: 792ef14df5c585c19b2831673a077504a09e5203 efi: Fix boot panic because of invalid BGRT image address

A boot crash fix for certain systems where the kernel would trust a piece of 
firmware data it should not have.

 Thanks,

	Ingo

------------------>
Dave Young (1):
      efi: Fix boot panic because of invalid BGRT image address


 drivers/firmware/efi/efi-bgrt.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
index 8bf27323f7a3..b58233e4ed71 100644
--- a/drivers/firmware/efi/efi-bgrt.c
+++ b/drivers/firmware/efi/efi-bgrt.c
@@ -27,6 +27,26 @@ struct bmp_header {
 	u32 size;
 } __packed;
 
+static bool efi_bgrt_addr_valid(u64 addr)
+{
+	efi_memory_desc_t *md;
+
+	for_each_efi_memory_desc(md) {
+		u64 size;
+		u64 end;
+
+		if (md->type != EFI_BOOT_SERVICES_DATA)
+			continue;
+
+		size = md->num_pages << EFI_PAGE_SHIFT;
+		end = md->phys_addr + size;
+		if (addr >= md->phys_addr && addr < end)
+			return true;
+	}
+
+	return false;
+}
+
 void __init efi_bgrt_init(struct acpi_table_header *table)
 {
 	void *image;
@@ -36,7 +56,7 @@ void __init efi_bgrt_init(struct acpi_table_header *table)
 	if (acpi_disabled)
 		return;
 
-	if (!efi_enabled(EFI_BOOT))
+	if (!efi_enabled(EFI_MEMMAP))
 		return;
 
 	if (table->length < sizeof(bgrt_tab)) {
@@ -65,6 +85,10 @@ void __init efi_bgrt_init(struct acpi_table_header *table)
 		goto out;
 	}
 
+	if (!efi_bgrt_addr_valid(bgrt->image_address)) {
+		pr_notice("Ignoring BGRT: invalid image address\n");
+		goto out;
+	}
 	image = early_memremap(bgrt->image_address, sizeof(bmp_header));
 	if (!image) {
 		pr_notice("Ignoring BGRT: failed to map image header memory\n");

[GIT PULL] EFI fix

[Ingo Molnar]

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: ede85e90be26e5de2a72f76feec01cfc5281d4bd Merge tag 'efi-urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi into efi/urgent

This fixes a bug in the efivars code.

 Thanks,

	Ingo

------------------>
Laszlo Ersek (1):
      efi: Fix out-of-bounds read in variable_matches()


 drivers/firmware/efi/vars.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 0ac594c0a234..34b741940494 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -202,29 +202,44 @@ static const struct variable_validate variable_validate[] = {
 	{ NULL_GUID, "", NULL },
 };
 
+/*
+ * Check if @var_name matches the pattern given in @match_name.
+ *
+ * @var_name: an array of @len non-NUL characters.
+ * @match_name: a NUL-terminated pattern string, optionally ending in "*". A
+ *              final "*" character matches any trailing characters @var_name,
+ *              including the case when there are none left in @var_name.
+ * @match: on output, the number of non-wildcard characters in @match_name
+ *         that @var_name matches, regardless of the return value.
+ * @return: whether @var_name fully matches @match_name.
+ */
 static bool
 variable_matches(const char *var_name, size_t len, const char *match_name,
 		 int *match)
 {
 	for (*match = 0; ; (*match)++) {
 		char c = match_name[*match];
-		char u = var_name[*match];
 
-		/* Wildcard in the matching name means we've matched */
-		if (c == '*')
+		switch (c) {
+		case '*':
+			/* Wildcard in @match_name means we've matched. */
 			return true;
 
-		/* Case sensitive match */
-		if (!c && *match == len)
-			return true;
+		case '\0':
+			/* @match_name has ended. Has @var_name too? */
+			return (*match == len);
 
-		if (c != u)
+		default:
+			/*
+			 * We've reached a non-wildcard char in @match_name.
+			 * Continue only if there's an identical character in
+			 * @var_name.
+			 */
+			if (*match < len && c == var_name[*match])
+				continue;
 			return false;
-
-		if (!c)
-			return true;
+		}
 	}
-	return true;
 }
 
 bool

[GIT PULL] EFI fix

[Ingo Molnar]

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: f4d5b8adf3668a2fb69a411974057ec3b150b747 Merge tag 'efi-urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi into efi/urgent

which contains an arm64 boot crash fix.

 Thanks,

	Ingo

------------------>
Ard Biesheuvel (1):
      efi/arm64: Don't apply MEMBLOCK_NOMAP to UEFI memory map mapping

Ingo Molnar (1):
      Merge tag 'efi-urgent' of git://git.kernel.org/.../mfleming/efi into efi/urgent

 drivers/firmware/efi/arm-init.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)