[PATCH v2 0/6] efi/x86: Avoid corrupted config tables under Xen

[PATCH v2 0/6] efi/x86: Avoid corrupted config tables under Xen

[Ard Biesheuvel]

This is an alternate approach to addressing the issue that Demi Marie is
attempting to fix in [0] (i.e., ESRT config table exposed to a x86 dom0
is corrupted because it resides in boot services memory as per the EFI
spec, where it gets corrupted by Xen). My main objection to that approach
is that it needs Xen-specific fixes in multiple different places, but we
still end up only fixing the ESRT case specifically.

So instead, I am proposing this series as a more generic way to handle
configuration tables that reside in boot services memory, and confining
the Xen specific logic to the Xen EFI glue code.

Given that EFI boot without a memory map is only permitted on x86 and
only when doing Xen boot, let's clear up some inconsistencies there
first so we can set the EFI_PARAVIRT flag on all architectures that do
pseudo-EFI boot straight into the core kernel (i.e., without going
through the stub). This moves a good chunk of EFI memory map
manipulation code into the x86 arch tree, where it arguably belongs as
no other architectures rely on it. This is implemented in patches 1 - 3.

Patch #4 refactors the ESRT sanity checks on the memory descriptor, by
moving them into the efi_mem_desc_lookup() helper, which should not
return corrupted descriptors in the first place.

Patch #5 adds a Xen hypercall fallback to efi_mem_desc_lookup() when
running under Xen without a EFI memory map, so that, e.g., the existing
ESRT code will perform its validation against the Xen provided
descriptor if no memory map is available.

Patch #6 updates the config table traversal code so that the Xen glue
code can force them to be disregarded, which happens when the table in
question points into a memory region that is not of a type that Xen
automatically reserves. Future changes can refine this logic if needed.

Changes since v1:
- add patch #4
- move Xen descriptor lookup into efi_mem_desc_lookup()
- drop allowlist for ACPI and SMBIOS tables

[0] https://lore.kernel.org/all/cover.1664298147.git.demi@invisiblethingslab.com/

Cc: Demi Marie Obenour <demi@invisiblethingslab.com>
Cc: Peter Jones <pjones@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Ard Biesheuvel (6):
  efi: Move EFI fake memmap support into x86 arch tree
  efi: memmap: Move manipulation routines into x86 arch tree
  efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures
  efi: memmap: Disregard bogus entries instead of returning them
  efi: xen: Implement memory descriptor lookup based on hypercall
  efi: Apply allowlist to EFI configuration tables when running under
    Xen

 arch/x86/Kconfig                                       |  20 ++
 arch/x86/include/asm/efi.h                             |  16 ++
 arch/x86/kernel/setup.c                                |   1 +
 arch/x86/platform/efi/Makefile                         |   4 +-
 arch/x86/platform/efi/efi.c                            |   8 +-
 {drivers/firmware => arch/x86/platform}/efi/fake_mem.c |  79 ++++++-
 arch/x86/platform/efi/memmap.c                         | 238 ++++++++++++++++++++
 drivers/firmware/efi/Kconfig                           |  22 --
 drivers/firmware/efi/Makefile                          |   4 -
 drivers/firmware/efi/efi.c                             |  25 +-
 drivers/firmware/efi/esrt.c                            |  18 +-
 drivers/firmware/efi/fake_mem.h                        |  10 -
 drivers/firmware/efi/fdtparams.c                       |   4 +
 drivers/firmware/efi/memmap.c                          | 224 +-----------------
 drivers/firmware/efi/x86_fake_mem.c                    |  75 ------
 drivers/xen/efi.c                                      |  58 +++++
 include/linux/efi.h                                    |  19 +-
 17 files changed, 446 insertions(+), 379 deletions(-)
 rename {drivers/firmware => arch/x86/platform}/efi/fake_mem.c (58%)
 create mode 100644 arch/x86/platform/efi/memmap.c
 delete mode 100644 drivers/firmware/efi/fake_mem.h
 delete mode 100644 drivers/firmware/efi/x86_fake_mem.c

-- 
2.35.1

[PATCH v2 1/6] efi: Move EFI fake memmap support into x86 arch tree

[Ard Biesheuvel]

The EFI fake memmap support is specific to x86, which manipulates the
EFI memory map in various different ways after receiving it from the EFI
stub. On other architectures, we have manages to push back on this, and
the EFI memory map is kept pristine.

So let's move the fake memmap code into the x86 arch tree, where it
arguably belongs.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/Kconfig                                       | 20 +++++
 arch/x86/include/asm/efi.h                             |  5 ++
 arch/x86/kernel/setup.c                                |  1 +
 arch/x86/platform/efi/Makefile                         |  1 +
 {drivers/firmware => arch/x86/platform}/efi/fake_mem.c | 79 +++++++++++++++++++-
 drivers/firmware/efi/Kconfig                           | 22 ------
 drivers/firmware/efi/Makefile                          |  4 -
 drivers/firmware/efi/fake_mem.h                        | 10 ---
 drivers/firmware/efi/x86_fake_mem.c                    | 75 -------------------
 include/linux/efi.h                                    |  6 --
 10 files changed, 103 insertions(+), 120 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..b98941c2fec4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1978,6 +1978,26 @@ config EFI_MIXED
 
 	  If unsure, say N.
 
+config EFI_FAKE_MEMMAP
+	bool "Enable EFI fake memory map"
+	depends on EFI
+	help
+	  Saying Y here will enable "efi_fake_mem" boot option.  By specifying
+	  this parameter, you can add arbitrary attribute to specific memory
+	  range by updating original (firmware provided) EFI memmap.  This is
+	  useful for debugging of EFI memmap related feature, e.g., Address
+	  Range Mirroring feature.
+
+config EFI_MAX_FAKE_MEM
+	int "maximum allowable number of ranges in efi_fake_mem boot option"
+	depends on EFI_FAKE_MEMMAP
+	range 1 128
+	default 8
+	help
+	  Maximum allowable number of ranges in efi_fake_mem boot option.
+	  Ranges can be set up to this value using comma-separated list.
+	  The default value is 8.
+
 source "kernel/Kconfig.hz"
 
 config KEXEC
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 897ea4aec16e..68414d924332 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -404,10 +404,15 @@ static inline void efi_reserve_boot_services(void)
 
 #ifdef CONFIG_EFI_FAKE_MEMMAP
 extern void __init efi_fake_memmap_early(void);
+extern void __init efi_fake_memmap(void);
 #else
 static inline void efi_fake_memmap_early(void)
 {
 }
+
+static inline void efi_fake_memmap(void)
+{
+}
 #endif
 
 #define arch_ima_efi_boot_mode	\
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7144ee..41ec3a69f3c7 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -31,6 +31,7 @@
 #include <xen/xen.h>
 
 #include <asm/apic.h>
+#include <asm/efi.h>
 #include <asm/numa.h>
 #include <asm/bios_ebda.h>
 #include <asm/bugs.h>
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index a50245157685..b481719b16cc 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -4,3 +4,4 @@ GCOV_PROFILE := n
 
 obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_FAKE_MEMMAP)	+= fake_mem.o
diff --git a/drivers/firmware/efi/fake_mem.c b/arch/x86/platform/efi/fake_mem.c
similarity index 58%
rename from drivers/firmware/efi/fake_mem.c
rename to arch/x86/platform/efi/fake_mem.c
index 6e0f34a38171..41d57cad3d84 100644
--- a/drivers/firmware/efi/fake_mem.c
+++ b/arch/x86/platform/efi/fake_mem.c
@@ -17,10 +17,13 @@
 #include <linux/memblock.h>
 #include <linux/types.h>
 #include <linux/sort.h>
-#include "fake_mem.h"
+#include <asm/e820/api.h>
+#include <asm/efi.h>
 
-struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
-int nr_fake_mem;
+#define EFI_MAX_FAKEMEM CONFIG_EFI_MAX_FAKE_MEM
+
+static struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
+static int nr_fake_mem;
 
 static int __init cmp_fake_mem(const void *x1, const void *x2)
 {
@@ -122,3 +125,73 @@ static int __init setup_fake_mem(char *p)
 }
 
 early_param("efi_fake_mem", setup_fake_mem);
+
+void __init efi_fake_memmap_early(void)
+{
+	int i;
+
+	/*
+	 * The late efi_fake_mem() call can handle all requests if
+	 * EFI_MEMORY_SP support is disabled.
+	 */
+	if (!efi_soft_reserve_enabled())
+		return;
+
+	if (!efi_enabled(EFI_MEMMAP) || !nr_fake_mem)
+		return;
+
+	/*
+	 * Given that efi_fake_memmap() needs to perform memblock
+	 * allocations it needs to run after e820__memblock_setup().
+	 * However, if efi_fake_mem specifies EFI_MEMORY_SP for a given
+	 * address range that potentially needs to mark the memory as
+	 * reserved prior to e820__memblock_setup(). Update e820
+	 * directly if EFI_MEMORY_SP is specified for an
+	 * EFI_CONVENTIONAL_MEMORY descriptor.
+	 */
+	for (i = 0; i < nr_fake_mem; i++) {
+		struct efi_mem_range *mem = &efi_fake_mems[i];
+		efi_memory_desc_t *md;
+		u64 m_start, m_end;
+
+		if ((mem->attribute & EFI_MEMORY_SP) == 0)
+			continue;
+
+		m_start = mem->range.start;
+		m_end = mem->range.end;
+		for_each_efi_memory_desc(md) {
+			u64 start, end, size;
+
+			if (md->type != EFI_CONVENTIONAL_MEMORY)
+				continue;
+
+			start = md->phys_addr;
+			end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+
+			if (m_start <= end && m_end >= start)
+				/* fake range overlaps descriptor */;
+			else
+				continue;
+
+			/*
+			 * Trim the boundary of the e820 update to the
+			 * descriptor in case the fake range overlaps
+			 * !EFI_CONVENTIONAL_MEMORY
+			 */
+			start = max(start, m_start);
+			end = min(end, m_end);
+			size = end - start + 1;
+
+			if (end <= start)
+				continue;
+
+			/*
+			 * Ensure each efi_fake_mem instance results in
+			 * a unique e820 resource
+			 */
+			e820__range_remove(start, size, E820_TYPE_RAM, 1);
+			e820__range_add(start, size, E820_TYPE_SOFT_RESERVED);
+			e820__update_table(e820_table);
+		}
+	}
+}
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 4f7e65293297..fceeea74522e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -37,28 +37,6 @@ config EFI_RUNTIME_MAP
 
 	  See also Documentation/ABI/testing/sysfs-firmware-efi-runtime-map.
 
-config EFI_FAKE_MEMMAP
-	bool "Enable EFI fake memory map"
-	depends on EFI && X86
-	default n
-	help
-	  Saying Y here will enable "efi_fake_mem" boot option.
-	  By specifying this parameter, you can add arbitrary attribute
-	  to specific memory range by updating original (firmware provided)
-	  EFI memmap.
-	  This is useful for debugging of EFI memmap related feature.
-	  e.g. Address Range Mirroring feature.
-
-config EFI_MAX_FAKE_MEM
-	int "maximum allowable number of ranges in efi_fake_mem boot option"
-	depends on EFI_FAKE_MEMMAP
-	range 1 128
-	default 8
-	help
-	  Maximum allowable number of ranges in efi_fake_mem boot option.
-	  Ranges can be set up to this value using comma-separated list.
-	  The default value is 8.
-
 config EFI_SOFT_RESERVE
 	bool "Reserve EFI Specific Purpose Memory"
 	depends on EFI && EFI_STUB && ACPI_HMAT
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 8d151e332584..8e4f0d5b26e5 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -23,7 +23,6 @@ obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 subdir-$(CONFIG_EFI_STUB)		+= libstub
-obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_map.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
@@ -32,9 +31,6 @@ obj-$(CONFIG_EFI_RCI2_TABLE)		+= rci2-table.o
 obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
 obj-$(CONFIG_LOAD_UEFI_KEYS)		+= mokvar-table.o
 
-fake_map-y				+= fake_mem.o
-fake_map-$(CONFIG_X86)			+= x86_fake_mem.o
-
 obj-$(CONFIG_SYSFB)			+= sysfb_efi.o
 
 arm-obj-$(CONFIG_EFI)			:= efi-init.o arm-runtime.o
