memory attribute table support

Recent UEFI versions expose permission attributes for runtime services memory regions, either in the UEFI memory map or in the separate memory attributes table. This allows the kernel to map these regions with stricter permissions, rather than the RWX permissions that are used by default. So wire this up in our mapping routine.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/include/asm/efi.h |  1 +
 arch/arm/kernel/efi.c      | 41 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

Quoted 6 lines · Ard Biesheuvel

Recent UEFI versions expose permission attributes for runtime services memory regions, either in the UEFI memory map or in the separate memory attributes table. This allows the kernel to map these regions with stricter permissions, rather than the RWX permissions that are used by default. So wire this up in our mapping routine.

Quoted 5 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/include/asm/efi.h |  1 +
 arch/arm/kernel/efi.c      | 41 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

Looks fine from an EFI perspective, but it would be nice for somebody else with ARM knowledge to ACK it.

Quoted 13 lines · Matt Fleming

Quoted 11 lines · Ard Biesheuvel

Recent UEFI versions expose permission attributes for runtime services memory regions, either in the UEFI memory map or in the separate memory attributes table. This allows the kernel to map these regions with stricter permissions, rather than the RWX permissions that are used by default. So wire this up in our mapping routine.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/include/asm/efi.h |  1 +
 arch/arm/kernel/efi.c      | 41 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

Quoted 2 lines

Looks fine from an EFI perspective, but it would be nice for somebody else with ARM knowledge to ACK it.

I agree.

Note that the whole memory attribute series needs to wait for some memremap() changes that I proposed for ARM (or at least the bits where we wire it up for arm64+ARM)

Recent UEFI versions expose permission attributes for runtime services memory regions, either in the UEFI memory map or in the separate memory attributes table. This allows the kernel to map these regions with stricter permissions, rather than the RWX permissions that are used by default. So wire this up in our mapping routine.

Note that in the absence of permission attributes, we still only map regions of type EFI_RUNTIME_SERVICE_CODE with the executable bit set. Also, we base the mapping attributes of EFI_MEMORY_MAPPED_IO on the type directly rather than on the absence of the EFI_MEMORY_WB attribute. This is more correct, but is also required for compatibility with the upcoming support for the Memory Attributes Table, which only carries permission attributes, not memory type attributes.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

arch/arm64/kernel/efi.c +16 -5

index b6abc852f2a1..3364408c154f 100644
@@ -24,15 +24,32 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 	/*
 	 * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
 	 * executable, everything else can be mapped with the XN bits
-	 * set.
+	 * set. Also take the new (optional) RO/XP bits into account.
 	 */
-	if ((md->attribute & EFI_MEMORY_WB) == 0)
+	if (md->type == EFI_MEMORY_MAPPED_IO)
 		prot_val = PROT_DEVICE_nGnRE;
-	else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
-		 !PAGE_ALIGNED(md->phys_addr))
+	else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr),
+			   "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?"))
+		/*
+		 * If the region is not aligned to the page size of the OS, we
+		 * can not use strict permissions, since that would also affect
+		 * the mapping attributes of the adjacent regions.
+		 */
 		prot_val = pgprot_val(PAGE_KERNEL_EXEC);
-	else
+	else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
+		 (EFI_MEMORY_XP | EFI_MEMORY_RO))
+		/* R-- */
+		prot_val = pgprot_val(PAGE_KERNEL_RO);
+	else if (md->attribute & EFI_MEMORY_RO)
+		/* R-X */
+		prot_val = pgprot_val(PAGE_KERNEL_ROX);

Quoted 51 lines · Ard Biesheuvel

Recent UEFI versions expose permission attributes for runtime services memory regions, either in the UEFI memory map or in the separate memory attributes table. This allows the kernel to map these regions with stricter permissions, rather than the RWX permissions that are used by default. So wire this up in our mapping routine.

Note that in the absence of permission attributes, we still only map regions of type EFI_RUNTIME_SERVICE_CODE with the executable bit set. Also, we base the mapping attributes of EFI_MEMORY_MAPPED_IO on the type directly rather than on the absence of the EFI_MEMORY_WB attribute. This is more correct, but is also required for compatibility with the upcoming support for the Memory Attributes Table, which only carries permission attributes, not memory type attributes.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

