[PATCH 0/6] Warn about illegal accesses to EFI_BOOT_SERVICES_* memory

[PATCH 0/6] Warn about illegal accesses to EFI_BOOT_SERVICES_* memory

[Ricardo Neri]

The UEFI specification states that the firmware shall not access the
BOOT_SERVICES_DATA/CODE * memory regions after the operating system has
called ExitBootServices on it. Thus, the operating system is free to use
such regions as it sees fit. Still, buggy UEFI firmware implementations
may want to keep accessing these regions.

The current approach of the kernel is to reserve and map the
EFI_BOOT_SERVICES_* regions until efi_free_boot_services() is called
(after calling SetVirtualAddressMap() on the firmware). Further details
are show in the commit 916f676f8dc0 ("x86, efi: Retain boot service code
until after switching to virtual mode") by Matthew Garrett.

A drawback of the current approach is that silently working around this
kind of illegal accesses encourages the perpetuation of these bugs in
UEFI firmware implementations. Rather, this set of patches proposes a
more verbose behavior: continue reserving the EFI_BOOT_SERVICES_* regions
but not map them. If they are not mapped, any access will cause a page
fault that we can catch. Once the fault is catched, the kernel will fix
it up (i.e., map the page for the firmware to use it) and, more important,
complain about it.

We are guaranteed to not have false positives (i.e., page faults caused
by bad kernel code) as these memory regions are still reserved.

Besides fixing up the illegal accesses, no further action is required to
update the memory map the firmware sees. This is true because after boot,
the firmware would require access to the runtime services memory only,
which should be mapped before calling SetVirtualAddressMap. Furthermore,
a second attempt to update the virtual address map will result in a
EFI_UNSUPPORTED from the firmware, as per the UEFI specification.

Also, there is no need to update the system table as it should have been
when mapping the rest of the memory regions.

Finally, kexec is concerned only about the runtime services memory
sections. Thus we don't need any special arrangements for kexec.

The four last patches of the set implement this approach. The first two
provide a rework for code reuse of the convenience functions that look
for the descriptor of a physical memory address when then be used by the
proposed solution.

Ricardo Neri (6):
  x86/efi: Add function to obtain mem descriptor from phys address
  x86/efi: Use efi_memory_descriptor in mem convenience functions
  x86/efi: Add function to fixup page faults in BOOT_SERVICES_* regions
  x86/efi: Remove __init attribute from memory mapping functions
  yx86/efi: Fixup faults from UEFI firmware
  x86/efi: Introduce EFI_BOOT_SERVICES_WARN

 arch/x86/Kconfig               | 12 ++++++++
 arch/x86/include/asm/efi.h     |  4 +--
 arch/x86/mm/fault.c            |  8 +++++
 arch/x86/platform/efi/efi.c    | 66 ++++++++++++++++++++++++++++++++----------
 arch/x86/platform/efi/efi_32.c |  2 +-
 arch/x86/platform/efi/efi_64.c |  8 ++---
 drivers/firmware/efi/efi.c     | 36 ++++++++++-------------
 include/linux/efi.h            |  9 ++++++
 8 files changed, 101 insertions(+), 44 deletions(-)

-- 
1.9.1

[PATCH 0/6] Warn about illegal accesses to EFI_BOOT_SERVICES_* memory

[Ricardo Neri]

The UEFI specification states that the firmware shall not access the
BOOT_SERVICES_DATA/CODE * memory regions after the operating system has
called ExitBootServices on it. Thus, the operating system is free to use
such regions as it sees fit. Still, buggy UEFI firmware implementations
may want to keep accessing these regions.

The current approach of the kernel is to reserve and map the
EFI_BOOT_SERVICES_* regions until efi_free_boot_services() is called
(after calling SetVirtualAddressMap() on the firmware). Further details
are show in the commit 916f676f8dc0 ("x86, efi: Retain boot service code
until after switching to virtual mode") by Matthew Garrett.

A drawback of the current approach is that silently working around this
kind of illegal accesses encourages the perpetuation of these bugs in
UEFI firmware implementations. Rather, this set of patches proposes a
more verbose behavior: continue reserving the EFI_BOOT_SERVICES_* regions
but not map them. If they are not mapped, any access will cause a page
fault that we can catch. Once the fault is catched, the kernel will fix
it up (i.e., map the page for the firmware to use it) and, more important,
complain about it.

We are guaranteed to not have false positives (i.e., page faults caused
by bad kernel code) as these memory regions are still reserved.

Besides fixing up the illegal accesses, no further action is required to
update the memory map the firmware sees. This is true because after boot,
the firmware would require access to the runtime services memory only,
which should be mapped before calling SetVirtualAddressMap. Furthermore,
a second attempt to update the virtual address map will result in a
EFI_UNSUPPORTED from the firmware, as per the UEFI specification.

Also, there is no need to update the system table as it should have been
when mapping the rest of the memory regions.

Finally, kexec is concerned only about the runtime services memory
sections. Thus we don't need any special arrangements for kexec.

The four last patches of the set implement this approach. The first two
provide a rework for code reuse of the convenience functions that look
for the descriptor of a physical memory address when then be used by the
proposed solution.

Ricardo Neri (6):
  x86/efi: Add function to obtain mem descriptor from phys address
  x86/efi: Use efi_memory_descriptor in mem convenience functions
  x86/efi: Add function to fixup page faults in BOOT_SERVICES_* regions
  x86/efi: Remove __init attribute from memory mapping functions
  yx86/efi: Fixup faults from UEFI firmware
  x86/efi: Introduce EFI_BOOT_SERVICES_WARN

 arch/x86/Kconfig               | 12 ++++++++
 arch/x86/include/asm/efi.h     |  4 +--
 arch/x86/mm/fault.c            |  8 +++++
 arch/x86/platform/efi/efi.c    | 66 ++++++++++++++++++++++++++++++++----------
 arch/x86/platform/efi/efi_32.c |  2 +-
 arch/x86/platform/efi/efi_64.c |  8 ++---
 drivers/firmware/efi/efi.c     | 36 ++++++++++-------------
 include/linux/efi.h            |  9 ++++++
 8 files changed, 101 insertions(+), 44 deletions(-)

-- 
1.9.1

[PATCH 1/6] x86/efi: find mem descriptor from phys address

[Ricardo Neri]

Several functions (efi_mem_type, efi_mem_attributes,
efi_lookup_mapped_addr) scan the memory map looking for the memory
descriptor to which a given physical address belongs for various
purposes.

The scan functionality is duplicated in all the three functions. The
common functionality is consolidated into a single function that three
functions mentioned above may call.

When checking the validity of the validity of the memory map, use
efi_enabled(), provided for this purpose.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 21 ++++++++++++++++++++-
 include/linux/efi.h         |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 850da94..782d617 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -900,8 +900,27 @@ void __init efi_enter_virtual_mode(void)
 }
 
 /*
- * Convenience functions to obtain memory types and attributes
+ * Convenience functions to obtain memory descriptors,
+ * memory types and attributes
  */
+efi_memory_desc_t *efi_memory_descriptor(unsigned long phys_addr)
+{
+	efi_memory_desc_t *md;
+	void *p;
+
+	if (!efi_enabled(EFI_MEMMAP))
+		return NULL;
+
+	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+		md = p;
+		if ((md->phys_addr <= phys_addr) &&
+		    (phys_addr < (md->phys_addr +
+				  (md->num_pages << EFI_PAGE_SHIFT))))
+			return md;
+	}
+	return NULL;
+}
+
 u32 efi_mem_type(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 45cb4ff..b36b588 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -866,6 +866,7 @@ static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned lon
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
+extern efi_memory_desc_t *efi_memory_descriptor(unsigned long phys_addr);
 extern u32 efi_mem_type (unsigned long phys_addr);
 extern u64 efi_mem_attributes (unsigned long phys_addr);
 extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
-- 
1.9.1

[PATCH 2/6] x86/efi: use efi_memory_descriptor in convenience functions

[Ricardo Neri]

Rather than duplicating the code to lookup for the memory descriptor of
a given physical address, use the utility function efi_memory_descriptor.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 26 ++++++--------------------
 drivers/firmware/efi/efi.c  | 36 +++++++++++++++---------------------
 2 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 782d617..d45decf 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -924,36 +924,22 @@ efi_memory_desc_t *efi_memory_descriptor(unsigned long phys_addr)
 u32 efi_mem_type(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
-	void *p;
 
-	if (!efi_enabled(EFI_MEMMAP))
-		return 0;
+	md = efi_memory_descriptor(phys_addr);
+	if (md)
+		return md->type;
 
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
-		md = p;
-		if ((md->phys_addr <= phys_addr) &&
-		    (phys_addr < (md->phys_addr +
-				  (md->num_pages << EFI_PAGE_SHIFT))))
-			return md->type;
-	}
 	return 0;
 }
 
 u64 efi_mem_attributes(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
-	void *p;
 
-	if (!efi_enabled(EFI_MEMMAP))
-		return 0;
+	md = efi_memory_descriptor(phys_addr);
+	if (md)
+		return md->attribute;
 
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
-		md = p;
-		if ((md->phys_addr <= phys_addr) &&
-		    (phys_addr < (md->phys_addr +
-				  (md->num_pages << EFI_PAGE_SHIFT))))
-			return md->attribute;
-	}
 	return 0;
 }
 
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 64ecbb5..29c85fe 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -206,29 +206,23 @@ subsys_initcall(efisubsys_init);
  */
 void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
 {
-	struct efi_memory_map *map;
-	void *p;
-	map = efi.memmap;
-	if (!map)
+	efi_memory_desc_t *md;
+
+	md = efi_memory_descriptor(phys_addr);
+
+	if (!md)
 		return NULL;
-	if (WARN_ON(!map->map))
+
+	if (!md->virt_addr)
 		return NULL;
-	for (p = map->map; p < map->map_end; p += map->desc_size) {
-		efi_memory_desc_t *md = p;
-		u64 size = md->num_pages << EFI_PAGE_SHIFT;
-		u64 end = md->phys_addr + size;
-		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
-		    md->type != EFI_BOOT_SERVICES_CODE &&
-		    md->type != EFI_BOOT_SERVICES_DATA)
-			continue;
-		if (!md->virt_addr)
-			continue;
-		if (phys_addr >= md->phys_addr && phys_addr < end) {
-			phys_addr += md->virt_addr - md->phys_addr;
-			return (__force void __iomem *)(unsigned long)phys_addr;
-		}
-	}
-	return NULL;
+
+	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
+	    md->type != EFI_BOOT_SERVICES_CODE &&
+	    md->type != EFI_BOOT_SERVICES_DATA)
+		return NULL;
+
+	phys_addr += md->virt_addr - md->phys_addr;
+	return (__force void __iomem *)(unsigned long)phys_addr;
 }
 
 static __initdata efi_config_table_type_t common_tables[] = {
-- 
1.9.1

[PATCH 2/6] x86/efi: use efi_memory_descriptor in convenience functions

[Ricardo Neri]

Rather than duplicating the code to lookup for the memory descriptor of
a given physical address, use the utility function efi_memory_descriptor.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 26 ++++++--------------------
 drivers/firmware/efi/efi.c  | 36 +++++++++++++++---------------------
 2 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 782d617..d45decf 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -924,36 +924,22 @@ efi_memory_desc_t *efi_memory_descriptor(unsigned long phys_addr)
 u32 efi_mem_type(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
-	void *p;
 
-	if (!efi_enabled(EFI_MEMMAP))
-		return 0;
+	md = efi_memory_descriptor(phys_addr);
+	if (md)
+		return md->type;
 
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
-		md = p;
-		if ((md->phys_addr <= phys_addr) &&
-		    (phys_addr < (md->phys_addr +
-				  (md->num_pages << EFI_PAGE_SHIFT))))
-			return md->type;
-	}
 	return 0;
 }
 
 u64 efi_mem_attributes(unsigned long phys_addr)
 {
 	efi_memory_desc_t *md;
-	void *p;
 
-	if (!efi_enabled(EFI_MEMMAP))
-		return 0;
+	md = efi_memory_descriptor(phys_addr);
+	if (md)
+		return md->attribute;
 
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
-		md = p;
-		if ((md->phys_addr <= phys_addr) &&
-		    (phys_addr < (md->phys_addr +
-				  (md->num_pages << EFI_PAGE_SHIFT))))
-			return md->attribute;
-	}
 	return 0;
 }
 
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 64ecbb5..29c85fe 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -206,29 +206,23 @@ subsys_initcall(efisubsys_init);
  */
 void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
 {
-	struct efi_memory_map *map;
-	void *p;
-	map = efi.memmap;
-	if (!map)
+	efi_memory_desc_t *md;
+
+	md = efi_memory_descriptor(phys_addr);
+
+	if (!md)
 		return NULL;
-	if (WARN_ON(!map->map))
+
+	if (!md->virt_addr)
 		return NULL;
-	for (p = map->map; p < map->map_end; p += map->desc_size) {
-		efi_memory_desc_t *md = p;
-		u64 size = md->num_pages << EFI_PAGE_SHIFT;
-		u64 end = md->phys_addr + size;
-		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
-		    md->type != EFI_BOOT_SERVICES_CODE &&
-		    md->type != EFI_BOOT_SERVICES_DATA)
-			continue;
-		if (!md->virt_addr)
-			continue;
-		if (phys_addr >= md->phys_addr && phys_addr < end) {
-			phys_addr += md->virt_addr - md->phys_addr;
-			return (__force void __iomem *)(unsigned long)phys_addr;
-		}
-	}
-	return NULL;
+
+	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
+	    md->type != EFI_BOOT_SERVICES_CODE &&
+	    md->type != EFI_BOOT_SERVICES_DATA)
+		return NULL;
+
+	phys_addr += md->virt_addr - md->phys_addr;
+	return (__force void __iomem *)(unsigned long)phys_addr;
 }
 
 static __initdata efi_config_table_type_t common_tables[] = {
-- 
1.9.1

[PATCH 3/6] x86/efi: add code to fixup page faults in BOOT_SERVICES_* regions

[Ricardo Neri]

As per the UEFI specification, accesses to BOOT_SERVICES_* memory
regions by the UEFI firmware are illegal after the OS has called
ExitBootServices. However, buggy firmware implementations may still
access these regions after such call.

The current approach of the kernel is to reserve and map all the
EFI_BOOT_SERVICES_* memory regions until efi_free_boot_services() is
called so that the buggy firmware can freely access such regions.

A good way to detect these illegal accesses is to not map (but only
reserve) these regions so that the accesses generate a page fault that
the kernel can detect. Upon detection, the fault is fixed-up (i.e., the
region is mapped for the buggy firmware to use). As the pages are
reserved, the fixup is safe.

Thus, rather than just silently allowing the buggy firmware to proceed,
we detect the access and complain about it. Of course, this function
will be called by the page fault handler fixup code in a subsequent
patch.

Ideally, the new memory map with the just mapped region should be passed
to the firmware. However, as per the UEFI specification,
SetVirtualAddressMap may be called only once. Subsequent calls will
return EFI_UNSUPPORTED. Thus, it is pointless to pass the new map.
Furthermore, all the EFI_RUNTIME_SERVICES_* should already be mapped at
this point.

Also, at this point, the virtual address of the system table should have
been found. Thus, there is no need to look for it in the just-mapped
region.

Finally, this new mapping will not impact a reboot from kexec, as kexec
is only concerned about runtime memory regions.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 29 +++++++++++++++++++++++++++++
 include/linux/efi.h         |  8 ++++++++
 2 files changed, 37 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index d45decf..2e78083 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -954,3 +954,32 @@ static int __init parse_efi_cmdline(char *str)
 	return 0;
 }
 early_param("efi", parse_efi_cmdline);