diff --git a/drivers/firmware/efi/fake_mem.h b/drivers/firmware/efi/fake_mem.h
deleted file mode 100644
index d52791af4b18..000000000000
--- a/drivers/firmware/efi/fake_mem.h
+++ /dev/null
@@ -1,10 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __EFI_FAKE_MEM_H__
-#define __EFI_FAKE_MEM_H__
-#include <asm/efi.h>
-
-#define EFI_MAX_FAKEMEM CONFIG_EFI_MAX_FAKE_MEM
-
-extern struct efi_mem_range efi_fake_mems[EFI_MAX_FAKEMEM];
-extern int nr_fake_mem;
-#endif /* __EFI_FAKE_MEM_H__ */
diff --git a/drivers/firmware/efi/x86_fake_mem.c b/drivers/firmware/efi/x86_fake_mem.c
deleted file mode 100644
index 0bafcc1bb0f6..000000000000
--- a/drivers/firmware/efi/x86_fake_mem.c
+++ /dev/null
@@ -1,75 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright(c) 2019 Intel Corporation. All rights reserved. */
-#include <linux/efi.h>
-#include <asm/e820/api.h>
-#include "fake_mem.h"
-
-void __init efi_fake_memmap_early(void)
-{
-	int i;
-
-	/*
-	 * The late efi_fake_mem() call can handle all requests if
-	 * EFI_MEMORY_SP support is disabled.
-	 */
-	if (!efi_soft_reserve_enabled())
-		return;
-
-	if (!efi_enabled(EFI_MEMMAP) || !nr_fake_mem)
-		return;
-
-	/*
-	 * Given that efi_fake_memmap() needs to perform memblock
-	 * allocations it needs to run after e820__memblock_setup().
-	 * However, if efi_fake_mem specifies EFI_MEMORY_SP for a given
-	 * address range that potentially needs to mark the memory as
-	 * reserved prior to e820__memblock_setup(). Update e820
-	 * directly if EFI_MEMORY_SP is specified for an
-	 * EFI_CONVENTIONAL_MEMORY descriptor.
-	 */
-	for (i = 0; i < nr_fake_mem; i++) {
-		struct efi_mem_range *mem = &efi_fake_mems[i];
-		efi_memory_desc_t *md;
-		u64 m_start, m_end;
-
-		if ((mem->attribute & EFI_MEMORY_SP) == 0)
-			continue;
-
-		m_start = mem->range.start;
-		m_end = mem->range.end;
-		for_each_efi_memory_desc(md) {
-			u64 start, end, size;
-
-			if (md->type != EFI_CONVENTIONAL_MEMORY)
-				continue;
-
-			start = md->phys_addr;
-			end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
-
-			if (m_start <= end && m_end >= start)
-				/* fake range overlaps descriptor */;
-			else
-				continue;
-
-			/*
-			 * Trim the boundary of the e820 update to the
-			 * descriptor in case the fake range overlaps
-			 * !EFI_CONVENTIONAL_MEMORY
-			 */
-			start = max(start, m_start);
-			end = min(end, m_end);
-			size = end - start + 1;
-
-			if (end <= start)
-				continue;
-
-			/*
-			 * Ensure each efi_fake_mem instance results in
-			 * a unique e820 resource
-			 */
-			e820__range_remove(start, size, E820_TYPE_RAM, 1);
-			e820__range_add(start, size, E820_TYPE_SOFT_RESERVED);
-			e820__update_table(e820_table);
-		}
-	}
-}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e739196ce9b2..a6dbf354d2c3 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -749,12 +749,6 @@ extern struct kobject *efi_kobj;
 extern int efi_reboot_quirk_mode;
 extern bool efi_poweroff_required(void);
 
-#ifdef CONFIG_EFI_FAKE_MEMMAP
-extern void __init efi_fake_memmap(void);
-#else
-static inline void efi_fake_memmap(void) { }
-#endif
-
 extern unsigned long efi_mem_attr_table;
 
 /*
-- 
2.35.1

[PATCH v2 2/6] efi: memmap: Move manipulation routines into x86 arch tree

[Ard Biesheuvel]

The EFI memory map is a description of the memory layout as provided by
the firmware, and only x86 manipulates it in various different ways for
its own memory bookkeeping. So let's move the memmap routines that are
only used by x86 into the x86 arch tree.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h     |  11 +
 arch/x86/platform/efi/Makefile |   3 +-
 arch/x86/platform/efi/memmap.c | 235 ++++++++++++++++++++
 drivers/firmware/efi/memmap.c  | 221 +-----------------
 include/linux/efi.h            |  10 +-
 5 files changed, 251 insertions(+), 229 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 68414d924332..1fb4686f3d12 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -415,6 +415,17 @@ static inline void efi_fake_memmap(void)
 }
 #endif
 
+extern int __init efi_memmap_alloc(unsigned int num_entries,
+				   struct efi_memory_map_data *data);
+extern void __efi_memmap_free(u64 phys, unsigned long size,
+			      unsigned long flags);
+
+extern int __init efi_memmap_install(struct efi_memory_map_data *data);
+extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
+					 struct range *range);
+extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
+				     void *buf, struct efi_mem_range *mem);
+
 #define arch_ima_efi_boot_mode	\
 	({ extern struct boot_params boot_params; boot_params.secure_boot; })
 
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index b481719b16cc..ed5502a5185d 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -2,6 +2,7 @@
 KASAN_SANITIZE := n
 GCOV_PROFILE := n
 
-obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
+obj-$(CONFIG_EFI) 		+= memmap.o quirks.o efi.o efi_$(BITS).o \
+				   efi_stub_$(BITS).o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
 obj-$(CONFIG_EFI_FAKE_MEMMAP)	+= fake_mem.o
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
new file mode 100644
index 000000000000..44b886acf301
--- /dev/null
+++ b/arch/x86/platform/efi/memmap.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common EFI memory map functions.
+ */
+
+#define pr_fmt(fmt) "efi: " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/efi.h>
+#include <linux/io.h>
+#include <asm/early_ioremap.h>
+#include <linux/memblock.h>
+#include <linux/slab.h>
+
+static phys_addr_t __init __efi_memmap_alloc_early(unsigned long size)
+{
+	return memblock_phys_alloc(size, SMP_CACHE_BYTES);
+}
+
+static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
+{
+	unsigned int order = get_order(size);
+	struct page *p = alloc_pages(GFP_KERNEL, order);
+
+	if (!p)
+		return 0;
+
+	return PFN_PHYS(page_to_pfn(p));
+}
+
+void __init __efi_memmap_free(u64 phys, unsigned long size, unsigned long flags)
+{
+	if (flags & EFI_MEMMAP_MEMBLOCK) {
+		if (slab_is_available())
+			memblock_free_late(phys, size);
+		else
+			memblock_phys_free(phys, size);
+	} else if (flags & EFI_MEMMAP_SLAB) {
+		struct page *p = pfn_to_page(PHYS_PFN(phys));
+		unsigned int order = get_order(size);
+
+		free_pages((unsigned long) page_address(p), order);
+	}
+}
+
+/**
+ * efi_memmap_alloc - Allocate memory for the EFI memory map
+ * @num_entries: Number of entries in the allocated map.
+ * @data: efi memmap installation parameters
+ *
+ * Depending on whether mm_init() has already been invoked or not,
+ * either memblock or "normal" page allocation is used.
+ *
+ * Returns zero on success, a negative error code on failure.
+ */
+int __init efi_memmap_alloc(unsigned int num_entries,
+		struct efi_memory_map_data *data)
+{
+	/* Expect allocation parameters are zero initialized */
+	WARN_ON(data->phys_map || data->size);
+
+	data->size = num_entries * efi.memmap.desc_size;
+	data->desc_version = efi.memmap.desc_version;
+	data->desc_size = efi.memmap.desc_size;
+	data->flags &= ~(EFI_MEMMAP_SLAB | EFI_MEMMAP_MEMBLOCK);
+	data->flags |= efi.memmap.flags & EFI_MEMMAP_LATE;
+
+	if (slab_is_available()) {
+		data->flags |= EFI_MEMMAP_SLAB;
+		data->phys_map = __efi_memmap_alloc_late(data->size);
+	} else {
+		data->flags |= EFI_MEMMAP_MEMBLOCK;
+		data->phys_map = __efi_memmap_alloc_early(data->size);
+	}
+
+	if (!data->phys_map)
+		return -ENOMEM;
+	return 0;
+}
+
+/**
+ * efi_memmap_install - Install a new EFI memory map in efi.memmap
+ * @ctx: map allocation parameters (address, size, flags)
+ *
+ * Unlike efi_memmap_init_*(), this function does not allow the caller
+ * to switch from early to late mappings. It simply uses the existing
+ * mapping function and installs the new memmap.
+ *
+ * Returns zero on success, a negative error code on failure.
+ */
+int __init efi_memmap_install(struct efi_memory_map_data *data)
+{
+	efi_memmap_unmap();
+
+	return __efi_memmap_init(data);
+}
+
+/**
+ * efi_memmap_split_count - Count number of additional EFI memmap entries
+ * @md: EFI memory descriptor to split
+ * @range: Address range (start, end) to split around
+ *
+ * Returns the number of additional EFI memmap entries required to
+ * accommodate @range.
+ */
+int __init efi_memmap_split_count(efi_memory_desc_t *md, struct range *range)
+{
+	u64 m_start, m_end;
+	u64 start, end;
+	int count = 0;
+
+	start = md->phys_addr;
+	end = start + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+
+	/* modifying range */
+	m_start = range->start;
+	m_end = range->end;
+
+	if (m_start <= start) {
+		/* split into 2 parts */
+		if (start < m_end && m_end < end)
+			count++;
+	}
+
+	if (start < m_start && m_start < end) {
+		/* split into 3 parts */
+		if (m_end < end)
+			count += 2;
+		/* split into 2 parts */
+		if (end <= m_end)
+			count++;
+	}
+
+	return count;
+}
+
+/**
+ * efi_memmap_insert - Insert a memory region in an EFI memmap
+ * @old_memmap: The existing EFI memory map structure
+ * @buf: Address of buffer to store new map
+ * @mem: Memory map entry to insert
+ *
+ * It is suggested that you call efi_memmap_split_count() first
+ * to see how large @buf needs to be.
+ */
+void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
+			      struct efi_mem_range *mem)
+{
+	u64 m_start, m_end, m_attr;
+	efi_memory_desc_t *md;
+	u64 start, end;
+	void *old, *new;
+
+	/* modifying range */
+	m_start = mem->range.start;
+	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) {
+
+		/* copy original EFI memory descriptor */
+		memcpy(new, old, old_memmap->desc_size);
+		md = new;
+		start = md->phys_addr;
+		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
+
+		if (m_start <= start && end <= m_end)
+			md->attribute |= m_attr;
+
+		if (m_start <= start &&
+		    (start < m_end && m_end < end)) {
+			/* first part */
+			md->attribute |= m_attr;
+			md->num_pages = (m_end - md->phys_addr + 1) >>
+				EFI_PAGE_SHIFT;
+			/* latter part */
+			new += old_memmap->desc_size;
+			memcpy(new, old, old_memmap->desc_size);
+			md = new;
+			md->phys_addr = m_end + 1;
+			md->num_pages = (end - md->phys_addr + 1) >>
+				EFI_PAGE_SHIFT;
+		}
+
+		if ((start < m_start && m_start < end) && m_end < end) {
+			/* first part */
+			md->num_pages = (m_start - md->phys_addr) >>
+				EFI_PAGE_SHIFT;
+			/* middle part */
+			new += old_memmap->desc_size;
+			memcpy(new, old, old_memmap->desc_size);
+			md = new;
+			md->attribute |= m_attr;
+			md->phys_addr = m_start;
+			md->num_pages = (m_end - m_start + 1) >>
+				EFI_PAGE_SHIFT;
+			/* last part */
+			new += old_memmap->desc_size;
+			memcpy(new, old, old_memmap->desc_size);
+			md = new;
+			md->phys_addr = m_end + 1;
+			md->num_pages = (end - m_end) >>
+				EFI_PAGE_SHIFT;
+		}
+
+		if ((start < m_start && m_start < end) &&
+		    (end <= m_end)) {
+			/* first part */
+			md->num_pages = (m_start - md->phys_addr) >>
+				EFI_PAGE_SHIFT;
+			/* latter part */
+			new += old_memmap->desc_size;
+			memcpy(new, old, old_memmap->desc_size);
+			md = new;
+			md->phys_addr = m_start;
+			md->num_pages = (end - md->phys_addr + 1) >>
+				EFI_PAGE_SHIFT;
+			md->attribute |= m_attr;
+		}
+	}
+}
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 6ec7970dbd40..3501d3814f22 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -13,35 +13,8 @@
 #include <linux/memblock.h>
 #include <linux/slab.h>
 
