* [RFC PATCH 1/5] efi: Move EFI fake memmap support into x86 arch tree
2022-10-02 9:56 [RFC PATCH 0/5] efi/x86: Avoid corrupted config tables under Xen Ard Biesheuvel
@ 2022-10-02 9:56 ` Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 2/5] efi: memmap: Move manipulation routines " Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 9:56 UTC (permalink / raw)
To: linux-efi
Cc: xen-devel, Ard Biesheuvel, Demi Marie Obenour, Peter Jones,
Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko,
Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [RFC PATCH 2/5] efi: memmap: Move manipulation routines into x86 arch tree
2022-10-02 9:56 [RFC PATCH 0/5] efi/x86: Avoid corrupted config tables under Xen Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 1/5] efi: Move EFI fake memmap support into x86 arch tree Ard Biesheuvel
@ 2022-10-02 9:56 ` Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 3/5] efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 9:56 UTC (permalink / raw)
To: linux-efi
Cc: xen-devel, Ard Biesheuvel, Demi Marie Obenour, Peter Jones,
Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko,
Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [RFC PATCH 3/5] efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures
2022-10-02 9:56 [RFC PATCH 0/5] efi/x86: Avoid corrupted config tables under Xen Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 1/5] efi: Move EFI fake memmap support into x86 arch tree Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 2/5] efi: memmap: Move manipulation routines " Ard Biesheuvel
@ 2022-10-02 9:56 ` Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen Ard Biesheuvel
2022-10-02 9:56 ` [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available Ard Biesheuvel
4 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 9:56 UTC (permalink / raw)
To: linux-efi
Cc: xen-devel, Ard Biesheuvel, Demi Marie Obenour, Peter Jones,
Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko,
Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen
2022-10-02 9:56 [RFC PATCH 0/5] efi/x86: Avoid corrupted config tables under Xen Ard Biesheuvel
` (2 preceding siblings ...)
2022-10-02 9:56 ` [RFC PATCH 3/5] efi: xen: Set EFI_PARAVIRT for Xen dom0 boot on all architectures Ard Biesheuvel
@ 2022-10-02 9:56 ` Ard Biesheuvel
2022-10-02 16:27 ` Demi Marie Obenour
2022-10-02 9:56 ` [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available Ard Biesheuvel
4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 9:56 UTC (permalink / raw)
To: linux-efi
Cc: xen-devel, Ard Biesheuvel, Demi Marie Obenour, Peter Jones,
Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko,
Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
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 let's first disregard all EFI configuration tables except the ones
that are known (or can be expected) to reside in memory regions of a
type that Xen preserves, i.e., ACPI tables (which are passed in
EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually
passed in EfiRuntimeServicesData regions, even though the UEFI spec only
mentions this as a recommendation). Then, cross reference unknown tables
against either the EFI memory map (if available) or do a Xen hypercall
to determine the memory type, and allow the config table if the type is
one that is guaranteed to be preserved.
Future patches can augment the logic in this routine to allow other
table types based on the size of the allocation, or based on a table
specific header size field.
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 | 69 ++++++++++++++++++++
include/linux/efi.h | 2 +
3 files changed, 78 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 11857af72859..e8c0747011d7 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -556,6 +556,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 d1ff2186ebb4..3f1f365b37d0 100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -292,3 +292,72 @@ 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;
}
+
+static const efi_guid_t cfg_table_allow_list[] __initconst = {
+ ACPI_20_TABLE_GUID,
+ ACPI_TABLE_GUID,
+ SMBIOS_TABLE_GUID,
+ SMBIOS3_TABLE_GUID,
+};
+
+bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
+ unsigned long table)
+{
+ u32 memtype;
+ int i, rc;
+
+ if (!efi_enabled(EFI_PARAVIRT))
+ return true;
+
+ for (i = 0; i < ARRAY_SIZE(cfg_table_allow_list); i++) {
+ if (!efi_guidcmp(*guid, cfg_table_allow_list[i]))
+ return true;
+ }
+
+ if (efi_enabled(EFI_MEMMAP)) {
+ /* check against the EFI memory map */
+ efi_memory_desc_t md;
+
+ rc = efi_mem_desc_lookup(table, &md);
+ if (rc) {
+ pr_warn("Failed to lookup header 0x%lx in EFI memory map (%d)\n",
+ table, rc);
+ return false;
+ }
+ memtype = md.type;
+ } else {
+ /* check against the Xen hypercall */
+ 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 = table,
+ .u.efi_info.mem.size = U64_MAX - table,
+ }
+ };
+ union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+
+ rc = HYPERVISOR_platform_op(&op);
+ if (rc) {
+ pr_warn("Failed to lookup header 0x%lx in Xen memory map (%d)\n",
+ table, rc);
+ return false;
+ }
+ memtype = info->mem.type;
+ }
+
+ switch (memtype) {
+ case EFI_RUNTIME_SERVICES_CODE:
+ case EFI_RUNTIME_SERVICES_DATA:
+ case EFI_ACPI_RECLAIM_MEMORY:
+ case EFI_RESERVED_TYPE:
+ return true;
+ case EFI_BOOT_SERVICES_DATA:
+ break;
+ default:
+ return false;
+ }
+
+ return false;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 256e70e42114..6edc627798b6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1351,4 +1351,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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen
2022-10-02 9:56 ` [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen Ard Biesheuvel
@ 2022-10-02 16:27 ` Demi Marie Obenour
2022-10-02 21:22 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Demi Marie Obenour @ 2022-10-02 16:27 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi
Cc: xen-devel, Peter Jones, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, Kees Cook, Anton Vorontsov, Colin Cross,
Tony Luck, Marek Marczykowski-Górecki
[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]
On Sun, Oct 02, 2022 at 11:56:25AM +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 let's first disregard all EFI configuration tables except the ones
> that are known (or can be expected) to reside in memory regions of a
> type that Xen preserves, i.e., ACPI tables (which are passed in
> EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually
> passed in EfiRuntimeServicesData regions, even though the UEFI spec only
> mentions this as a recommendation). Then, cross reference unknown tables
> against either the EFI memory map (if available) or do a Xen hypercall
> to determine the memory type, and allow the config table if the type is
> one that is guaranteed to be preserved.
>
> Future patches can augment the logic in this routine to allow other
> table types based on the size of the allocation, or based on a table
> specific header size field.
>
> 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 | 69 ++++++++++++++++++++
> include/linux/efi.h | 2 +
> 3 files changed, 78 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 11857af72859..e8c0747011d7 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -556,6 +556,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 d1ff2186ebb4..3f1f365b37d0 100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -292,3 +292,72 @@ 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;
> }
> +
> +static const efi_guid_t cfg_table_allow_list[] __initconst = {
> + ACPI_20_TABLE_GUID,
> + ACPI_TABLE_GUID,
> + SMBIOS_TABLE_GUID,
> + SMBIOS3_TABLE_GUID,
> +};
This allowlist seems redundant. Either the tables are already in memory
that Xen will preserve or they aren’t. In both cases the subsequent
code will do the right thing.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen
2022-10-02 16:27 ` Demi Marie Obenour
@ 2022-10-02 21:22 ` Ard Biesheuvel
2022-10-02 23:00 ` Demi Marie Obenour
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 21:22 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: linux-efi, xen-devel, Peter Jones, Juergen Gross,
Stefano Stabellini, Oleksandr Tyshchenko, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Sun, Oct 02, 2022 at 11:56:25AM +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 let's first disregard all EFI configuration tables except the ones
> > that are known (or can be expected) to reside in memory regions of a
> > type that Xen preserves, i.e., ACPI tables (which are passed in
> > EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually
> > passed in EfiRuntimeServicesData regions, even though the UEFI spec only
> > mentions this as a recommendation). Then, cross reference unknown tables
> > against either the EFI memory map (if available) or do a Xen hypercall
> > to determine the memory type, and allow the config table if the type is
> > one that is guaranteed to be preserved.
> >
> > Future patches can augment the logic in this routine to allow other
> > table types based on the size of the allocation, or based on a table
> > specific header size field.
> >
> > 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 | 69 ++++++++++++++++++++
> > include/linux/efi.h | 2 +
> > 3 files changed, 78 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 11857af72859..e8c0747011d7 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -556,6 +556,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 d1ff2186ebb4..3f1f365b37d0 100644
> > --- a/drivers/xen/efi.c
> > +++ b/drivers/xen/efi.c
> > @@ -292,3 +292,72 @@ 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;
> > }
> > +
> > +static const efi_guid_t cfg_table_allow_list[] __initconst = {
> > + ACPI_20_TABLE_GUID,
> > + ACPI_TABLE_GUID,
> > + SMBIOS_TABLE_GUID,
> > + SMBIOS3_TABLE_GUID,
> > +};
>
> This allowlist seems redundant. Either the tables are already in memory
> that Xen will preserve or they aren’t. In both cases the subsequent
> code will do the right thing.
Will it? Currently, Xen simply accepts all ACPI and SMBIOS tables,
regardless of what type of memory region they reside in (if any).
So what will happen with buggy firmware where the ACPI or SMBIOS
tables are not covered by the memory map at all? Currently, this works
fine but now, it will be rejected. And without ACPI tables, the boot
will not get far enough to even inform the user what is wrong. And
SMBIOS tables are used for platform quirks, which means they might be
essential for a platform to boot as well.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen
2022-10-02 21:22 ` Ard Biesheuvel
@ 2022-10-02 23:00 ` Demi Marie Obenour
0 siblings, 0 replies; 12+ messages in thread
From: Demi Marie Obenour @ 2022-10-02 23:00 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, xen-devel, Peter Jones, Juergen Gross,
Stefano Stabellini, Oleksandr Tyshchenko, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
[-- Attachment #1: Type: text/plain, Size: 5066 bytes --]
On Sun, Oct 02, 2022 at 11:22:58PM +0200, Ard Biesheuvel wrote:
> On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Sun, Oct 02, 2022 at 11:56:25AM +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 let's first disregard all EFI configuration tables except the ones
> > > that are known (or can be expected) to reside in memory regions of a
> > > type that Xen preserves, i.e., ACPI tables (which are passed in
> > > EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually
> > > passed in EfiRuntimeServicesData regions, even though the UEFI spec only
> > > mentions this as a recommendation). Then, cross reference unknown tables
> > > against either the EFI memory map (if available) or do a Xen hypercall
> > > to determine the memory type, and allow the config table if the type is
> > > one that is guaranteed to be preserved.
> > >
> > > Future patches can augment the logic in this routine to allow other
> > > table types based on the size of the allocation, or based on a table
> > > specific header size field.
> > >
> > > 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 | 69 ++++++++++++++++++++
> > > include/linux/efi.h | 2 +
> > > 3 files changed, 78 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 11857af72859..e8c0747011d7 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -556,6 +556,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 d1ff2186ebb4..3f1f365b37d0 100644
> > > --- a/drivers/xen/efi.c
> > > +++ b/drivers/xen/efi.c
> > > @@ -292,3 +292,72 @@ 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;
> > > }
> > > +
> > > +static const efi_guid_t cfg_table_allow_list[] __initconst = {
> > > + ACPI_20_TABLE_GUID,
> > > + ACPI_TABLE_GUID,
> > > + SMBIOS_TABLE_GUID,
> > > + SMBIOS3_TABLE_GUID,
> > > +};
> >
> > This allowlist seems redundant. Either the tables are already in memory
> > that Xen will preserve or they aren’t. In both cases the subsequent
> > code will do the right thing.
>
> Will it? Currently, Xen simply accepts all ACPI and SMBIOS tables,
> regardless of what type of memory region they reside in (if any).
>
> So what will happen with buggy firmware where the ACPI or SMBIOS
> tables are not covered by the memory map at all? Currently, this works
> fine but now, it will be rejected. And without ACPI tables, the boot
> will not get far enough to even inform the user what is wrong. And
> SMBIOS tables are used for platform quirks, which means they might be
> essential for a platform to boot as well.
If the tables are not in the memory map at all, I recommend
add_taint(TAINT_FIRMWARE_WORKAROUND) but otherwise continuing to boot.
If the tables are somewhere nonsensical, then the platform is FUBAR
anyway. Linux alone might be able to work by reserving the memory, but
under Xen that does not work.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available
2022-10-02 9:56 [RFC PATCH 0/5] efi/x86: Avoid corrupted config tables under Xen Ard Biesheuvel
` (3 preceding siblings ...)
2022-10-02 9:56 ` [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen Ard Biesheuvel
@ 2022-10-02 9:56 ` Ard Biesheuvel
2022-10-02 16:27 ` Demi Marie Obenour
4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 9:56 UTC (permalink / raw)
To: linux-efi
Cc: xen-devel, Ard Biesheuvel, Demi Marie Obenour, Peter Jones,
Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko,
Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
In order to permit the ESRT to be used when doing pseudo-EFI boot
without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
make the sanity checks optional based on whether the memory map is
available.
If additional validation is needed, it is up to the Xen EFI glue code to
implement this in its xen_efi_config_table_is_valid() helper, or provide
a EFI memory map like it does on other architectures.
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>
---
arch/x86/platform/efi/quirks.c | 3 +
drivers/firmware/efi/esrt.c | 61 +++++++++++---------
2 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index b0b848d6933a..9307be2f4afa 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -250,6 +250,9 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
int num_entries;
void *new;
+ if (!efi_enabled(EFI_MEMMAP))
+ return;
+
if (efi_mem_desc_lookup(addr, &md) ||
md.type != EFI_BOOT_SERVICES_DATA) {
pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e7..adb31fba45ae 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -243,40 +243,45 @@ void __init efi_esrt_init(void)
void *va;
struct efi_system_resource_table tmpesrt;
size_t size, max, entry_size, entries_size;
- efi_memory_desc_t md;
- int rc;
+ bool reserve_esrt;
phys_addr_t end;
- if (!efi_enabled(EFI_MEMMAP))
- return;
-
pr_debug("esrt-init: loading.\n");
if (!esrt_table_exists())
return;
- rc = efi_mem_desc_lookup(efi.esrt, &md);
- if (rc < 0 ||
- (!(md.attribute & EFI_MEMORY_RUNTIME) &&
- md.type != EFI_BOOT_SERVICES_DATA &&
- md.type != EFI_RUNTIME_SERVICES_DATA)) {
- pr_warn("ESRT header is not in the memory map.\n");
- return;
- }
+ size = sizeof(*esrt);
+ if (efi_enabled(EFI_MEMMAP)) {
+ efi_memory_desc_t md;
+ int rc;
+
+ rc = efi_mem_desc_lookup(efi.esrt, &md);
+ if (rc < 0 ||
+ (!(md.attribute & EFI_MEMORY_RUNTIME) &&
+ md.type != EFI_BOOT_SERVICES_DATA &&
+ md.type != EFI_RUNTIME_SERVICES_DATA)) {
+ pr_warn("ESRT header is not in the memory map.\n");
+ 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;
- }
+ reserve_esrt = (md.type == EFI_BOOT_SERVICES_DATA);
+ 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;
+ }
- size = sizeof(*esrt);
- max -= efi.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;
+ if (max < size) {
+ pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
+ size, max);
+ return;
+ }
+ } else {
+ reserve_esrt = true;
+ max = SIZE_MAX;
}
va = early_memremap(efi.esrt, size);
@@ -332,9 +337,11 @@ void __init efi_esrt_init(void)
esrt_data_size = size;
end = esrt_data + size;
- pr_info("Reserving ESRT space from %pa to %pa.\n", &esrt_data, &end);
- if (md.type == EFI_BOOT_SERVICES_DATA)
+ if (reserve_esrt) {
+ pr_info("Reserving ESRT space from %pa to %pa.\n", &esrt_data,
+ &end);
efi_mem_reserve(esrt_data, esrt_data_size);
+ }
pr_debug("esrt-init: loaded.\n");
}
--
2.35.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available
2022-10-02 9:56 ` [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available Ard Biesheuvel
@ 2022-10-02 16:27 ` Demi Marie Obenour
2022-10-02 21:43 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Demi Marie Obenour @ 2022-10-02 16:27 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi
Cc: xen-devel, Peter Jones, Juergen Gross, Stefano Stabellini,
Oleksandr Tyshchenko, Kees Cook, Anton Vorontsov, Colin Cross,
Tony Luck, Marek Marczykowski-Górecki
[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]
On Sun, Oct 02, 2022 at 11:56:26AM +0200, Ard Biesheuvel wrote:
> In order to permit the ESRT to be used when doing pseudo-EFI boot
> without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
> make the sanity checks optional based on whether the memory map is
> available.
>
> If additional validation is needed, it is up to the Xen EFI glue code to
> implement this in its xen_efi_config_table_is_valid() helper, or provide
> a EFI memory map like it does on other architectures.
I don’t like this. It is easy to use a hypercall to get the end of the
memory region containing the config table, which is what my one of my
previous patches actually does. Skipping all of the validation could
easily lead to a regression. I understand wanting to get Xen-specific
code out of esrt.c, but this isn’t the answer. Some sort of abstraction
over both cases would be a much better solution.
> 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>
> ---
> arch/x86/platform/efi/quirks.c | 3 +
> drivers/firmware/efi/esrt.c | 61 +++++++++++---------
> 2 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index b0b848d6933a..9307be2f4afa 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -250,6 +250,9 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
> int num_entries;
> void *new;
>
> + if (!efi_enabled(EFI_MEMMAP))
> + return;
> +
This function does not actually work under Xen, even if EFI_MEMMAP is
set. When running under Xen, either this function must never be
called (in which case there should be at least a WARN()), or it should
return an error that callers must check for.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available
2022-10-02 16:27 ` Demi Marie Obenour
@ 2022-10-02 21:43 ` Ard Biesheuvel
2022-10-03 8:41 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-02 21:43 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: linux-efi, xen-devel, Peter Jones, Juergen Gross,
Stefano Stabellini, Oleksandr Tyshchenko, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Sun, Oct 02, 2022 at 11:56:26AM +0200, Ard Biesheuvel wrote:
> > In order to permit the ESRT to be used when doing pseudo-EFI boot
> > without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
> > make the sanity checks optional based on whether the memory map is
> > available.
> >
> > If additional validation is needed, it is up to the Xen EFI glue code to
> > implement this in its xen_efi_config_table_is_valid() helper, or provide
> > a EFI memory map like it does on other architectures.
>
> I don’t like this. It is easy to use a hypercall to get the end of the
> memory region containing the config table, which is what my one of my
> previous patches actually does. Skipping all of the validation could
> easily lead to a regression.
I don't like putting Xen specific hacks left and right because Xen on
x86 cannot be bothered to provide an EFI memory map. And as for
regressions, ESRT does not work at all under Xen (given the lack of a
memory map) and so I fail to see how this could break a currently
working case.
> I understand wanting to get Xen-specific
> code out of esrt.c, but this isn’t the answer. Some sort of abstraction
> over both cases would be a much better solution.
>
We have such an abstraction already, it is called the EFI memory map.
So there are two options here:
- expose a EFI memory map
- add a ESRT specific check to xen_efi_config_table_is_valid() so that
the ESRT is withheld from dom0 if there is something wrong with it.
And frankly, the validation itself could use some attention as well:
"""
rc = efi_mem_desc_lookup(efi.esrt, &md);
...
max = efi_mem_desc_end(&md);
if (max < efi.esrt) {
"""
Unless I am missing something, this can never occur so the check is
pointless and the pr_err() that follows is unreachable.
Then we have
"""
size = sizeof(*esrt);
max -= efi.esrt;
if (max < size) {
"""
'size' is 16 bytes here, so the only way this can become true is if
the memory descriptor describes a region of 0 pages in length, which
is explicitly forbidden by the EFI spec. If such a descriptor exists
in spite of that, this is a memory map problem not a ESRT problem.
So actually, instead of making these checks conditional on EFI_MEMMAP
being set, I might just rip them out entirely and be done with it.
> > 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>
> > ---
> > arch/x86/platform/efi/quirks.c | 3 +
> > drivers/firmware/efi/esrt.c | 61 +++++++++++---------
> > 2 files changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> > index b0b848d6933a..9307be2f4afa 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -250,6 +250,9 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
> > int num_entries;
> > void *new;
> >
> > + if (!efi_enabled(EFI_MEMMAP))
> > + return;
> > +
>
> This function does not actually work under Xen, even if EFI_MEMMAP is
> set. When running under Xen, either this function must never be
> called (in which case there should be at least a WARN()), or it should
> return an error that callers must check for.
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available
2022-10-02 21:43 ` Ard Biesheuvel
@ 2022-10-03 8:41 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2022-10-03 8:41 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: linux-efi, xen-devel, Peter Jones, Juergen Gross,
Stefano Stabellini, Oleksandr Tyshchenko, Kees Cook,
Anton Vorontsov, Colin Cross, Tony Luck,
Marek Marczykowski-Górecki
On Sun, 2 Oct 2022 at 23:43, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Sun, Oct 02, 2022 at 11:56:26AM +0200, Ard Biesheuvel wrote:
> > > In order to permit the ESRT to be used when doing pseudo-EFI boot
> > > without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
> > > make the sanity checks optional based on whether the memory map is
> > > available.
> > >
> > > If additional validation is needed, it is up to the Xen EFI glue code to
> > > implement this in its xen_efi_config_table_is_valid() helper, or provide
> > > a EFI memory map like it does on other architectures.
> >
> > I don’t like this. It is easy to use a hypercall to get the end of the
> > memory region containing the config table, which is what my one of my
> > previous patches actually does. Skipping all of the validation could
> > easily lead to a regression.
>
> I don't like putting Xen specific hacks left and right because Xen on
> x86 cannot be bothered to provide an EFI memory map. And as for
> regressions, ESRT does not work at all under Xen (given the lack of a
> memory map) and so I fail to see how this could break a currently
> working case.
>
> > I understand wanting to get Xen-specific
> > code out of esrt.c, but this isn’t the answer. Some sort of abstraction
> > over both cases would be a much better solution.
> >
>
> We have such an abstraction already, it is called the EFI memory map.
>
> So there are two options here:
> - expose a EFI memory map
> - add a ESRT specific check to xen_efi_config_table_is_valid() so that
> the ESRT is withheld from dom0 if there is something wrong with it.
>
Actually, the obvious answer here is to implement
efi_mem_desc_lookup() for the EFI_PARAVIRT / !EFI_MEMMAP case. I'll
have a go at that and send a v2 shortly.
^ permalink raw reply [flat|nested] 12+ messages in thread