arch/arm64/kernel/efi.c +16 -5

index b6abc852f2a1..3364408c154f 100644
@@ -24,15 +24,32 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
      /*
       * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
       * executable, everything else can be mapped with the XN bits
-      * set.
+      * set. Also take the new (optional) RO/XP bits into account.
       */
-     if ((md->attribute & EFI_MEMORY_WB) == 0)
+     if (md->type == EFI_MEMORY_MAPPED_IO)
              prot_val = PROT_DEVICE_nGnRE;
-     else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
-              !PAGE_ALIGNED(md->phys_addr))
+     else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr),
+                        "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?"))
+             /*
+              * If the region is not aligned to the page size of the OS, we
+              * can not use strict permissions, since that would also affect
+              * the mapping attributes of the adjacent regions.
+              */
              prot_val = pgprot_val(PAGE_KERNEL_EXEC);
-     else
+     else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
+              (EFI_MEMORY_XP | EFI_MEMORY_RO))
+             /* R-- */
+             prot_val = pgprot_val(PAGE_KERNEL_RO);
+     else if (md->attribute & EFI_MEMORY_RO)
+             /* R-X */
+             prot_val = pgprot_val(PAGE_KERNEL_ROX);

arch/arm64/kernel/efi.c int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md) · line 55

Quoted 10 lines

arch/arm64/kernel/efi.c +6 -0

+     else if (md->attribute & EFI_MEMORY_XP ||
+              md->type != EFI_RUNTIME_SERVICES_CODE)
+             /* RW- */
              prot_val = pgprot_val(PAGE_KERNEL);
+     else
+             /* RWX */
+             prot_val = pgprot_val(PAGE_KERNEL_EXEC);
 
      create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
                         md->num_pages << EFI_PAGE_SHIFT,

The actual logic looks fine but it seems like there's quite a lot going on in this function which is fairly difficult to decipher with the if/else if clauses.

Would you be open to splitting this out a little? It's just a suggestion, but maybe something like this,

---

arch/arm64/kernel/efi.c +35 -26

index 3364408c154f..33a6da160a50 100644
@@ -17,39 +17,48 @@
 
 #include <asm/efi.h>
 
-int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
+/*
+ * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
+ * executable, everything else can be mapped with the XN bits
+ * set. Also take the new (optional) RO/XP bits into account.
+ */
+static __init pteval_t create_mapping_protection(efi_memory_desc_t *md)
 {
-       pteval_t prot_val;
+       u64 attr = md->attribute;
+       u32 type = md->type;
 
-       /*
-        * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
-        * executable, everything else can be mapped with the XN bits
-        * set. Also take the new (optional) RO/XP bits into account.
-        */
-       if (md->type == EFI_MEMORY_MAPPED_IO)
-               prot_val = PROT_DEVICE_nGnRE;
-       else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr),
-                          "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?"))
+       if (type == EFI_MEMORY_MAPPED_IO)
+               return PROT_DEVICE_nGnRE;
+
+       if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr),
+                     "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?"))
                /*
                 * If the region is not aligned to the page size of the OS, we
                 * can not use strict permissions, since that would also affect
                 * the mapping attributes of the adjacent regions.
                 */