-static phys_addr_t __init __efi_memmap_alloc_early(unsigned long size)
+void __weak __efi_memmap_free(u64 phys, unsigned long size, unsigned long flags)
 {
-	return memblock_phys_alloc(size, SMP_CACHE_BYTES);
-}
-
-static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
-{
-	unsigned int order = get_order(size);
-	struct page *p = alloc_pages(GFP_KERNEL, order);
-
-	if (!p)
-		return 0;
-
-	return PFN_PHYS(page_to_pfn(p));
-}
-
-void __init __efi_memmap_free(u64 phys, unsigned long size, unsigned long flags)
-{
-	if (flags & EFI_MEMMAP_MEMBLOCK) {
-		if (slab_is_available())
-			memblock_free_late(phys, size);
-		else
-			memblock_phys_free(phys, size);
-	} else if (flags & EFI_MEMMAP_SLAB) {
-		struct page *p = pfn_to_page(PHYS_PFN(phys));
-		unsigned int order = get_order(size);
-
-		free_pages((unsigned long) page_address(p), order);
-	}
 }
 
 static void __init efi_memmap_free(void)
@@ -51,41 +24,6 @@ static void __init efi_memmap_free(void)
 			efi.memmap.flags);
 }
 
-/**
- * efi_memmap_alloc - Allocate memory for the EFI memory map
- * @num_entries: Number of entries in the allocated map.
- * @data: efi memmap installation parameters
- *
- * Depending on whether mm_init() has already been invoked or not,
- * either memblock or "normal" page allocation is used.
- *
- * Returns zero on success, a negative error code on failure.
- */
-int __init efi_memmap_alloc(unsigned int num_entries,
-		struct efi_memory_map_data *data)
-{
-	/* Expect allocation parameters are zero initialized */
-	WARN_ON(data->phys_map || data->size);
-
-	data->size = num_entries * efi.memmap.desc_size;
-	data->desc_version = efi.memmap.desc_version;
-	data->desc_size = efi.memmap.desc_size;
-	data->flags &= ~(EFI_MEMMAP_SLAB | EFI_MEMMAP_MEMBLOCK);
-	data->flags |= efi.memmap.flags & EFI_MEMMAP_LATE;
-
-	if (slab_is_available()) {
-		data->flags |= EFI_MEMMAP_SLAB;
-		data->phys_map = __efi_memmap_alloc_late(data->size);
-	} else {
-		data->flags |= EFI_MEMMAP_MEMBLOCK;
-		data->phys_map = __efi_memmap_alloc_early(data->size);
-	}
-
-	if (!data->phys_map)
-		return -ENOMEM;
-	return 0;
-}
-
 /**
  * __efi_memmap_init - Common code for mapping the EFI memory map
  * @data: EFI memory map data
@@ -101,7 +39,7 @@ int __init efi_memmap_alloc(unsigned int num_entries,
  *
  * Returns zero on success, a negative error code on failure.
  */
-static int __init __efi_memmap_init(struct efi_memory_map_data *data)
+int __init __efi_memmap_init(struct efi_memory_map_data *data)
 {
 	struct efi_memory_map map;
 	phys_addr_t phys_map;
@@ -220,158 +158,3 @@ int __init efi_memmap_init_late(phys_addr_t addr, unsigned long size)
 
 	return __efi_memmap_init(&data);
 }
-
-/**
- * efi_memmap_install - Install a new EFI memory map in efi.memmap
- * @ctx: map allocation parameters (address, size, flags)
- *
- * Unlike efi_memmap_init_*(), this function does not allow the caller
- * to switch from early to late mappings. It simply uses the existing
- * mapping function and installs the new memmap.
- *
- * Returns zero on success, a negative error code on failure.
- */
-int __init efi_memmap_install(struct efi_memory_map_data *data)
-{
-	efi_memmap_unmap();
-
-	return __efi_memmap_init(data);
-}
-
-/**
- * efi_memmap_split_count - Count number of additional EFI memmap entries
- * @md: EFI memory descriptor to split
- * @range: Address range (start, end) to split around
- *
- * Returns the number of additional EFI memmap entries required to
- * accommodate @range.
- */
-int __init efi_memmap_split_count(efi_memory_desc_t *md, struct range *range)
-{
-	u64 m_start, m_end;
-	u64 start, end;
-	int count = 0;
-
-	start = md->phys_addr;
-	end = start + (md->num_pages << EFI_PAGE_SHIFT) - 1;
-
-	/* modifying range */
-	m_start = range->start;
-	m_end = range->end;
-
-	if (m_start <= start) {
-		/* split into 2 parts */
-		if (start < m_end && m_end < end)
-			count++;
-	}
-
-	if (start < m_start && m_start < end) {
-		/* split into 3 parts */
-		if (m_end < end)
-			count += 2;
-		/* split into 2 parts */
-		if (end <= m_end)
-			count++;
-	}
-
-	return count;
-}
-
-/**
- * efi_memmap_insert - Insert a memory region in an EFI memmap
- * @old_memmap: The existing EFI memory map structure
- * @buf: Address of buffer to store new map
- * @mem: Memory map entry to insert
- *
- * It is suggested that you call efi_memmap_split_count() first
- * to see how large @buf needs to be.
- */
-void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
-			      struct efi_mem_range *mem)
-{
-	u64 m_start, m_end, m_attr;
-	efi_memory_desc_t *md;
-	u64 start, end;
-	void *old, *new;
-
-	/* modifying range */
-	m_start = mem->range.start;
-	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) {
-
-		/* copy original EFI memory descriptor */
-		memcpy(new, old, old_memmap->desc_size);
-		md = new;
-		start = md->phys_addr;
-		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
-
-		if (m_start <= start && end <= m_end)
-			md->attribute |= m_attr;
-
-		if (m_start <= start &&
-		    (start < m_end && m_end < end)) {
-			/* first part */
-			md->attribute |= m_attr;
-			md->num_pages = (m_end - md->phys_addr + 1) >>
-				EFI_PAGE_SHIFT;
-			/* latter part */
-			new += old_memmap->desc_size;
-			memcpy(new, old, old_memmap->desc_size);
-			md = new;
-			md->phys_addr = m_end + 1;
-			md->num_pages = (end - md->phys_addr + 1) >>
-				EFI_PAGE_SHIFT;
-		}
-
-		if ((start < m_start && m_start < end) && m_end < end) {
-			/* first part */
-			md->num_pages = (m_start - md->phys_addr) >>
-				EFI_PAGE_SHIFT;
-			/* middle part */
-			new += old_memmap->desc_size;
-			memcpy(new, old, old_memmap->desc_size);
-			md = new;
-			md->attribute |= m_attr;
-			md->phys_addr = m_start;
-			md->num_pages = (m_end - m_start + 1) >>
-				EFI_PAGE_SHIFT;
-			/* last part */
-			new += old_memmap->desc_size;
-			memcpy(new, old, old_memmap->desc_size);
-			md = new;
-			md->phys_addr = m_end + 1;
-			md->num_pages = (end - m_end) >>
-				EFI_PAGE_SHIFT;
-		}
-
-		if ((start < m_start && m_start < end) &&
-		    (end <= m_end)) {
-			/* first part */
-			md->num_pages = (m_start - md->phys_addr) >>
-				EFI_PAGE_SHIFT;
-			/* latter part */
-			new += old_memmap->desc_size;
-			memcpy(new, old, old_memmap->desc_size);
-			md = new;
-			md->phys_addr = m_start;
-			md->num_pages = (end - md->phys_addr + 1) >>
-				EFI_PAGE_SHIFT;
-			md->attribute |= m_attr;
-		}
-	}
-}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index a6dbf354d2c3..256e70e42114 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -707,18 +707,10 @@ static inline efi_status_t efi_query_variable_store(u32 attributes,
 #endif
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 
-extern int __init efi_memmap_alloc(unsigned int num_entries,
-				   struct efi_memory_map_data *data);
-extern void __efi_memmap_free(u64 phys, unsigned long size,
-			      unsigned long flags);
+extern int __init __efi_memmap_init(struct efi_memory_map_data *data);
 extern int __init efi_memmap_init_early(struct efi_memory_map_data *data);
 extern int __init efi_memmap_init_late(phys_addr_t addr, unsigned long size);
 extern void __init efi_memmap_unmap(void);
-extern int __init efi_memmap_install(struct efi_memory_map_data *data);
-extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
-					 struct range *range);
-extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
-				     void *buf, struct efi_mem_range *mem);
 
 #ifdef CONFIG_EFI_ESRT
 extern void __init efi_esrt_init(void);
-- 
2.35.1

[PATCH v2 3/6] efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures

[Ard Biesheuvel]

Currently, the EFI_PARAVIRT flag is only used by x86, even though other
architectures also support pseudo-EFI boot, where the core kernel is
invoked directly and provided with a set of data tables that resemble
the ones constructed by the EFI stub, which never actually runs in that
case.

Let's fix this inconsistency, and always set this flag when booting dom0
via the EFI boot path. Note that Xen on x86 does not provide the EFI
memory map in this case, whereas other architectures do, so move the
associated EFI_PARAVIRT check into the x86 platform code.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi.c      | 8 +++++---
 arch/x86/platform/efi/memmap.c   | 3 +++
 drivers/firmware/efi/fdtparams.c | 4 ++++
 drivers/firmware/efi/memmap.c    | 3 ---
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 6e598bd78eef..6a6f2a585a3d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -214,9 +214,11 @@ int __init efi_memblock_x86_reserve_range(void)
 	data.desc_size		= e->efi_memdesc_size;
 	data.desc_version	= e->efi_memdesc_version;
 
-	rv = efi_memmap_init_early(&data);
-	if (rv)
-		return rv;
+	if (!efi_enabled(EFI_PARAVIRT)) {
+		rv = efi_memmap_init_early(&data);
+		if (rv)
+			return rv;
+	}
 
 	if (add_efi_memmap || do_efi_soft_reserve())
 		do_add_efi_memmap();
diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
index 44b886acf301..18e14ec16720 100644
--- a/arch/x86/platform/efi/memmap.c
+++ b/arch/x86/platform/efi/memmap.c
@@ -93,6 +93,9 @@ int __init efi_memmap_install(struct efi_memory_map_data *data)
 {
 	efi_memmap_unmap();
 
+	if (efi_enabled(EFI_PARAVIRT))
+		return 0;
+
 	return __efi_memmap_init(data);
 }
 