+
+#ifdef CONFIG_EFI_BOOT_SERVICES_WARN
+static const char boot_services_warning[] =
+"Fixing illegal access to BOOT_SERVICES_*. Bug in EFI firmware?\n";
+
+int efi_boot_services_fixup(unsigned long phys_addr)
+{
+	efi_memory_desc_t *md;
+
+	md = efi_memory_descriptor(phys_addr);
+
+	if (!md)
+		return 0;
+
+	if (md->type == EFI_BOOT_SERVICES_CODE ||
+	    md->type == EFI_BOOT_SERVICES_DATA)	{
+		/*
+		 * If the page fault was caused by an acccess to BOOT_SERVICES_*
+		 * memory regions, just map the region... and warn about it.
+		 * By now we should have found the virtual address of the system
+		 * table. Thus, no need to update.
+		 */
+		pr_warn_once("%s", (char *)&boot_services_warning);
+		efi_map_region(md);
+		return 1;
+	}
+	return 0;
+}
+#endif
diff --git a/include/linux/efi.h b/include/linux/efi.h
index b36b588..fbdc412 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -875,6 +875,14 @@ extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
 extern void efi_get_time(struct timespec *now);
 extern void efi_reserve_boot_services(void);
+#ifdef CONFIG_EFI_BOOT_SERVICES_WARN
+extern int efi_boot_services_fixup(unsigned long phys_addr);
+#else
+static inline int efi_boot_services_fixup(unsigned long phys_addr)
+{
+		return 0;
+}
+#endif
 extern int efi_get_fdt_params(struct efi_fdt_params *params, int verbose);
 extern struct efi_memory_map memmap;
 
