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

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

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;

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

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

[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

[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

[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

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

[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

[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

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

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

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

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