diff --git a/drivers/firmware/efi/fdtparams.c b/drivers/firmware/efi/fdtparams.c
index e901f8564ca0..0ec83ba58097 100644
--- a/drivers/firmware/efi/fdtparams.c
+++ b/drivers/firmware/efi/fdtparams.c
@@ -30,11 +30,13 @@ static __initconst const char name[][22] = {
 
 static __initconst const struct {
 	const char	path[17];
+	u8		paravirt;
 	const char	params[PARAMCOUNT][26];
 } dt_params[] = {
 	{
 #ifdef CONFIG_XEN    //  <-------17------>
 		.path = "/hypervisor/uefi",
+		.paravirt = 1,
 		.params = {
 			[SYSTAB] = "xen,uefi-system-table",
 			[MMBASE] = "xen,uefi-mmap-start",
@@ -121,6 +123,8 @@ u64 __init efi_get_fdt_params(struct efi_memory_map_data *mm)
 			pr_err("Can't find property '%s' in DT!\n", pname);
 			return 0;
 		}
+		if (dt_params[i].paravirt)
+			set_bit(EFI_PARAVIRT, &efi.flags);
 		return systab;
 	}
 notfound:
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 3501d3814f22..9508082af907 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -44,9 +44,6 @@ int __init __efi_memmap_init(struct efi_memory_map_data *data)
 	struct efi_memory_map map;
 	phys_addr_t phys_map;
 
-	if (efi_enabled(EFI_PARAVIRT))
-		return 0;
-
 	phys_map = data->phys_map;
 
 	if (data->flags & EFI_MEMMAP_LATE)
-- 
2.35.1

[PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Ard Biesheuvel]

The ESRT code currently contains some sanity checks on the memory
descriptor it obtains, but these can only trigger when the descriptor is
invalid (if at all).

So let's drop these checks, and instead, disregard descriptors entirely
if the start address is misaligned, or the number of pages reaches
beyond the end of the address space. Note that the memory map as a whole
could still be inconsistent, i.e., multiple entries might cover the same
area, or the address could be outside of the addressable VA space, but
validating that goes beyond the scope of these helpers.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/efi.c  | 13 +++++++------
 drivers/firmware/efi/esrt.c | 18 +-----------------
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 11857af72859..55bd3f4aab28 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 	efi_memory_desc_t *md;
 
 	if (!efi_enabled(EFI_MEMMAP)) {
-		pr_err_once("EFI_MEMMAP is not enabled.\n");
+		pr_warn_once("EFI_MEMMAP is not enabled.\n");
 		return -EINVAL;
 	}
 
-	if (!out_md) {
-		pr_err_once("out_md is null.\n");
-		return -EINVAL;
-        }
-
 	for_each_efi_memory_desc(md) {
 		u64 size;
 		u64 end;
 
+		/* skip bogus entries */
+		if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
+		    (md->phys_addr > 0 &&
+		     (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT)))
+			continue;
+
 		size = md->num_pages << EFI_PAGE_SHIFT;
 		end = md->phys_addr + size;
 		if (phys_addr >= md->phys_addr && phys_addr < end) {
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e7..8f86f2b0734b 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
 	int rc;
 	phys_addr_t end;
 
-	if (!efi_enabled(EFI_MEMMAP))
-		return;
-
 	pr_debug("esrt-init: loading.\n");
 	if (!esrt_table_exists())
 		return;
@@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
 		return;
 	}
 
-	max = efi_mem_desc_end(&md);
-	if (max < efi.esrt) {
-		pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
-		       (void *)efi.esrt, (void *)max);
-		return;
-	}
-
+	max = efi_mem_desc_end(&md) - efi.esrt;
 	size = sizeof(*esrt);
-	max -= efi.esrt;
-
-	if (max < size) {
-		pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
-		       size, max);
-		return;
-	}
 
 	va = early_memremap(efi.esrt, size);
 	if (!va) {
-- 
2.35.1

[PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Ard Biesheuvel]

Xen on x86 boots dom0 in EFI mode but without providing a memory map.
This means that some sanity checks we would like to perform on
configuration tables or other data structures in memory are not
currently possible. Xen does, however, expose EFI memory descriptor info
via a Xen hypercall, so let's wire that up instead.

Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/efi.c |  5 ++-
 drivers/xen/efi.c          | 34 ++++++++++++++++++++
 include/linux/efi.h        |  1 +
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 55bd3f4aab28..2c12b1a06481 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
  * and if so, populate the supplied memory descriptor with the appropriate
  * data.
  */
-int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 {
 	efi_memory_desc_t *md;
 
@@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 	return -ENOENT;
 }
 
+extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+	 __weak __alias(__efi_mem_desc_lookup);
+
 /*
  * Calculate the highest address of an efi memory descriptor.
  */
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index d1ff2186ebb4..74f3f6d8cdc8 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -26,6 +26,7 @@
 
 #include <xen/interface/xen.h>
 #include <xen/interface/platform.h>
+#include <xen/page.h>
 #include <xen/xen.h>
 #include <xen/xen-ops.h>
 
@@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
 	efi.get_next_high_mono_count	= xen_efi_get_next_high_mono_count;
 	efi.reset_system		= xen_efi_reset_system;
 }
+
+int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+{
+	static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
+		      "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
+	struct xen_platform_op op = {
+		.cmd = XENPF_firmware_info,
+		.u.firmware_info = {
+			.type = XEN_FW_EFI_INFO,
+			.index = XEN_FW_EFI_MEM_INFO,
+			.u.efi_info.mem.addr = phys_addr,
+			.u.efi_info.mem.size = U64_MAX - phys_addr,
+		}
+	};
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+	int rc;
+
+	if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
+		return __efi_mem_desc_lookup(phys_addr, out_md);
+
+	rc = HYPERVISOR_platform_op(&op);
+	if (rc) {
+		pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
+			phys_addr, rc);
+	}
+
+	out_md->phys_addr	= info->mem.addr;
+	out_md->num_pages	= info->mem.size >> EFI_PAGE_SHIFT;
+	out_md->type		= info->mem.type;
+	out_md->attribute	= info->mem.attr;
+
+        return 0;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 256e70e42114..e0ee6f6da4b4 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -731,6 +731,7 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern u64 efi_mem_desc_end(efi_memory_desc_t *md);
 extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
+extern int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
 extern void efi_mem_reserve(phys_addr_t addr, u64 size);
 extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
-- 
2.35.1

[PATCH v2 6/6] efi: Apply allowlist to EFI configuration tables when running under Xen

[Ard Biesheuvel]

As it turns out, Xen does not guarantee that EFI bootservices data
regions in memory are preserved, which means that EFI configuration
tables pointing into such memory regions may be corrupted before the
dom0 OS has had a chance to inspect them.

Demi Marie reports that this is causing problems for Qubes OS when it
attempts to perform system firmware updates, which requires that the
contents of the ESRT configuration table are valid when the fwupd user
space program runs.

However, other configuration tables such as the memory attributes
table or the runtime properties table are equally affected, and so we
need a comprehensive workaround that works for any table type.

So when running under Xen, check the EFI memory descriptor covering the
start of the table, and disregard the table if it does not reside in
memory that is preserved by Xen.

Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/efi.c |  7 ++++++
 drivers/xen/efi.c          | 24 ++++++++++++++++++++
 include/linux/efi.h        |  2 ++
 3 files changed, 33 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 2c12b1a06481..0a4583c13a40 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -560,6 +560,13 @@ static __init int match_config_table(const efi_guid_t *guid,
 
 	for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) {
 		if (!efi_guidcmp(*guid, table_types[i].guid)) {
+			if (IS_ENABLED(CONFIG_XEN_EFI) &&
+			    !xen_efi_config_table_is_usable(guid, table)) {
+				if (table_types[i].name[0])
+					pr_cont("(%s=0x%lx) ",
+						table_types[i].name, table);
+				return 1;
+			}
 			*(table_types[i].ptr) = table;
 			if (table_types[i].name[0])
 				pr_cont("%s=0x%lx ",
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 74f3f6d8cdc8..c275a9c377fe 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -326,3 +326,27 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 
         return 0;
 }
+
+bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
+					   unsigned long table)
+{
+	efi_memory_desc_t md;
+	int rc;
+
+	if (!efi_enabled(EFI_PARAVIRT))
+		return true;
+
+	rc = efi_mem_desc_lookup(table, &md);
+	if (rc)
+		return false;
+
+	switch (md.type) {
+	case EFI_RUNTIME_SERVICES_CODE:
+	case EFI_RUNTIME_SERVICES_DATA:
+	case EFI_ACPI_RECLAIM_MEMORY:
+	case EFI_RESERVED_TYPE:
+		return true;
+	}
+
+	return false;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e0ee6f6da4b4..b0cba86352ce 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1352,4 +1352,6 @@ struct linux_efi_initrd {
 /* Header of a populated EFI secret area */
 #define EFI_SECRET_TABLE_HEADER_GUID	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66,  0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
 
+bool xen_efi_config_table_is_usable(const efi_guid_t *, unsigned long table);
+
 #endif /* _LINUX_EFI_H */
-- 
2.35.1

Re: [PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote:
> The ESRT code currently contains some sanity checks on the memory
> descriptor it obtains, but these can only trigger when the descriptor is
> invalid (if at all).
> 
> So let's drop these checks, and instead, disregard descriptors entirely
> if the start address is misaligned, or the number of pages reaches
> beyond the end of the address space. Note that the memory map as a whole
> could still be inconsistent, i.e., multiple entries might cover the same
> area, or the address could be outside of the addressable VA space, but
> validating that goes beyond the scope of these helpers.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/efi.c  | 13 +++++++------
>  drivers/firmware/efi/esrt.c | 18 +-----------------
>  2 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 11857af72859..55bd3f4aab28 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  	efi_memory_desc_t *md;
>  
>  	if (!efi_enabled(EFI_MEMMAP)) {
> -		pr_err_once("EFI_MEMMAP is not enabled.\n");
> +		pr_warn_once("EFI_MEMMAP is not enabled.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (!out_md) {
> -		pr_err_once("out_md is null.\n");
> -		return -EINVAL;
> -        }
> -

Nit: this seems unrelated.

>  	for_each_efi_memory_desc(md) {
>  		u64 size;
>  		u64 end;
>  
> +		/* skip bogus entries */
> +		if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
> +		    (md->phys_addr > 0 &&
> +		     (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT)))
> +			continue;

Should this also check if md->num_pages is 0?  Also, should this check
be part of for_each_efi_memory_desc()?

> +
>  		size = md->num_pages << EFI_PAGE_SHIFT;
>  		end = md->phys_addr + size;
>  		if (phys_addr >= md->phys_addr && phys_addr < end) {
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 2a2f52b017e7..8f86f2b0734b 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
>  	int rc;
>  	phys_addr_t end;
>  
> -	if (!efi_enabled(EFI_MEMMAP))
> -		return;
> -
>  	pr_debug("esrt-init: loading.\n");
>  	if (!esrt_table_exists())
>  		return;
> @@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
>  		return;
>  	}
>  
> -	max = efi_mem_desc_end(&md);
> -	if (max < efi.esrt) {
> -		pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> -		       (void *)efi.esrt, (void *)max);
> -		return;
> -	}
> -
> +	max = efi_mem_desc_end(&md) - efi.esrt;
>  	size = sizeof(*esrt);
> -	max -= efi.esrt;
> -
> -	if (max < size) {
> -		pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
> -		       size, max);
> -		return;
> -	}

This can still happen if the ESRT pointer is very very close to the end
of a memory map entry, unless there is another check that handles
such cases.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 3616 bytes --]

On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> This means that some sanity checks we would like to perform on
> configuration tables or other data structures in memory are not
> currently possible. Xen does, however, expose EFI memory descriptor info
> via a Xen hypercall, so let's wire that up instead.
> 
> Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/efi.c |  5 ++-
>  drivers/xen/efi.c          | 34 ++++++++++++++++++++
>  include/linux/efi.h        |  1 +
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 55bd3f4aab28..2c12b1a06481 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
>   * and if so, populate the supplied memory descriptor with the appropriate
>   * data.
>   */
> -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  {
>  	efi_memory_desc_t *md;
>  
> @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  	return -ENOENT;
>  }
>  
> +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> +	 __weak __alias(__efi_mem_desc_lookup);
> +
>  /*
>   * Calculate the highest address of an efi memory descriptor.
>   */
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index d1ff2186ebb4..74f3f6d8cdc8 100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -26,6 +26,7 @@
>  
>  #include <xen/interface/xen.h>
>  #include <xen/interface/platform.h>
> +#include <xen/page.h>
>  #include <xen/xen.h>
>  #include <xen/xen-ops.h>
>  
> @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
>  	efi.get_next_high_mono_count	= xen_efi_get_next_high_mono_count;
>  	efi.reset_system		= xen_efi_reset_system;
>  }
> +
> +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> +{
> +	static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> +		      "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> +	struct xen_platform_op op = {
> +		.cmd = XENPF_firmware_info,
> +		.u.firmware_info = {
> +			.type = XEN_FW_EFI_INFO,
> +			.index = XEN_FW_EFI_MEM_INFO,
> +			.u.efi_info.mem.addr = phys_addr,
> +			.u.efi_info.mem.size = U64_MAX - phys_addr,
> +		}
> +	};
> +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +	int rc;
> +
> +	if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> +		return __efi_mem_desc_lookup(phys_addr, out_md);
> +
> +	rc = HYPERVISOR_platform_op(&op);
> +	if (rc) {
> +		pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> +			phys_addr, rc);
> +	}
> +
> +	out_md->phys_addr	= info->mem.addr;

This will be equal to phys_addr, not the actual start of the memory
region.

> +	out_md->num_pages	= info->mem.size >> EFI_PAGE_SHIFT;

Similarly, this will be the number of bytes in the memory region
after phys_addr, not the total number of bytes in the region.  These two
differences mean that this function is not strictly equivalent to the
original efi_mem_desc_lookup().

I am not sure if this matters in practice, but I thought you would want
to be aware of it.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/6] efi: memmap: Disregard bogus entries instead of returning them

[Ard Biesheuvel]

On Mon, 3 Oct 2022 at 17:18, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Mon, Oct 03, 2022 at 01:26:23PM +0200, Ard Biesheuvel wrote:
> > The ESRT code currently contains some sanity checks on the memory
> > descriptor it obtains, but these can only trigger when the descriptor is
> > invalid (if at all).
> >
> > So let's drop these checks, and instead, disregard descriptors entirely
> > if the start address is misaligned, or the number of pages reaches
> > beyond the end of the address space. Note that the memory map as a whole
> > could still be inconsistent, i.e., multiple entries might cover the same
> > area, or the address could be outside of the addressable VA space, but
> > validating that goes beyond the scope of these helpers.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  drivers/firmware/efi/efi.c  | 13 +++++++------
> >  drivers/firmware/efi/esrt.c | 18 +-----------------
> >  2 files changed, 8 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 11857af72859..55bd3f4aab28 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -461,19 +461,20 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> >       efi_memory_desc_t *md;
> >
> >       if (!efi_enabled(EFI_MEMMAP)) {
> > -             pr_err_once("EFI_MEMMAP is not enabled.\n");
> > +             pr_warn_once("EFI_MEMMAP is not enabled.\n");
> >               return -EINVAL;
> >       }
> >
> > -     if (!out_md) {
> > -             pr_err_once("out_md is null.\n");
> > -             return -EINVAL;
> > -        }
> > -
>
> Nit: this seems unrelated.
>
> >       for_each_efi_memory_desc(md) {
> >               u64 size;
> >               u64 end;
> >
> > +             /* skip bogus entries */
> > +             if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
> > +                 (md->phys_addr > 0 &&
> > +                  (md->num_pages > (U64_MAX - md->phys_addr + 1) >> EFI_PAGE_SHIFT)))
> > +                     continue;
>
> Should this also check if md->num_pages is 0?