-- 
1.9.1

[PATCH 4/6] x86/efi: remove __init attribute from memory mapping functions

[Ricardo Neri]

Even though these functions are called at kernel boot, they will
also be used by the page fault handler to fixup access to BOOT_SERVICES_*
regions, which do not have the __init attribute.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/include/asm/efi.h     | 4 ++--
 arch/x86/platform/efi/efi.c    | 2 +-
 arch/x86/platform/efi/efi_32.c | 2 +-
 arch/x86/platform/efi/efi_64.c | 8 ++++----
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 044a2fd..d427a35 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -94,12 +94,12 @@ extern void efi_call_phys_prelog(void);
 extern void efi_call_phys_epilog(void);
 extern void efi_unmap_memmap(void);
 extern void efi_memory_uc(u64 addr, unsigned long size);
-extern void __init efi_map_region(efi_memory_desc_t *md);
+extern void efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);
 extern void efi_sync_low_kernel_mappings(void);
 extern int efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages);
 extern void efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages);
-extern void __init old_map_region(efi_memory_desc_t *md);
+extern void old_map_region(efi_memory_desc_t *md);
 extern void __init runtime_code_page_mkexec(void);
 extern void __init efi_runtime_mkexec(void);
 extern void __init efi_dump_pagetable(void);
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 2e78083..fd52004 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -547,7 +547,7 @@ void efi_memory_uc(u64 addr, unsigned long size)
 	set_memory_uc(addr, npages);
 }
 