-               prot_val = pgprot_val(PAGE_KERNEL_EXEC);
-       else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
-                (EFI_MEMORY_XP | EFI_MEMORY_RO))
-               /* R-- */
-               prot_val = pgprot_val(PAGE_KERNEL_RO);
-       else if (md->attribute & EFI_MEMORY_RO)
-               /* R-X */
-               prot_val = pgprot_val(PAGE_KERNEL_ROX);
-       else if (md->attribute & EFI_MEMORY_XP ||
-                md->type != EFI_RUNTIME_SERVICES_CODE)
-               /* RW- */
-               prot_val = pgprot_val(PAGE_KERNEL);
-       else
-               /* RWX */
-               prot_val = pgprot_val(PAGE_KERNEL_EXEC);
+               return pgprot_val(PAGE_KERNEL_EXEC);
+
+       /* R-- */
+       if ((attr & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
+           (EFI_MEMORY_XP | EFI_MEMORY_RO))
+               return pgprot_val(PAGE_KERNEL_RO);
+
+       /* R-X */
+       if (attr & EFI_MEMORY_RO)
+               return pgprot_val(PAGE_KERNEL_ROX);
+
+       /* RW- */
+       if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE)
+               return pgprot_val(PAGE_KERNEL);
+
+       /* RWX */
+       return pgprot_val(PAGE_KERNEL_EXEC);
+}
+
+int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
+{
+       pteval_t prot_val = create_mapping_protection(md);
 
        create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
                           md->num_pages << EFI_PAGE_SHIFT,

This implements shared support for discovering the presence of the Memory Attributes table, and for parsing and validating its contents.

The table is validated against the construction rules in the UEFI spec. Since this is a new table, it makes sense to complain if we encounter a table that does not follow those rules.

The parsing and validation routine takes a callback that can be specified per architecture, that gets passed each unique validated region, with the virtual address retrieved from the ordinary memory map.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/Makefile  |   2 +-
 drivers/firmware/efi/memattr.c | 137 ++++++++++++++++++++
 include/linux/efi.h            |   6 +
 3 files changed, 144 insertions(+), 1 deletion(-)

drivers/firmware/efi/Makefile +1 -1

index 62e654f255f4..d5be62399130 100644
@@ -9,7 +9,7 @@
 #
 KASAN_SANITIZE_runtime-wrappers.o	:= n
 
-obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o
+obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o memattr.o
 obj-$(CONFIG_EFI_VARS)			+= efivars.o
 obj-$(CONFIG_EFI_ESRT)			+= esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o

drivers/firmware/efi/memattr.c new file +0 -0

new file mode 100644
index 000000000000..059cf4522a7d

drivers/firmware/efi/memattr.c new file +22 -0

+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/memblock.h>
+
+#include <asm/early_ioremap.h>
+
+static int __initdata tbl_size;
+
+/*
+ * Reserve the memory associated with the Memory Attributes configuration
+ * table, if it exists.
+ */
+int __init efi_memattr_init(void)
+{
+	efi_memory_attributes_table_t *tbl;
+
+	if (efi.mem_attr_table == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	tbl = early_memremap(efi.mem_attr_table, sizeof(*tbl));
+	if (!tbl) {

drivers/firmware/efi/memattr.c new file +4 -0

+			*out = *in;
+			out->virt_addr = in_paddr +
+					 (md->virt_addr - md->phys_addr);
+			return true;

drivers/firmware/efi/memattr.c new file +34 -0

+
+	if (tbl_size <= sizeof(*tbl))
+		return 0;
+
+	tbl = memremap(efi.mem_attr_table, tbl_size, MEMREMAP_WB);
+	if (!tbl) {
+		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
+		       efi.mem_attr_table);
+		return -ENOMEM;
+	}
+
+	if (efi_enabled(EFI_DBG))
+		pr_info("Processing UEFI Memory Attributes table:\n");
+
+	for (i = ret = 0; ret == 0 && i < tbl->num_entries; i++) {
+		efi_memory_desc_t md;
+		unsigned long size;
+		bool valid;
+		char buf[64];
+
+		valid = validate_entry((void *)tbl->entry + i * tbl->desc_size,
+				       &md);
+		size = md.num_pages << EFI_PAGE_SHIFT;
+		if (efi_enabled(EFI_DBG) || !valid)
+			pr_info("  0x%012llx-0x%012llx %s\n", md.phys_addr,
+				md.phys_addr + size - 1,
+				efi_md_typeattr_format(buf, sizeof(buf), &md));
+
+		if (valid)
+			ret = fn(mm, &md);
+	}
+	memunmap(tbl);
+	return ret;
+}

include/linux/efi.h +0 -0

index 808d97299c70..94e41872e5e6 100644

include/linux/efi.h +4 -0

+extern int efi_memattr_init(void);
+extern int efi_memattr_apply_permissions(struct mm_struct *mm,
+					 efi_memattr_perm_setter fn);
+
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc(m, md)					   \
 	for ((md) = (m)->map;						   \

Signature · 1 line

2.5.0

This declares the GUID and struct typedef for the new memory attributes table which contains the permissions that can be used to apply stricter permissions to UEFI Runtime Services memory regions.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c |  2 ++
 include/linux/efi.h        | 13 +++++++++++++
 2 files changed, 15 insertions(+)