Yes, probably.

>  Also, should this check
> be part of for_each_efi_memory_desc()?
>

No, I don't think so. The for_each_xxx() helpers we have throughout
the kernel usually don't incorporate such checks, and I'd prefer to
adhere to the principle of least surprise here.

> > +
> >               size = md->num_pages << EFI_PAGE_SHIFT;
> >               end = md->phys_addr + size;
> >               if (phys_addr >= md->phys_addr && phys_addr < end) {
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index 2a2f52b017e7..8f86f2b0734b 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -247,9 +247,6 @@ void __init efi_esrt_init(void)
> >       int rc;
> >       phys_addr_t end;
> >
> > -     if (!efi_enabled(EFI_MEMMAP))
> > -             return;
> > -
> >       pr_debug("esrt-init: loading.\n");
> >       if (!esrt_table_exists())
> >               return;
> > @@ -263,21 +260,8 @@ void __init efi_esrt_init(void)
> >               return;
> >       }
> >
> > -     max = efi_mem_desc_end(&md);
> > -     if (max < efi.esrt) {
> > -             pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> > -                    (void *)efi.esrt, (void *)max);
> > -             return;
> > -     }
> > -
> > +     max = efi_mem_desc_end(&md) - efi.esrt;
> >       size = sizeof(*esrt);
> > -     max -= efi.esrt;
> > -
> > -     if (max < size) {
> > -             pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
> > -                    size, max);
> > -             return;
> > -     }
>
> This can still happen if the ESRT pointer is very very close to the end
> of a memory map entry, unless there is another check that handles
> such cases.

You're right - I missed that.

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Ard Biesheuvel]

On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > This means that some sanity checks we would like to perform on
> > configuration tables or other data structures in memory are not
> > currently possible. Xen does, however, expose EFI memory descriptor info
> > via a Xen hypercall, so let's wire that up instead.
> >
> > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  drivers/firmware/efi/efi.c |  5 ++-
> >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> >  include/linux/efi.h        |  1 +
> >  3 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 55bd3f4aab28..2c12b1a06481 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> >   * and if so, populate the supplied memory descriptor with the appropriate
> >   * data.
> >   */
> > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> >  {
> >       efi_memory_desc_t *md;
> >
> > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> >       return -ENOENT;
> >  }
> >
> > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > +      __weak __alias(__efi_mem_desc_lookup);
> > +
> >  /*
> >   * Calculate the highest address of an efi memory descriptor.
> >   */
> > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > --- a/drivers/xen/efi.c
> > +++ b/drivers/xen/efi.c
> > @@ -26,6 +26,7 @@
> >
> >  #include <xen/interface/xen.h>
> >  #include <xen/interface/platform.h>
> > +#include <xen/page.h>
> >  #include <xen/xen.h>
> >  #include <xen/xen-ops.h>
> >
> > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> >       efi.reset_system                = xen_efi_reset_system;
> >  }
> > +
> > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > +{
> > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > +     struct xen_platform_op op = {
> > +             .cmd = XENPF_firmware_info,
> > +             .u.firmware_info = {
> > +                     .type = XEN_FW_EFI_INFO,
> > +                     .index = XEN_FW_EFI_MEM_INFO,
> > +                     .u.efi_info.mem.addr = phys_addr,
> > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > +             }
> > +     };
> > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > +     int rc;
> > +
> > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > +
> > +     rc = HYPERVISOR_platform_op(&op);
> > +     if (rc) {
> > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > +                     phys_addr, rc);
> > +     }
> > +
> > +     out_md->phys_addr       = info->mem.addr;
>
> This will be equal to phys_addr, not the actual start of the memory
> region.
>
> > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
>
> Similarly, this will be the number of bytes in the memory region
> after phys_addr, not the total number of bytes in the region.  These two
> differences mean that this function is not strictly equivalent to the
> original efi_mem_desc_lookup().
>
> I am not sure if this matters in practice, but I thought you would want
> to be aware of it.

This is a bit disappointing. Is there no way to obtain this
information via a Xen hypercall?

In any case, it means we'll need to round down phys_addr to page size
at the very least.

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > +     out_md->phys_addr       = info->mem.addr;
> >
> > This will be equal to phys_addr, not the actual start of the memory
> > region.
> >
> > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> >
> > Similarly, this will be the number of bytes in the memory region
> > after phys_addr, not the total number of bytes in the region.  These two
> > differences mean that this function is not strictly equivalent to the
> > original efi_mem_desc_lookup().
> >
> > I am not sure if this matters in practice, but I thought you would want
> > to be aware of it.
> 
> This is a bit disappointing. Is there no way to obtain this
> information via a Xen hypercall?

I don't think so, unfortunately. That said, with the below adjustment, I
think that's okay for the _current_ users of efi_mem_desc_lookup().

> In any case, it means we'll need to round down phys_addr to page size
> at the very least.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 4820 bytes --]

On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > This means that some sanity checks we would like to perform on
> > > configuration tables or other data structures in memory are not
> > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > via a Xen hypercall, so let's wire that up instead.
> > >
> > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  drivers/firmware/efi/efi.c |  5 ++-
> > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > >  include/linux/efi.h        |  1 +
> > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 55bd3f4aab28..2c12b1a06481 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > >   * and if so, populate the supplied memory descriptor with the appropriate
> > >   * data.
> > >   */
> > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > >  {
> > >       efi_memory_desc_t *md;
> > >
> > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > >       return -ENOENT;
> > >  }
> > >
> > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > +      __weak __alias(__efi_mem_desc_lookup);
> > > +
> > >  /*
> > >   * Calculate the highest address of an efi memory descriptor.
> > >   */
> > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > --- a/drivers/xen/efi.c
> > > +++ b/drivers/xen/efi.c
> > > @@ -26,6 +26,7 @@
> > >
> > >  #include <xen/interface/xen.h>
> > >  #include <xen/interface/platform.h>
> > > +#include <xen/page.h>
> > >  #include <xen/xen.h>
> > >  #include <xen/xen-ops.h>
> > >
> > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > >       efi.reset_system                = xen_efi_reset_system;
> > >  }
> > > +
> > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > +{
> > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > +     struct xen_platform_op op = {
> > > +             .cmd = XENPF_firmware_info,
> > > +             .u.firmware_info = {
> > > +                     .type = XEN_FW_EFI_INFO,
> > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > +                     .u.efi_info.mem.addr = phys_addr,
> > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > +             }
> > > +     };
> > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > +     int rc;
> > > +
> > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > +
> > > +     rc = HYPERVISOR_platform_op(&op);
> > > +     if (rc) {
> > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > +                     phys_addr, rc);
> > > +     }
> > > +
> > > +     out_md->phys_addr       = info->mem.addr;
> >
> > This will be equal to phys_addr, not the actual start of the memory
> > region.
> >
> > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> >
> > Similarly, this will be the number of bytes in the memory region
> > after phys_addr, not the total number of bytes in the region.  These two
> > differences mean that this function is not strictly equivalent to the
> > original efi_mem_desc_lookup().
> >
> > I am not sure if this matters in practice, but I thought you would want
> > to be aware of it.
> 
> This is a bit disappointing. Is there no way to obtain this
> information via a Xen hypercall?

It is possible, but doing so is very complex (it essentially requires a
binary search).  This really should be fixed on the Xen side.

> In any case, it means we'll need to round down phys_addr to page size
> at the very least.

That makes sense.  Are there any callers that will be broken even with
this rounding?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Ard Biesheuvel]

On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > <demi@invisiblethingslab.com> wrote:
> > >
> > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > This means that some sanity checks we would like to perform on
> > > > configuration tables or other data structures in memory are not
> > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > via a Xen hypercall, so let's wire that up instead.
> > > >
> > > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > > >  include/linux/efi.h        |  1 +
> > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > --- a/drivers/firmware/efi/efi.c
> > > > +++ b/drivers/firmware/efi/efi.c
> > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > >   * and if so, populate the supplied memory descriptor with the appropriate
> > > >   * data.
> > > >   */
> > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > >  {
> > > >       efi_memory_desc_t *md;
> > > >
> > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > >       return -ENOENT;
> > > >  }
> > > >
> > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > +      __weak __alias(__efi_mem_desc_lookup);
> > > > +
> > > >  /*
> > > >   * Calculate the highest address of an efi memory descriptor.
> > > >   */
> > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > --- a/drivers/xen/efi.c
> > > > +++ b/drivers/xen/efi.c
> > > > @@ -26,6 +26,7 @@
> > > >
> > > >  #include <xen/interface/xen.h>
> > > >  #include <xen/interface/platform.h>
> > > > +#include <xen/page.h>
> > > >  #include <xen/xen.h>
> > > >  #include <xen/xen-ops.h>
> > > >
> > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > > >       efi.reset_system                = xen_efi_reset_system;
> > > >  }
> > > > +
> > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > +{
> > > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > > +     struct xen_platform_op op = {
> > > > +             .cmd = XENPF_firmware_info,
> > > > +             .u.firmware_info = {
> > > > +                     .type = XEN_FW_EFI_INFO,
> > > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > > +                     .u.efi_info.mem.addr = phys_addr,
> > > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > +             }
> > > > +     };
> > > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > > +     int rc;
> > > > +
> > > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > +
> > > > +     rc = HYPERVISOR_platform_op(&op);
> > > > +     if (rc) {
> > > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > > +                     phys_addr, rc);
> > > > +     }
> > > > +
> > > > +     out_md->phys_addr       = info->mem.addr;
> > >
> > > This will be equal to phys_addr, not the actual start of the memory
> > > region.
> > >
> > > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> > >
> > > Similarly, this will be the number of bytes in the memory region
> > > after phys_addr, not the total number of bytes in the region.  These two
> > > differences mean that this function is not strictly equivalent to the
> > > original efi_mem_desc_lookup().
> > >
> > > I am not sure if this matters in practice, but I thought you would want
> > > to be aware of it.
> >
> > This is a bit disappointing. Is there no way to obtain this
> > information via a Xen hypercall?
>
> It is possible, but doing so is very complex (it essentially requires a
> binary search).  This really should be fixed on the Xen side.
>
> > In any case, it means we'll need to round down phys_addr to page size
> > at the very least.
>
> That makes sense.  Are there any callers that will be broken even with
> this rounding?

As far as I can tell, it should work fine. The only thing to double
check is whether we are not creating spurious error messages from
efi_arch_mem_reserve() this way, but as far as I can tell, that should
be fine too.

Is there anyone at your end that can give this a spin on an actual
Xen/x86 system?

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 5954 bytes --]