-void __init old_map_region(efi_memory_desc_t *md)
+void old_map_region(efi_memory_desc_t *md)
 {
 	u64 start_pfn, end_pfn, end;
 	unsigned long size;
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index 9ee3491..766dcaf 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -47,7 +47,7 @@ int efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 }
 void efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages) {}
 
-void __init efi_map_region(efi_memory_desc_t *md)
+void efi_map_region(efi_memory_desc_t *md)
 {
 	old_map_region(md);
 }
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 290d397..32434ed 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -199,7 +199,7 @@ void efi_cleanup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	kernel_unmap_pages_in_pgd(pgd, pa_memmap, num_pages);
 }
 
-static void __init __map_region(efi_memory_desc_t *md, u64 va)
+static void  __map_region(efi_memory_desc_t *md, u64 va)
 {
 	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
 	unsigned long pf = 0;
@@ -212,7 +212,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va)
 			   md->phys_addr, va);
 }
 
-void __init efi_map_region(efi_memory_desc_t *md)
+void efi_map_region(efi_memory_desc_t *md)
 {
 	unsigned long size = md->num_pages << PAGE_SHIFT;
 	u64 pa = md->phys_addr;
@@ -273,8 +273,8 @@ void __init efi_map_region_fixed(efi_memory_desc_t *md)
 	__map_region(md, md->virt_addr);
 }
 
