Cannot load linux after recent efi-related changes

DISCUSSION Mike Krinkin 2 participants 5 messages Sep 17, 2016 – Sep 19, 2016

Subsystems (2)

x86 EFI

lore

Share & tools

lore

Mike Krinkin

author

2016-09-17 16:24 UTC #0 lore raw

Hello,

after commit 3dad6f7f6975 ("x86/efi: Defer efi_esrt_init until after memblock_x86_fill") kernel hits BUG_ON __efi_enter_virtual_mode because efi.systab is NULL. With older kernel versions i face the problem with efi_mem_reserve described in the commit.

AFAICS, get_systab_virt_addr called from efi_map_regions should set efi.systab, but i dumped memory desciptors in efi_map_regions and apparently none of them describes region that contains efi_phys.systab, so efi.systab remains unset.

Mike Krinkin

author

2016-09-18 01:14 UTC #1 lore raw

Quoted 11 lines · Mike Krinkin

Hello,
after commit 3dad6f7f6975 ("x86/efi: Defer efi_esrt_init until after memblock_x86_fill") kernel hits BUG_ON __efi_enter_virtual_mode because efi.systab is NULL. With older kernel versions i face the problem with efi_mem_reserve described in the commit.
AFAICS, get_systab_virt_addr called from efi_map_regions should set efi.systab, but i dumped memory desciptors in efi_map_regions and apparently none of them describes region that contains efi_phys.systab, so efi.systab remains unset.

I investigated it a bit further, and apparently problem occurs because efi_esrt_init calls efi_mem_reserve with unaligned range boundaries, and efi_memmap_insert doesn't handle unaligned ranges properly. The following fix solves problem for me:

Quoted 1 line

From 23f7134a6dd3a3c47f875395933a68e1a83d0f0e Mon Sep 17 00:00:00 2001

From: Mike Krinkin <[email protected]>
Date: Sun, 18 Sep 2016 03:53:52 +0300
Subject: [PATCH] efi: force page alignment in efi_mem_insert

efi_mem_insert might be called with unaligned range boundaries, for example, for me it happens because esrt size is not page aligned, that, in turn, results in wrong memory map and triggers BUG_ON in __efi_enter_virtual_mode.

Force page alignment on memory range boundaries in efi_mem_insert.

Signed-off-by Mike Krinkin <[email protected]>

---
 drivers/firmware/efi/memmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/firmware/efi/memmap.c +2 -2

index cd96086..34322d1 100644
@@ -221,8 +221,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	void *old, *new;
 
 	/* modifying range */