On Mon, Oct 03, 2022 at 06:37:19PM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > > <demi@invisiblethingslab.com> wrote:
> > > >
> > > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > > This means that some sanity checks we would like to perform on
> > > > > configuration tables or other data structures in memory are not
> > > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > > via a Xen hypercall, so let's wire that up instead.
> > > > >
> > > > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > ---
> > > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > > > >  include/linux/efi.h        |  1 +
> > > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > > --- a/drivers/firmware/efi/efi.c
> > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > > >   * and if so, populate the supplied memory descriptor with the appropriate
> > > > >   * data.
> > > > >   */
> > > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > >  {
> > > > >       efi_memory_desc_t *md;
> > > > >
> > > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > >       return -ENOENT;
> > > > >  }
> > > > >
> > > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > +      __weak __alias(__efi_mem_desc_lookup);
> > > > > +
> > > > >  /*
> > > > >   * Calculate the highest address of an efi memory descriptor.
> > > > >   */
> > > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > > --- a/drivers/xen/efi.c
> > > > > +++ b/drivers/xen/efi.c
> > > > > @@ -26,6 +26,7 @@
> > > > >
> > > > >  #include <xen/interface/xen.h>
> > > > >  #include <xen/interface/platform.h>
> > > > > +#include <xen/page.h>
> > > > >  #include <xen/xen.h>
> > > > >  #include <xen/xen-ops.h>
> > > > >
> > > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > > > >       efi.reset_system                = xen_efi_reset_system;
> > > > >  }
> > > > > +
> > > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > +{
> > > > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > > > +     struct xen_platform_op op = {
> > > > > +             .cmd = XENPF_firmware_info,
> > > > > +             .u.firmware_info = {
> > > > > +                     .type = XEN_FW_EFI_INFO,
> > > > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > > > +                     .u.efi_info.mem.addr = phys_addr,
> > > > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > > +             }
> > > > > +     };
> > > > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > > > +     int rc;
> > > > > +
> > > > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > > +
> > > > > +     rc = HYPERVISOR_platform_op(&op);
> > > > > +     if (rc) {
> > > > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > > > +                     phys_addr, rc);
> > > > > +     }
> > > > > +
> > > > > +     out_md->phys_addr       = info->mem.addr;
> > > >
> > > > This will be equal to phys_addr, not the actual start of the memory
> > > > region.
> > > >
> > > > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> > > >
> > > > Similarly, this will be the number of bytes in the memory region
> > > > after phys_addr, not the total number of bytes in the region.  These two
> > > > differences mean that this function is not strictly equivalent to the
> > > > original efi_mem_desc_lookup().
> > > >
> > > > I am not sure if this matters in practice, but I thought you would want
> > > > to be aware of it.
> > >
> > > This is a bit disappointing. Is there no way to obtain this
> > > information via a Xen hypercall?
> >
> > It is possible, but doing so is very complex (it essentially requires a
> > binary search).  This really should be fixed on the Xen side.
> >
> > > In any case, it means we'll need to round down phys_addr to page size
> > > at the very least.
> >
> > That makes sense.  Are there any callers that will be broken even with
> > this rounding?
> 
> As far as I can tell, it should work fine. The only thing to double
> check is whether we are not creating spurious error messages from
> efi_arch_mem_reserve() this way, but as far as I can tell, that should
> be fine too.
> 
> Is there anyone at your end that can give this a spin on an actual
> Xen/x86 system?

Demi, if you open a PR with this at
https://github.com/QubesOS/qubes-linux-kernel/pulls, I can run it
through our CI - (at least) one of the machines has ESRT table. AFAIR
your test laptop has it too.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 6329 bytes --]

On Mon, Oct 03, 2022 at 07:04:02PM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Oct 03, 2022 at 06:37:19PM +0200, Ard Biesheuvel wrote:
> > On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
> > <demi@invisiblethingslab.com> wrote:
> > >
> > > On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > > > <demi@invisiblethingslab.com> wrote:
> > > > >
> > > > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > > > This means that some sanity checks we would like to perform on
> > > > > > configuration tables or other data structures in memory are not
> > > > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > > > via a Xen hypercall, so let's wire that up instead.
> > > > > >
> > > > > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > ---
> > > > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > > > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > > > > >  include/linux/efi.h        |  1 +
> > > > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > > > --- a/drivers/firmware/efi/efi.c
> > > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > > > >   * and if so, populate the supplied memory descriptor with the appropriate
> > > > > >   * data.
> > > > > >   */
> > > > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > >  {
> > > > > >       efi_memory_desc_t *md;
> > > > > >
> > > > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > >       return -ENOENT;
> > > > > >  }
> > > > > >
> > > > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > +      __weak __alias(__efi_mem_desc_lookup);
> > > > > > +
> > > > > >  /*
> > > > > >   * Calculate the highest address of an efi memory descriptor.
> > > > > >   */
> > > > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > > > --- a/drivers/xen/efi.c
> > > > > > +++ b/drivers/xen/efi.c
> > > > > > @@ -26,6 +26,7 @@
> > > > > >
> > > > > >  #include <xen/interface/xen.h>
> > > > > >  #include <xen/interface/platform.h>
> > > > > > +#include <xen/page.h>
> > > > > >  #include <xen/xen.h>
> > > > > >  #include <xen/xen-ops.h>
> > > > > >
> > > > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > > > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > > > > >       efi.reset_system                = xen_efi_reset_system;
> > > > > >  }
> > > > > > +
> > > > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > +{
> > > > > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > > > > +     struct xen_platform_op op = {
> > > > > > +             .cmd = XENPF_firmware_info,
> > > > > > +             .u.firmware_info = {
> > > > > > +                     .type = XEN_FW_EFI_INFO,
> > > > > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > > > > +                     .u.efi_info.mem.addr = phys_addr,
> > > > > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > > > +             }
> > > > > > +     };
> > > > > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > > > > +     int rc;
> > > > > > +
> > > > > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > > > +
> > > > > > +     rc = HYPERVISOR_platform_op(&op);
> > > > > > +     if (rc) {
> > > > > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > > > > +                     phys_addr, rc);
> > > > > > +     }
> > > > > > +
> > > > > > +     out_md->phys_addr       = info->mem.addr;
> > > > >
> > > > > This will be equal to phys_addr, not the actual start of the memory
> > > > > region.
> > > > >
> > > > > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> > > > >
> > > > > Similarly, this will be the number of bytes in the memory region
> > > > > after phys_addr, not the total number of bytes in the region.  These two
> > > > > differences mean that this function is not strictly equivalent to the
> > > > > original efi_mem_desc_lookup().
> > > > >
> > > > > I am not sure if this matters in practice, but I thought you would want
> > > > > to be aware of it.
> > > >
> > > > This is a bit disappointing. Is there no way to obtain this
> > > > information via a Xen hypercall?
> > >
> > > It is possible, but doing so is very complex (it essentially requires a
> > > binary search).  This really should be fixed on the Xen side.
> > >
> > > > In any case, it means we'll need to round down phys_addr to page size
> > > > at the very least.
> > >
> > > That makes sense.  Are there any callers that will be broken even with
> > > this rounding?
> > 
> > As far as I can tell, it should work fine. The only thing to double
> > check is whether we are not creating spurious error messages from
> > efi_arch_mem_reserve() this way, but as far as I can tell, that should
> > be fine too.
> > 
> > Is there anyone at your end that can give this a spin on an actual
> > Xen/x86 system?
> 
> Demi, if you open a PR with this at
> https://github.com/QubesOS/qubes-linux-kernel/pulls, I can run it
> through our CI - (at least) one of the machines has ESRT table. AFAIR
> your test laptop has it too.

Just this patch or the whole series?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 6676 bytes --]

On Mon, Oct 03, 2022 at 01:57:14PM -0400, Demi Marie Obenour wrote:
> On Mon, Oct 03, 2022 at 07:04:02PM +0200, Marek Marczykowski-Górecki wrote:
> > On Mon, Oct 03, 2022 at 06:37:19PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
> > > <demi@invisiblethingslab.com> wrote:
> > > >
> > > > On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > > > > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > > > > <demi@invisiblethingslab.com> wrote:
> > > > > >
> > > > > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > > > > This means that some sanity checks we would like to perform on
> > > > > > > configuration tables or other data structures in memory are not
> > > > > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > > > > via a Xen hypercall, so let's wire that up instead.
> > > > > > >
> > > > > > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > ---
> > > > > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > > > > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > > > > > >  include/linux/efi.h        |  1 +
> > > > > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > > > > --- a/drivers/firmware/efi/efi.c
> > > > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > > > > >   * and if so, populate the supplied memory descriptor with the appropriate
> > > > > > >   * data.
> > > > > > >   */
> > > > > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > >  {
> > > > > > >       efi_memory_desc_t *md;
> > > > > > >
> > > > > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > >       return -ENOENT;
> > > > > > >  }
> > > > > > >
> > > > > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > +      __weak __alias(__efi_mem_desc_lookup);
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * Calculate the highest address of an efi memory descriptor.
> > > > > > >   */
> > > > > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > > > > --- a/drivers/xen/efi.c
> > > > > > > +++ b/drivers/xen/efi.c
> > > > > > > @@ -26,6 +26,7 @@
> > > > > > >
> > > > > > >  #include <xen/interface/xen.h>
> > > > > > >  #include <xen/interface/platform.h>
> > > > > > > +#include <xen/page.h>
> > > > > > >  #include <xen/xen.h>
> > > > > > >  #include <xen/xen-ops.h>
> > > > > > >
> > > > > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > > > > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > > > > > >       efi.reset_system                = xen_efi_reset_system;
> > > > > > >  }
> > > > > > > +
> > > > > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > +{
> > > > > > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > > > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > > > > > +     struct xen_platform_op op = {
> > > > > > > +             .cmd = XENPF_firmware_info,
> > > > > > > +             .u.firmware_info = {
> > > > > > > +                     .type = XEN_FW_EFI_INFO,
> > > > > > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > > > > > +                     .u.efi_info.mem.addr = phys_addr,
> > > > > > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > > > > +             }
> > > > > > > +     };
> > > > > > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > > > > > +     int rc;
> > > > > > > +
> > > > > > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > > > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > > > > +
> > > > > > > +     rc = HYPERVISOR_platform_op(&op);
> > > > > > > +     if (rc) {
> > > > > > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > > > > > +                     phys_addr, rc);
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     out_md->phys_addr       = info->mem.addr;
> > > > > >
> > > > > > This will be equal to phys_addr, not the actual start of the memory
> > > > > > region.
> > > > > >
> > > > > > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> > > > > >
> > > > > > Similarly, this will be the number of bytes in the memory region
> > > > > > after phys_addr, not the total number of bytes in the region.  These two
> > > > > > differences mean that this function is not strictly equivalent to the
> > > > > > original efi_mem_desc_lookup().
> > > > > >
> > > > > > I am not sure if this matters in practice, but I thought you would want
> > > > > > to be aware of it.
> > > > >
> > > > > This is a bit disappointing. Is there no way to obtain this
> > > > > information via a Xen hypercall?
> > > >
> > > > It is possible, but doing so is very complex (it essentially requires a
> > > > binary search).  This really should be fixed on the Xen side.
> > > >
> > > > > In any case, it means we'll need to round down phys_addr to page size
> > > > > at the very least.
> > > >
> > > > That makes sense.  Are there any callers that will be broken even with
> > > > this rounding?
> > > 
> > > As far as I can tell, it should work fine. The only thing to double
> > > check is whether we are not creating spurious error messages from
> > > efi_arch_mem_reserve() this way, but as far as I can tell, that should
> > > be fine too.
> > > 
> > > Is there anyone at your end that can give this a spin on an actual
> > > Xen/x86 system?
> > 
> > Demi, if you open a PR with this at
> > https://github.com/QubesOS/qubes-linux-kernel/pulls, I can run it
> > through our CI - (at least) one of the machines has ESRT table. AFAIR
> > your test laptop has it too.
> 
> Just this patch or the whole series?

Whole series.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 6422 bytes --]