-void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
-				 u32 type, u64 attribute)
+void __iomem *efi_ioremap(unsigned long phys_addr, unsigned long size,
+			  u32 type, u64 attribute)
 {
 	unsigned long last_map_pfn;
 
-- 
1.9.1

[PATCH 5/6] yx86/efi: fixup faults from UEFI firmware

[Ricardo Neri]

Buggy UEFI firmware implementations may try to access the
EFI_BOOT_SERVICES_* memory regions even after those regions have been
surrendered to the kernel (after calling ExitBootServices() on the
firmware). If such regions are not mapped, a page fault will be
generated. Fix that up.

We are sure that we will not have false positives as those memory regions
are reserved and the kernel cannot use them.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/mm/fault.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index a241946..f084912 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -14,6 +14,7 @@
 #include <linux/hugetlb.h>		/* hstate_index_to_shift	*/
 #include <linux/prefetch.h>		/* prefetchw			*/
 #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
+#include <linux/efi.h>			/* fixup for buggy UEFI firmware*/
 
 #include <asm/traps.h>			/* dotraplinkage, ...		*/
 #include <asm/pgalloc.h>		/* pgd_*(), ...			*/
@@ -702,6 +703,13 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 		return;
 
 	/*
+	 * Try to fixup faults caused by illegal access to BOOT_SERVICES_*
+	 * regions by UEFI firmware.
+	 */
+	if (efi_boot_services_fixup(address))
+		return;
+
+	/*
 	 * Oops. The kernel tried to access some bad page. We'll have to
 	 * terminate things with extreme prejudice:
 	 */
-- 
1.9.1

[PATCH 6/6] x86/efi: introduce EFI_BOOT_SERVICES_WARN

[Ricardo Neri]

There may exist buggy implementations of UEFI firmaware that may still
try to access the EFI_BOOT_SERVICES_* memory regions after the call to
ExitBootServices() has been made. This is a violation of the UEFI
specification.

If selected, this debug option will print a warning message if the
conditions mentioned above are met. Along with the warning, the EFI
platform code will fix up the page fault so that the firmware can
proceed further. We are sure that the page fault will be caused by the
firmware trying to access an unmapped page as the kernel has reserved
such pages.

If not selected, EFI_BOOT_SERVICES_CODE/DATA memory regions will be
reserved and mapped along with the runtime memory regions so that the
buggy firmware does not cause any page faults when trying to accessing
such memory regions. This is the approach from Matthew Garrett in commit
916f676f8dc0 ("x86, efi: Retain boot service code until after switching
to virtual mode").

Being more verbose about this kind of illegal access from the firmware
increases the likelihood of this kind firmware bugs to be fixed.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
 arch/x86/Kconfig            | 12 ++++++++++++
 arch/x86/platform/efi/efi.c |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 778178f..d1c958a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1565,6 +1565,18 @@ config EFI_MIXED
 
 	   If unsure, say N.
 
+config EFI_BOOT_SERVICES_WARN
+	bool "Warn about illegal accesses to BOOT_SERVICES memory"
+	depends on EFI
+	---help---
+	   Enable this debug feature to make the kernel issue a warning if
+	   memory regions marked as EFI_BOOT_SERVICES_CODE/DATA are
+	   accessed after the kernel calls ExitBootServices() on the
+	   firmware. Please see the UEFI specification for details on
+	   the expectations of memory usage.
+
+	   If unsure, say N.
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index fd52004..c67637b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -689,7 +689,7 @@ static void * __init efi_map_regions(int *count, int *pg_shift)
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
 		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
-#ifdef CONFIG_X86_64
+#if defined(CONFIG_X86_64) && !defined(CONFIG_EFI_BOOT_SERVICES_WARN)
 			if (md->type != EFI_BOOT_SERVICES_CODE &&
 			    md->type != EFI_BOOT_SERVICES_DATA)
 #endif
-- 
1.9.1

[PATCH 6/6] x86/efi: introduce EFI_BOOT_SERVICES_WARN

[Ricardo Neri]

There may exist buggy implementations of UEFI firmaware that may still
try to access the EFI_BOOT_SERVICES_* memory regions after the call to
ExitBootServices() has been made. This is a violation of the UEFI
specification.

If selected, this debug option will print a warning message if the
conditions mentioned above are met. Along with the warning, the EFI
platform code will fix up the page fault so that the firmware can
proceed further. We are sure that the page fault will be caused by the
firmware trying to access an unmapped page as the kernel has reserved
such pages.

If not selected, EFI_BOOT_SERVICES_CODE/DATA memory regions will be
reserved and mapped along with the runtime memory regions so that the
buggy firmware does not cause any page faults when trying to accessing
such memory regions. This is the approach from Matthew Garrett in commit
916f676f8dc0 ("x86, efi: Retain boot service code until after switching
to virtual mode").

Being more verbose about this kind of illegal access from the firmware
increases the likelihood of this kind firmware bugs to be fixed.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 arch/x86/Kconfig            | 12 ++++++++++++
 arch/x86/platform/efi/efi.c |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 778178f..d1c958a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1565,6 +1565,18 @@ config EFI_MIXED
 
 	   If unsure, say N.
 
+config EFI_BOOT_SERVICES_WARN
+	bool "Warn about illegal accesses to BOOT_SERVICES memory"
+	depends on EFI
+	---help---
+	   Enable this debug feature to make the kernel issue a warning if
+	   memory regions marked as EFI_BOOT_SERVICES_CODE/DATA are
+	   accessed after the kernel calls ExitBootServices() on the
+	   firmware. Please see the UEFI specification for details on
+	   the expectations of memory usage.
+
+	   If unsure, say N.
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index fd52004..c67637b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -689,7 +689,7 @@ static void * __init efi_map_regions(int *count, int *pg_shift)
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
 		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
-#ifdef CONFIG_X86_64
+#if defined(CONFIG_X86_64) && !defined(CONFIG_EFI_BOOT_SERVICES_WARN)
 			if (md->type != EFI_BOOT_SERVICES_CODE &&
 			    md->type != EFI_BOOT_SERVICES_DATA)
 #endif
-- 
1.9.1

Re: [PATCH 6/6] x86/efi: introduce EFI_BOOT_SERVICES_WARN

[Borislav Petkov]

On Sat, Sep 13, 2014 at 11:36:16AM -0700, Ricardo Neri wrote:
> Being more verbose about this kind of illegal access from the firmware
> increases the likelihood of this kind firmware bugs to be fixed.

I sincerely hope you're right and, more importantly, how do we make sure
those warnings get seen in time for a fix to even be possible..?

-- 
Regards/Gruss,
    Boris.
--

Re: [PATCH 6/6] x86/efi: introduce EFI_BOOT_SERVICES_WARN

[Borislav Petkov]

On Sat, Sep 13, 2014 at 11:36:16AM -0700, Ricardo Neri wrote:
> Being more verbose about this kind of illegal access from the firmware
> increases the likelihood of this kind firmware bugs to be fixed.

I sincerely hope you're right and, more importantly, how do we make sure
those warnings get seen in time for a fix to even be possible..?

-- 
Regards/Gruss,
    Boris.
--

Re: [PATCH 6/6] x86/efi: introduce EFI_BOOT_SERVICES_WARN

[Matt Fleming]

On Sun, 14 Sep, at 02:01:30AM, Borislav Petkov wrote:
> On Sat, Sep 13, 2014 at 11:36:16AM -0700, Ricardo Neri wrote:
> > Being more verbose about this kind of illegal access from the firmware
> > increases the likelihood of this kind firmware bugs to be fixed.
> 
> I sincerely hope you're right and, more importantly, how do we make sure
> those warnings get seen in time for a fix to even be possible..?

Some firmware teams do run Linux as part of their validation process,
and have been known to pay attention to the kernel boot messages. So
there's definitely hope there.

But we are also taking a more active approach with the Linux UEFI
Validation project [1], where we consume these kinds of error messages
and turn them into explicit test passes/failures.

We've been attending the UEFI plugfests and trying to work directly with
firmware engineers to bridge that communication gap between firmware and
OS, so that we can fix these kinds of bugs before they appear in the
wild.

[1] - https://01.org/linux-uefi-validation

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 6/6] x86/efi: introduce EFI_BOOT_SERVICES_WARN

[Matt Fleming]

On Sun, 14 Sep, at 02:01:30AM, Borislav Petkov wrote:
> On Sat, Sep 13, 2014 at 11:36:16AM -0700, Ricardo Neri wrote:
> > Being more verbose about this kind of illegal access from the firmware
> > increases the likelihood of this kind firmware bugs to be fixed.
> 
> I sincerely hope you're right and, more importantly, how do we make sure
> those warnings get seen in time for a fix to even be possible..?

Some firmware teams do run Linux as part of their validation process,
and have been known to pay attention to the kernel boot messages. So
there's definitely hope there.

But we are also taking a more active approach with the Linux UEFI
Validation project [1], where we consume these kinds of error messages
and turn them into explicit test passes/failures.

We've been attending the UEFI plugfests and trying to work directly with
firmware engineers to bridge that communication gap between firmware and
OS, so that we can fix these kinds of bugs before they appear in the
wild.

[1] - https://01.org/linux-uefi-validation

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/6] x86/efi: use efi_memory_descriptor in convenience functions

[Matt Fleming]

On Sat, 13 Sep, at 11:36:12AM, Ricardo Neri wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 64ecbb5..29c85fe 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -206,29 +206,23 @@ subsys_initcall(efisubsys_init);
>   */
>  void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
>  {
> -	struct efi_memory_map *map;
> -	void *p;
> -	map = efi.memmap;
> -	if (!map)
> +	efi_memory_desc_t *md;
> +
> +	md = efi_memory_descriptor(phys_addr);

Whoops. This change is causing the following build errors on ia64,

/home/build/git/efi/arch/ia64/kernel/efi.c:722:1: error: static declaration of ‘efi_memory_descriptor’ follows non-static declaration
 efi_memory_descriptor (unsigned long phys_addr)
 ^
In file included from /home/build/git/efi/arch/ia64/kernel/efi.c:32:0:
/home/build/git/efi/include/linux/efi.h:870:27: note: previous declaration of ‘efi_memory_descriptor’ was here
 extern efi_memory_desc_t *efi_memory_descriptor(unsigned long phys_addr);
                           ^

and on arm64...

drivers/built-in.o: In function `efi_lookup_mapped_addr':
:(.text+0xb0e0): undefined reference to `efi_memory_descriptor'

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/6] x86/efi: use efi_memory_descriptor in convenience functions

[Matt Fleming]

On Sat, 13 Sep, at 11:36:12AM, Ricardo Neri wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 64ecbb5..29c85fe 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -206,29 +206,23 @@ subsys_initcall(efisubsys_init);
>   */
>  void __iomem *efi_lookup_mapped_addr(u64 phys_addr)
>  {
> -	struct efi_memory_map *map;
> -	void *p;
> -	map = efi.memmap;
> -	if (!map)
> +	efi_memory_desc_t *md;
> +
> +	md = efi_memory_descriptor(phys_addr);

Whoops. This change is causing the following build errors on ia64,

/home/build/git/efi/arch/ia64/kernel/efi.c:722:1: error: static declaration of ‘efi_memory_descriptor’ follows non-static declaration
 efi_memory_descriptor (unsigned long phys_addr)
 ^
In file included from /home/build/git/efi/arch/ia64/kernel/efi.c:32:0:
/home/build/git/efi/include/linux/efi.h:870:27: note: previous declaration of ‘efi_memory_descriptor’ was here
 extern efi_memory_desc_t *efi_memory_descriptor(unsigned long phys_addr);
                           ^

and on arm64...

drivers/built-in.o: In function `efi_lookup_mapped_addr':
:(.text+0xb0e0): undefined reference to `efi_memory_descriptor'

-- 
Matt Fleming, Intel Open Source Technology Center