-	m_start = mem->range.start;
-	m_end = mem->range.end;
+	m_start = mem->range.start & ~(u64)EFI_PAGE_SIZE;
+	m_end = ALIGN(mem->range.end, EFI_PAGE_SIZE) - 1;
 	m_attr = mem->attribute;
 
 	for (old = old_memmap->map, new = buf;

Signature · 1 line

2.7.4

Matt Fleming

2016-09-19 11:14 UTC #2 lore raw

Quoted 6 lines · Mike Krinkin

drivers/firmware/efi/memmap.c +0 -0
index cd96086..34322d1 100644

drivers/firmware/efi/memmap.c void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf, · line 228

Quoted 10 lines

drivers/firmware/efi/memmap.c +2 -2

@@ -221,8 +221,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	void *old, *new;
 
 	/* modifying range */
-	m_start = mem->range.start;
-	m_end = mem->range.end;
+	m_start = mem->range.start & ~(u64)EFI_PAGE_SIZE;
+	m_end = ALIGN(mem->range.end, EFI_PAGE_SIZE) - 1;
 	m_attr = mem->attribute;
 
 	for (old = old_memmap->map, new = buf;

Thanks for the analysis and patch Mike, but this needs fixing further up the call stack so that we don't map things the caller didn't expect.

This bug was also reported in this thread,

https://lkml.kernel.org/r/[email protected]

Could you try this patch?

---->8----

Quoted 1 line

From 7e750e3289a44fe3ad693bde45aea1ad8577dd2a Mon Sep 17 00:00:00 2001

From: Matt Fleming <[email protected]>
Date: Fri, 16 Sep 2016 15:12:47 +0100
Subject: [PATCH] x86/efi: Round EFI memmap reservations to EFI_PAGE_SIZE

Mike Galbraith reported that his machine started rebooting during boot after,

commit 8e80632fb23f ("efi/esrt: Use efi_mem_reserve() and avoid a kmalloc()")

The ESRT table on his machine is 56 bytes and at no point in the efi_arch_mem_reserve() call path is that size rounded up to EFI_PAGE_SIZE, nor is the start address on an EFI_PAGE_SIZE boundary.

Since the EFI memory map only deals with whole pages, inserting an EFI memory region with 56 bytes results in a new entry covering zero pages, and completely screws up the calculations for the old regions that were trimmed.

Round all sizes upwards, and start addresses downwards, to the nearest EFI_PAGE_SIZE boundary.

Additionally, efi_memmap_insert() expects the mem::range::end value to be one less than the end address for the region.

Reported-by Mike Galbraith <[email protected]>

Reported-by Mike Krinkin <[email protected]>

Cc Peter Jones <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Taku Izumi <[email protected]>

Signed-off-by Matt Fleming <[email protected]>

---
 arch/x86/platform/efi/quirks.c |  6 +++++-
 drivers/firmware/efi/memmap.c  | 11 +++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

arch/x86/platform/efi/quirks.c +5 -1

index f14b7a9da24b..10aca63a50d7 100644
@@ -201,8 +201,12 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 		return;
 	}
 
+	size += addr % EFI_PAGE_SIZE;
+	size = round_up(size, EFI_PAGE_SIZE);
+	addr = round_down(addr, EFI_PAGE_SIZE);
+
 	mr.range.start = addr;
-	mr.range.end = addr + size;
+	mr.range.end = addr + size - 1;
 	mr.attribute = md.attribute | EFI_MEMORY_RUNTIME;
 
 	num_entries = efi_memmap_split_count(&md, &mr.range);

drivers/firmware/efi/memmap.c +11 -0

index cd96086fd851..f03ddecd232b 100644
@@ -225,6 +225,17 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	m_end = mem->range.end;
 	m_attr = mem->attribute;
 
+	/*
+	 * The EFI memory map deals with regions in EFI_PAGE_SIZE
+	 * units. Ensure that the region described by 'mem' is aligned
+	 * correctly.
+	 */
+	if (!IS_ALIGNED(m_start, EFI_PAGE_SIZE) ||
+	    !IS_ALIGNED(m_end + 1, EFI_PAGE_SIZE)) {
+		WARN_ON(1);
+		return;
+	}
+
 	for (old = old_memmap->map, new = buf;
 	     old < old_memmap->map_end;
 	     old += old_memmap->desc_size, new += old_memmap->desc_size) {

Signature · 1 line

2.9.3

Mike Krinkin

author

2016-09-19 11:36 UTC #3 lore raw

Quoted 22 lines · Matt Fleming

Quoted 6 lines · Mike Krinkin
drivers/firmware/efi/memmap.c +0 -0
index cd96086..34322d1 100644
drivers/firmware/efi/memmap.c void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf, · line 228
Quoted 10 lines
drivers/firmware/efi/memmap.c +2 -2
@@ -221,8 +221,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	void *old, *new;
 
 	/* modifying range */
-	m_start = mem->range.start;
-	m_end = mem->range.end;
+	m_start = mem->range.start & ~(u64)EFI_PAGE_SIZE;
+	m_end = ALIGN(mem->range.end, EFI_PAGE_SIZE) - 1;
 	m_attr = mem->attribute;
 
 	for (old = old_memmap->map, new = buf;
Thanks for the analysis and patch Mike, but this needs fixing further up the call stack so that we don't map things the caller didn't expect.

Quoted 3 lines

This bug was also reported in this thread,
https://lkml.kernel.org/r/[email protected]

Sorry, i haven't seen that.

Quoted 2 lines

Could you try this patch?

Works fine for me.

Quoted 83 lines

---->8----
From 7e750e3289a44fe3ad693bde45aea1ad8577dd2a Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/[email protected]>
Date: Fri, 16 Sep 2016 15:12:47 +0100
Subject: [PATCH] x86/efi: Round EFI memmap reservations to EFI_PAGE_SIZE
Mike Galbraith reported that his machine started rebooting during boot after,
commit 8e80632fb23f ("efi/esrt: Use efi_mem_reserve() and avoid a kmalloc()")
The ESRT table on his machine is 56 bytes and at no point in the efi_arch_mem_reserve() call path is that size rounded up to EFI_PAGE_SIZE, nor is the start address on an EFI_PAGE_SIZE boundary.
Since the EFI memory map only deals with whole pages, inserting an EFI memory region with 56 bytes results in a new entry covering zero pages, and completely screws up the calculations for the old regions that were trimmed.
Round all sizes upwards, and start addresses downwards, to the nearest EFI_PAGE_SIZE boundary.
Additionally, efi_memmap_insert() expects the mem::range::end value to be one less than the end address for the region.
Reported-by Mike Galbraith <[email protected]>
Reported-by Mike Krinkin <[email protected]>
Cc Peter Jones <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Taku Izumi <[email protected]>
Signed-off-by Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/[email protected]>
---
 arch/x86/platform/efi/quirks.c |  6 +++++-
 drivers/firmware/efi/memmap.c  | 11 +++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
arch/x86/platform/efi/quirks.c +5 -1
index f14b7a9da24b..10aca63a50d7 100644
@@ -201,8 +201,12 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 		return;
 	}
 
+	size += addr % EFI_PAGE_SIZE;
+	size = round_up(size, EFI_PAGE_SIZE);
+	addr = round_down(addr, EFI_PAGE_SIZE);
+
 	mr.range.start = addr;
-	mr.range.end = addr + size;
+	mr.range.end = addr + size - 1;
 	mr.attribute = md.attribute | EFI_MEMORY_RUNTIME;
 
 	num_entries = efi_memmap_split_count(&md, &mr.range);
drivers/firmware/efi/memmap.c +11 -0
index cd96086fd851..f03ddecd232b 100644
@@ -225,6 +225,17 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	m_end = mem->range.end;
 	m_attr = mem->attribute;
 
+	/*
+	 * The EFI memory map deals with regions in EFI_PAGE_SIZE
+	 * units. Ensure that the region described by 'mem' is aligned
+	 * correctly.
+	 */
+	if (!IS_ALIGNED(m_start, EFI_PAGE_SIZE) ||
+	    !IS_ALIGNED(m_end + 1, EFI_PAGE_SIZE)) {
+		WARN_ON(1);
+		return;
+	}
+
 	for (old = old_memmap->map, new = buf;
 	     old < old_memmap->map_end;
 	     old += old_memmap->desc_size, new += old_memmap->desc_size) {
Signature · 1 line
2.9.3

Matt Fleming

2016-09-19 11:41 UTC #4 lore raw

Quoted 23 lines · Mike Krinkin

Quoted 22 lines · Matt Fleming
Quoted 6 lines · Mike Krinkin
drivers/firmware/efi/memmap.c +0 -0
index cd96086..34322d1 100644
drivers/firmware/efi/memmap.c void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf, · line 228
Quoted 10 lines
drivers/firmware/efi/memmap.c +2 -2
@@ -221,8 +221,8 @@ void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
 	void *old, *new;
 
 	/* modifying range */
-	m_start = mem->range.start;
-	m_end = mem->range.end;
+	m_start = mem->range.start & ~(u64)EFI_PAGE_SIZE;
+	m_end = ALIGN(mem->range.end, EFI_PAGE_SIZE) - 1;
 	m_attr = mem->attribute;
 
 	for (old = old_memmap->map, new = buf;
Thanks for the analysis and patch Mike, but this needs fixing further up the call stack so that we don't map things the caller didn't expect.

Quoted 5 lines

Quoted 3 lines
This bug was also reported in this thread,
https://lkml.kernel.org/r/[email protected]
Sorry, i haven't seen that.

No problem.

Quoted 4 lines

Quoted 2 lines
Could you try this patch?
Works fine for me.

Thanks, I've added your Tested-by.