On Mon, Oct 03, 2022 at 07:04:02PM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Oct 03, 2022 at 06:37:19PM +0200, Ard Biesheuvel wrote:
> > On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
> > <demi@invisiblethingslab.com> wrote:
> > >
> > > On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > > > <demi@invisiblethingslab.com> wrote:
> > > > >
> > > > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > > > This means that some sanity checks we would like to perform on
> > > > > > configuration tables or other data structures in memory are not
> > > > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > > > via a Xen hypercall, so let's wire that up instead.
> > > > > >
> > > > > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > ---
> > > > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > > > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > > > > >  include/linux/efi.h        |  1 +
> > > > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > > > --- a/drivers/firmware/efi/efi.c
> > > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > > > >   * and if so, populate the supplied memory descriptor with the appropriate
> > > > > >   * data.
> > > > > >   */
> > > > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > >  {
> > > > > >       efi_memory_desc_t *md;
> > > > > >
> > > > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > >       return -ENOENT;
> > > > > >  }
> > > > > >
> > > > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > +      __weak __alias(__efi_mem_desc_lookup);
> > > > > > +
> > > > > >  /*
> > > > > >   * Calculate the highest address of an efi memory descriptor.
> > > > > >   */
> > > > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > > > --- a/drivers/xen/efi.c
> > > > > > +++ b/drivers/xen/efi.c
> > > > > > @@ -26,6 +26,7 @@
> > > > > >
> > > > > >  #include <xen/interface/xen.h>
> > > > > >  #include <xen/interface/platform.h>
> > > > > > +#include <xen/page.h>
> > > > > >  #include <xen/xen.h>
> > > > > >  #include <xen/xen-ops.h>
> > > > > >
> > > > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > > > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > > > > >       efi.reset_system                = xen_efi_reset_system;
> > > > > >  }
> > > > > > +
> > > > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > +{
> > > > > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > > > > +     struct xen_platform_op op = {
> > > > > > +             .cmd = XENPF_firmware_info,
> > > > > > +             .u.firmware_info = {
> > > > > > +                     .type = XEN_FW_EFI_INFO,
> > > > > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > > > > +                     .u.efi_info.mem.addr = phys_addr,
> > > > > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > > > +             }
> > > > > > +     };
> > > > > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > > > > +     int rc;
> > > > > > +
> > > > > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > > > +
> > > > > > +     rc = HYPERVISOR_platform_op(&op);
> > > > > > +     if (rc) {
> > > > > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > > > > +                     phys_addr, rc);
> > > > > > +     }
> > > > > > +
> > > > > > +     out_md->phys_addr       = info->mem.addr;
> > > > >
> > > > > This will be equal to phys_addr, not the actual start of the memory
> > > > > region.
> > > > >
> > > > > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> > > > >
> > > > > Similarly, this will be the number of bytes in the memory region
> > > > > after phys_addr, not the total number of bytes in the region.  These two
> > > > > differences mean that this function is not strictly equivalent to the
> > > > > original efi_mem_desc_lookup().
> > > > >
> > > > > I am not sure if this matters in practice, but I thought you would want
> > > > > to be aware of it.
> > > >
> > > > This is a bit disappointing. Is there no way to obtain this
> > > > information via a Xen hypercall?
> > >
> > > It is possible, but doing so is very complex (it essentially requires a
> > > binary search).  This really should be fixed on the Xen side.
> > >
> > > > In any case, it means we'll need to round down phys_addr to page size
> > > > at the very least.
> > >
> > > That makes sense.  Are there any callers that will be broken even with
> > > this rounding?
> > 
> > As far as I can tell, it should work fine. The only thing to double
> > check is whether we are not creating spurious error messages from
> > efi_arch_mem_reserve() this way, but as far as I can tell, that should
> > be fine too.
> > 
> > Is there anyone at your end that can give this a spin on an actual
> > Xen/x86 system?
> 
> Demi, if you open a PR with this at
> https://github.com/QubesOS/qubes-linux-kernel/pulls, I can run it
> through our CI - (at least) one of the machines has ESRT table.

Done: https://github.com/QubesOS/qubes-linux-kernel/pull/681

> AFAIR your test laptop has it too.

It does; I plan to test a version that has the needed rounding.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/6] efi: Apply allowlist to EFI configuration tables when running under Xen

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 3723 bytes --]

On Mon, Oct 03, 2022 at 01:26:25PM +0200, Ard Biesheuvel wrote:
> As it turns out, Xen does not guarantee that EFI bootservices data
> regions in memory are preserved, which means that EFI configuration
> tables pointing into such memory regions may be corrupted before the
> dom0 OS has had a chance to inspect them.
> 
> Demi Marie reports that this is causing problems for Qubes OS when it
> attempts to perform system firmware updates, which requires that the
> contents of the ESRT configuration table are valid when the fwupd user
> space program runs.
> 
> However, other configuration tables such as the memory attributes
> table or the runtime properties table are equally affected, and so we
> need a comprehensive workaround that works for any table type.
> 
> So when running under Xen, check the EFI memory descriptor covering the
> start of the table, and disregard the table if it does not reside in
> memory that is preserved by Xen.
> 
> Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/efi.c |  7 ++++++
>  drivers/xen/efi.c          | 24 ++++++++++++++++++++
>  include/linux/efi.h        |  2 ++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 2c12b1a06481..0a4583c13a40 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -560,6 +560,13 @@ static __init int match_config_table(const efi_guid_t *guid,
>  
>  	for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) {
>  		if (!efi_guidcmp(*guid, table_types[i].guid)) {
> +			if (IS_ENABLED(CONFIG_XEN_EFI) &&
> +			    !xen_efi_config_table_is_usable(guid, table)) {
> +				if (table_types[i].name[0])
> +					pr_cont("(%s=0x%lx) ",
> +						table_types[i].name, table);
> +				return 1;
> +			}
>  			*(table_types[i].ptr) = table;
>  			if (table_types[i].name[0])
>  				pr_cont("%s=0x%lx ",
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index 74f3f6d8cdc8..c275a9c377fe 100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -326,3 +326,27 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  
>          return 0;
>  }
> +
> +bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
> +					   unsigned long table)
> +{
> +	efi_memory_desc_t md;
> +	int rc;
> +
> +	if (!efi_enabled(EFI_PARAVIRT))
> +		return true;
> +
> +	rc = efi_mem_desc_lookup(table, &md);
> +	if (rc)
> +		return false;
> +
> +	switch (md.type) {
> +	case EFI_RUNTIME_SERVICES_CODE:
> +	case EFI_RUNTIME_SERVICES_DATA:
> +	case EFI_ACPI_RECLAIM_MEMORY:
> +	case EFI_RESERVED_TYPE:

Some firmware uses EFI_ACPI_MEMORY_NVS to store ACPI tables, so this
needs to be added to the allowlist.  Otherwise affected systems will not
boot.  Xen treats EFI_ACPI_MEMORY_NVS the way it treats
EFI_ACPI_RECLAIM_MEMORY, so this is safe.

> +		return true;
> +	}
> +
> +	return false;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index e0ee6f6da4b4..b0cba86352ce 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1352,4 +1352,6 @@ struct linux_efi_initrd {
>  /* Header of a populated EFI secret area */
>  #define EFI_SECRET_TABLE_HEADER_GUID	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66,  0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
>  
> +bool xen_efi_config_table_is_usable(const efi_guid_t *, unsigned long table);
> +
>  #endif /* _LINUX_EFI_H */
> -- 
> 2.35.1
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/6] efi: xen: Implement memory descriptor lookup based on hypercall

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 7442 bytes --]

On Mon, Oct 03, 2022 at 08:01:03PM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Oct 03, 2022 at 01:57:14PM -0400, Demi Marie Obenour wrote:
> > On Mon, Oct 03, 2022 at 07:04:02PM +0200, Marek Marczykowski-Górecki wrote:
> > > On Mon, Oct 03, 2022 at 06:37:19PM +0200, Ard Biesheuvel wrote:
> > > > On Mon, 3 Oct 2022 at 18:23, Demi Marie Obenour
> > > > <demi@invisiblethingslab.com> wrote:
> > > > >
> > > > > On Mon, Oct 03, 2022 at 05:59:52PM +0200, Ard Biesheuvel wrote:
> > > > > > On Mon, 3 Oct 2022 at 17:29, Demi Marie Obenour
> > > > > > <demi@invisiblethingslab.com> wrote:
> > > > > > >
> > > > > > > On Mon, Oct 03, 2022 at 01:26:24PM +0200, Ard Biesheuvel wrote:
> > > > > > > > Xen on x86 boots dom0 in EFI mode but without providing a memory map.
> > > > > > > > This means that some sanity checks we would like to perform on
> > > > > > > > configuration tables or other data structures in memory are not
> > > > > > > > currently possible. Xen does, however, expose EFI memory descriptor info
> > > > > > > > via a Xen hypercall, so let's wire that up instead.
> > > > > > > >
> > > > > > > > Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > > ---
> > > > > > > >  drivers/firmware/efi/efi.c |  5 ++-
> > > > > > > >  drivers/xen/efi.c          | 34 ++++++++++++++++++++
> > > > > > > >  include/linux/efi.h        |  1 +
> > > > > > > >  3 files changed, 39 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > > > > index 55bd3f4aab28..2c12b1a06481 100644
> > > > > > > > --- a/drivers/firmware/efi/efi.c
> > > > > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > > > > @@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
> > > > > > > >   * and if so, populate the supplied memory descriptor with the appropriate
> > > > > > > >   * data.
> > > > > > > >   */
> > > > > > > > -int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > > +int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > >  {
> > > > > > > >       efi_memory_desc_t *md;
> > > > > > > >
> > > > > > > > @@ -485,6 +485,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > >       return -ENOENT;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > > +      __weak __alias(__efi_mem_desc_lookup);
> > > > > > > > +
> > > > > > > >  /*
> > > > > > > >   * Calculate the highest address of an efi memory descriptor.
> > > > > > > >   */
> > > > > > > > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > > > > > > > index d1ff2186ebb4..74f3f6d8cdc8 100644
> > > > > > > > --- a/drivers/xen/efi.c
> > > > > > > > +++ b/drivers/xen/efi.c
> > > > > > > > @@ -26,6 +26,7 @@
> > > > > > > >
> > > > > > > >  #include <xen/interface/xen.h>
> > > > > > > >  #include <xen/interface/platform.h>
> > > > > > > > +#include <xen/page.h>
> > > > > > > >  #include <xen/xen.h>
> > > > > > > >  #include <xen/xen-ops.h>
> > > > > > > >
> > > > > > > > @@ -292,3 +293,36 @@ void __init xen_efi_runtime_setup(void)
> > > > > > > >       efi.get_next_high_mono_count    = xen_efi_get_next_high_mono_count;
> > > > > > > >       efi.reset_system                = xen_efi_reset_system;
> > > > > > > >  }
> > > > > > > > +
> > > > > > > > +int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
> > > > > > > > +{
> > > > > > > > +     static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> > > > > > > > +                   "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> > > > > > > > +     struct xen_platform_op op = {
> > > > > > > > +             .cmd = XENPF_firmware_info,
> > > > > > > > +             .u.firmware_info = {
> > > > > > > > +                     .type = XEN_FW_EFI_INFO,
> > > > > > > > +                     .index = XEN_FW_EFI_MEM_INFO,
> > > > > > > > +                     .u.efi_info.mem.addr = phys_addr,
> > > > > > > > +                     .u.efi_info.mem.size = U64_MAX - phys_addr,
> > > > > > > > +             }
> > > > > > > > +     };
> > > > > > > > +     union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > > > > > > > +     int rc;
> > > > > > > > +
> > > > > > > > +     if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
> > > > > > > > +             return __efi_mem_desc_lookup(phys_addr, out_md);
> > > > > > > > +
> > > > > > > > +     rc = HYPERVISOR_platform_op(&op);
> > > > > > > > +     if (rc) {
> > > > > > > > +             pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
> > > > > > > > +                     phys_addr, rc);
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     out_md->phys_addr       = info->mem.addr;
> > > > > > >
> > > > > > > This will be equal to phys_addr, not the actual start of the memory
> > > > > > > region.
> > > > > > >
> > > > > > > > +     out_md->num_pages       = info->mem.size >> EFI_PAGE_SHIFT;
> > > > > > >
> > > > > > > Similarly, this will be the number of bytes in the memory region
> > > > > > > after phys_addr, not the total number of bytes in the region.  These two
> > > > > > > differences mean that this function is not strictly equivalent to the
> > > > > > > original efi_mem_desc_lookup().
> > > > > > >
> > > > > > > I am not sure if this matters in practice, but I thought you would want
> > > > > > > to be aware of it.
> > > > > >
> > > > > > This is a bit disappointing. Is there no way to obtain this
> > > > > > information via a Xen hypercall?
> > > > >
> > > > > It is possible, but doing so is very complex (it essentially requires a
> > > > > binary search).  This really should be fixed on the Xen side.
> > > > >
> > > > > > In any case, it means we'll need to round down phys_addr to page size
> > > > > > at the very least.
> > > > >
> > > > > That makes sense.  Are there any callers that will be broken even with
> > > > > this rounding?
> > > > 
> > > > As far as I can tell, it should work fine. The only thing to double
> > > > check is whether we are not creating spurious error messages from
> > > > efi_arch_mem_reserve() this way, but as far as I can tell, that should
> > > > be fine too.
> > > > 
> > > > Is there anyone at your end that can give this a spin on an actual
> > > > Xen/x86 system?
> > > 
> > > Demi, if you open a PR with this at
> > > https://github.com/QubesOS/qubes-linux-kernel/pulls, I can run it
> > > through our CI - (at least) one of the machines has ESRT table. AFAIR
> > > your test laptop has it too.
> > 
> > Just this patch or the whole series?
> 
> Whole series.

I have tested the series as in
https://github.com/QubesOS/qubes-linux-kernel/pull/681 and it seems to
work great.
Note the series there differs from this thread, and is marked as "v3" - I
assume (but haven't verified) it has changes requested in this thread
applied. Demi, can you confirm? If so, you can probably send this v3,
and feel free to include my Tested-by (unless you make significant
changes, ofc).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 0/5] efi: Support ESRT under Xen

[Demi Marie Obenour]

This patch series fixes handling of EFI tables when running under Xen.
These fixes allow the ESRT to be loaded when running paravirtualized in
dom0, making the use of EFI capsule updates possible.

Demi Marie Obenour (5):
  efi: memmap: Disregard bogus entries instead of returning them
  efi: xen: Implement memory descriptor lookup based on hypercall
  efi: Apply allowlist to EFI configuration tables when running under
    Xen
  efi: Actually enable the ESRT under Xen
  efi: Warn if trying to reserve memory under Xen

 drivers/firmware/efi/efi.c  | 22 ++++++++++++-
 drivers/firmware/efi/esrt.c | 15 +++------
 drivers/xen/efi.c           | 61 +++++++++++++++++++++++++++++++++++++
 include/linux/efi.h         |  3 ++
 4 files changed, 90 insertions(+), 11 deletions(-)

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v3 1/5] efi: memmap: Disregard bogus entries instead of returning them

[Demi Marie Obenour]

The ESRT code currently contains two consistency checks on the memory
descriptor it obtains, but one of them is both incomplete and can only
trigger on invalid descriptors.

So let's drop these checks, and instead disregard descriptors entirely
if the start address is misaligned, or if the number of pages reaches
to or beyond the end of the address space.  Note that the memory map as
a whole could still be inconsistent: multiple entries might cover the
same area, or the address could be outside of the addressable PA space,
but validating that goes beyond the scope of these helpers.  Also note
that since the physical address space is never 64-bits wide, a
descriptor that includes the last page of memory is not valid.  This is
fortunate, since it means that a valid physical address will never be an
error pointer and that the length of a memory descriptor in bytes will
fit in a 64-bit unsigned integer.

Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 drivers/firmware/efi/efi.c  | 6 ++++++
 drivers/firmware/efi/esrt.c | 9 +--------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index a06decee51e064d78a39752436487279d0660609..780caea594e0ffce30abb69bddcccf3bacf25382 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -474,6 +474,12 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 		u64 size;
 		u64 end;
 
+		/* skip bogus entries (including empty ones) */
+		if ((md->phys_addr & (EFI_PAGE_SIZE - 1)) ||
+		    (md->num_pages <= 0) ||
+		    (md->num_pages > (U64_MAX - md->phys_addr) >> EFI_PAGE_SHIFT))
+			continue;
+
 		size = md->num_pages << EFI_PAGE_SHIFT;
 		end = md->phys_addr + size;
 		if (phys_addr >= md->phys_addr && phys_addr < end) {
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..fb9fb70e1004132eff50c712c6fca05f7aeb1d57 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -263,15 +263,8 @@ void __init efi_esrt_init(void)
 		return;
 	}
 
-	max = efi_mem_desc_end(&md);
-	if (max < efi.esrt) {
-		pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
-		       (void *)efi.esrt, (void *)max);
-		return;
-	}
-
+	max = efi_mem_desc_end(&md) - efi.esrt;
 	size = sizeof(*esrt);
-	max -= efi.esrt;
 
 	if (max < size) {
 		pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v3 2/5] efi: xen: Implement memory descriptor lookup based on hypercall

[Demi Marie Obenour]

Xen on x86 boots dom0 in EFI mode but without providing a memory map.
This means that some consistency checks we would like to perform on
configuration tables or other data structures in memory are not
currently possible.  Xen does, however, expose EFI memory descriptor
info via a Xen hypercall, so let's wire that up instead.  It turns out
that the returned information is not identical to what Linux's
efi_mem_desc_lookup would return: the address returned is the address
passed to the hypercall, and the size returned is the number of bytes
remaining in the configuration table.  However, none of the callers of
efi_mem_desc_lookup() currently care about this.  In the future, Xen may
gain a hypercall that returns the actual start address, which can be
used instead.

Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 drivers/firmware/efi/efi.c |  5 ++++-
 drivers/xen/efi.c          | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/efi.h        |  1 +
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 780caea594e0ffce30abb69bddcccf3bacf25382..bcb848e44e7b1350b10b7c0479c0b38d980fe37d 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -456,7 +456,7 @@ void __init efi_find_mirror(void)
  * and if so, populate the supplied memory descriptor with the appropriate
  * data.
  */
-int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 {
 	efi_memory_desc_t *md;
 
@@ -490,6 +490,9 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 	return -ENOENT;
 }
 
+extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+	__weak __alias(__efi_mem_desc_lookup);
+
 /*
  * Calculate the highest address of an efi memory descriptor.
  */
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..3c792353b7308f9c2bf0a888eda9f827aa9177f8 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -26,6 +26,7 @@
 
 #include <xen/interface/xen.h>
 #include <xen/interface/platform.h>
+#include <xen/page.h>
 #include <xen/xen.h>
 #include <xen/xen-ops.h>
 
@@ -292,3 +293,38 @@ void __init xen_efi_runtime_setup(void)
 	efi.get_next_high_mono_count	= xen_efi_get_next_high_mono_count;
 	efi.reset_system		= xen_efi_reset_system;
 }
+
+int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
+{
+	static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
+	              "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
+	struct xen_platform_op op;
+	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+	int rc;
+
+	if (!efi_enabled(EFI_PARAVIRT) || efi_enabled(EFI_MEMMAP))
+		return __efi_mem_desc_lookup(phys_addr, out_md);
+	phys_addr &= ~(u64)(EFI_PAGE_SIZE - 1);
+	op = (struct xen_platform_op) {
+		.cmd = XENPF_firmware_info,
+		.u.firmware_info = {
+			.type = XEN_FW_EFI_INFO,
+			.index = XEN_FW_EFI_MEM_INFO,
+			.u.efi_info.mem.addr = phys_addr,
+			.u.efi_info.mem.size = U64_MAX - phys_addr,
+		},
+	};
+
+	rc = HYPERVISOR_platform_op(&op);
+	if (rc) {
+		pr_warn("Failed to lookup header 0x%llx in Xen memory map: error %d\n",
+		        phys_addr, rc);
+	}
+
+	out_md->phys_addr	= info->mem.addr;
+	out_md->num_pages	= info->mem.size >> EFI_PAGE_SHIFT;
+	out_md->type    	= info->mem.type;
+	out_md->attribute	= info->mem.attr;
+
+	return 0;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f87b2f5db9f83db6f7488648fe99a8f8fc4fdf04..b407a302b730a6cc7481afa0f582360e59faf1e0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -724,6 +724,7 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern u64 efi_mem_desc_end(efi_memory_desc_t *md);
 extern int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
+extern int __efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
 extern void efi_mem_reserve(phys_addr_t addr, u64 size);
 extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v3 3/5] efi: Apply allowlist to EFI configuration tables when running under Xen

[Demi Marie Obenour]

As it turns out, Xen does not guarantee that EFI boot services data
regions in memory are preserved, which means that EFI configuration
tables pointing into such memory regions may be corrupted before the
dom0 OS has had a chance to inspect them.

This is causing problems for Qubes OS when it attempts to perform system
firmware updates, which requires that the contents of the EFI System
Resource Table are valid when the fwupd userspace program runs.

However, other configuration tables such as the memory attributes table
or the runtime properties table are equally affected, and so we need a
comprehensive workaround that works for any table type.

So when running under Xen, check the EFI memory descriptor covering the
start of the table, and disregard the table if it does not reside in
memory that is preserved by Xen.

Co-developed-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 drivers/firmware/efi/efi.c |  7 +++++++
 drivers/xen/efi.c          | 25 +++++++++++++++++++++++++
 include/linux/efi.h        |  2 ++
 3 files changed, 34 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index bcb848e44e7b1350b10b7c0479c0b38d980fe37d..b49fcde06ca0ff5347047666f38b9309bd9cfe26 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -564,6 +564,13 @@ static __init int match_config_table(const efi_guid_t *guid,
 
 	for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) {
 		if (!efi_guidcmp(*guid, table_types[i].guid)) {
+			if (IS_ENABLED(CONFIG_XEN_EFI) &&
+			    !xen_efi_config_table_is_usable(guid, table)) {
+				if (table_types[i].name[0])
+					pr_cont("(%s=0x%lx) may have been clobbered by Xen ",
+					        table_types[i].name, table);
+				return 1;
+			}
 			*(table_types[i].ptr) = table;
 			if (table_types[i].name[0])
 				pr_cont("%s=0x%lx ",
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index 3c792353b7308f9c2bf0a888eda9f827aa9177f8..fb321cd6415a40e8c4d0ad940611adcabe20ab97 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -328,3 +328,28 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
 
 	return 0;
 }
+
+bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
+                                           unsigned long table)
+{
+	efi_memory_desc_t md;
+	int rc;
+
+	if (!efi_enabled(EFI_PARAVIRT))
+		return true;
+
+	rc = efi_mem_desc_lookup(table, &md);
+	if (rc)
+		return false;
+
+	switch (md.type) {
+	case EFI_RUNTIME_SERVICES_CODE:
+	case EFI_RUNTIME_SERVICES_DATA:
+	case EFI_ACPI_RECLAIM_MEMORY:
+	case EFI_ACPI_MEMORY_NVS:
+	case EFI_RESERVED_TYPE:
+		return true;
+	default:
+		return false;
+	}
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index b407a302b730a6cc7481afa0f582360e59faf1e0..b210b50c4bdedaafcce6f63d44f57ff8329d1cfd 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1322,4 +1322,6 @@ struct linux_efi_coco_secret_area {
 /* Header of a populated EFI secret area */
 #define EFI_SECRET_TABLE_HEADER_GUID	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66,  0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
 
+bool xen_efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table);
+
 #endif /* _LINUX_EFI_H */
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v3 4/5] efi: Actually enable the ESRT under Xen

[Demi Marie Obenour]

The ESRT can be parsed if EFI_PARAVIRT is enabled, even if EFI_MEMMAP is
not.  Also allow the ESRT to be in reclaimable memory, as that is where
future Xen versions will put it.

Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 drivers/firmware/efi/esrt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index fb9fb70e1004132eff50c712c6fca05f7aeb1d57..87729c365be1a804bb84e0b1ab874042848327b4 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -247,7 +247,7 @@ void __init efi_esrt_init(void)
 	int rc;
 	phys_addr_t end;
 
-	if (!efi_enabled(EFI_MEMMAP))
+	if (!efi_enabled(EFI_MEMMAP) && !efi_enabled(EFI_PARAVIRT))
 		return;
 
 	pr_debug("esrt-init: loading.\n");
@@ -258,7 +258,9 @@ void __init efi_esrt_init(void)
 	if (rc < 0 ||
 	    (!(md.attribute & EFI_MEMORY_RUNTIME) &&
 	     md.type != EFI_BOOT_SERVICES_DATA &&
-	     md.type != EFI_RUNTIME_SERVICES_DATA)) {
+	     md.type != EFI_RUNTIME_SERVICES_DATA &&
+	     md.type != EFI_ACPI_RECLAIM_MEMORY &&
+	     md.type != EFI_ACPI_MEMORY_NVS)) {
 		pr_warn("ESRT header is not in the memory map.\n");
 		return;
 	}
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v3 5/5] efi: Warn if trying to reserve memory under Xen

[Demi Marie Obenour]

Doing so cannot work and should never happen.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 drivers/firmware/efi/efi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index b49fcde06ca0ff5347047666f38b9309bd9cfe26..902f323499d8acc4f2b846a78993eb201448acad 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -519,6 +519,10 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
  */
 void __init efi_mem_reserve(phys_addr_t addr, u64 size)
 {
+	/* efi_mem_reserve() does not work under Xen */
+	if (WARN_ON_ONCE(efi_enabled(EFI_PARAVIRT)))
+		return;
+
 	if (!memblock_is_region_reserved(addr, size))
 		memblock_reserve(addr, size);
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH v3 0/5] efi: Support ESRT under Xen

[Ard Biesheuvel]

On Thu, 19 Jan 2023 at 20:04, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> This patch series fixes handling of EFI tables when running under Xen.
> These fixes allow the ESRT to be loaded when running paravirtualized in
> dom0, making the use of EFI capsule updates possible.
>
> Demi Marie Obenour (5):
>   efi: memmap: Disregard bogus entries instead of returning them
>   efi: xen: Implement memory descriptor lookup based on hypercall
>   efi: Apply allowlist to EFI configuration tables when running under
>     Xen
>   efi: Actually enable the ESRT under Xen
>   efi: Warn if trying to reserve memory under Xen
>

I have given these a spin on a system with a dodgy ESRT (the region in
question is not covered by the memory map at all), and things are
exactly as broken before, which is good.

I have queued these up in efi/next now, they should appear in -next tomorrow.


>  drivers/firmware/efi/efi.c  | 22 ++++++++++++-
>  drivers/firmware/efi/esrt.c | 15 +++------
>  drivers/xen/efi.c           | 61 +++++++++++++++++++++++++++++++++++++
>  include/linux/efi.h         |  3 ++
>  4 files changed, 90 insertions(+), 11 deletions(-)
>
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab