[PATCH 0/5] memory attribute table support

[PATCH 0/5] memory attribute table support

[Ard Biesheuvel]

This implements ARM/arm64 support for dealing with the UEFI memory attribute
table, which allows the OS to map UEFI Runtime Services regions with stricter
permissions.

I have tried to implement the early discovery and validation of the table
as generically as possible. I think it should be reusable on x86 as well,
but I haven't tried myself. Note that this also depends on the way we
deal with the reservation of BootServicesData regions goind forward.

Note that for 32-bit ARM, this depends on [1], which addresses an issue
with memremap() on that architecture

Patches #1 and #2 implement support for the EFI_MEMORY_RO and EFI_MEMORY_XP
attributes, and as it turns out, this code supports both the (obsolete)
PropertiesTable and the new Memory Attributes table, so I moved them to the
beginning of the series.

Patch #3 adds the GUID and type definitions for the memory attributes table.

Patch #4 implements the generic handling of the table.

Patch #5 wires it up for ARM and arm64.

[1] http://thread.gmane.org/gmane.linux.kernel/2157907

Ard Biesheuvel (5):
  ARM: efi: apply strict permissons for UEFI Runtime Services regions
  arm64: efi: apply strict permissons for UEFI Runtime Services regions
  efi: add support for the EFI_MEMORY_ATTRIBUTES_TABLE config table
  efi: implement generic support for the Memory Attributes table
  arm*: efi: take the Memory Attributes table into account

 arch/arm/include/asm/efi.h         |   1 +
 arch/arm/kernel/efi.c              |  41 ++++++
 arch/arm64/include/asm/efi.h       |   2 +
 arch/arm64/kernel/efi.c            |  27 +++-
 drivers/firmware/efi/Makefile      |   2 +-
 drivers/firmware/efi/arm-init.c    |   1 +
 drivers/firmware/efi/arm-runtime.c |   3 +
 drivers/firmware/efi/efi.c         |   2 +
 drivers/firmware/efi/memattr.c     | 137 ++++++++++++++++++++
 include/linux/efi.h                |  19 +++
 10 files changed, 229 insertions(+), 6 deletions(-)
 create mode 100644 drivers/firmware/efi/memattr.c

-- 
2.5.0

[PATCH 0/5] memory attribute table support

[Ard Biesheuvel]

This implements ARM/arm64 support for dealing with the UEFI memory attribute
table, which allows the OS to map UEFI Runtime Services regions with stricter
permissions.

I have tried to implement the early discovery and validation of the table
as generically as possible. I think it should be reusable on x86 as well,
but I haven't tried myself. Note that this also depends on the way we
deal with the reservation of BootServicesData regions goind forward.

Note that for 32-bit ARM, this depends on [1], which addresses an issue
with memremap() on that architecture

Patches #1 and #2 implement support for the EFI_MEMORY_RO and EFI_MEMORY_XP
attributes, and as it turns out, this code supports both the (obsolete)
PropertiesTable and the new Memory Attributes table, so I moved them to the
beginning of the series.

Patch #3 adds the GUID and type definitions for the memory attributes table.

Patch #4 implements the generic handling of the table.

Patch #5 wires it up for ARM and arm64.

[1] http://thread.gmane.org/gmane.linux.kernel/2157907

Ard Biesheuvel (5):
  ARM: efi: apply strict permissons for UEFI Runtime Services regions
  arm64: efi: apply strict permissons for UEFI Runtime Services regions
  efi: add support for the EFI_MEMORY_ATTRIBUTES_TABLE config table
  efi: implement generic support for the Memory Attributes table
  arm*: efi: take the Memory Attributes table into account

 arch/arm/include/asm/efi.h         |   1 +
 arch/arm/kernel/efi.c              |  41 ++++++
 arch/arm64/include/asm/efi.h       |   2 +
 arch/arm64/kernel/efi.c            |  27 +++-
 drivers/firmware/efi/Makefile      |   2 +-
 drivers/firmware/efi/arm-init.c    |   1 +
 drivers/firmware/efi/arm-runtime.c |   3 +
 drivers/firmware/efi/efi.c         |   2 +
 drivers/firmware/efi/memattr.c     | 137 ++++++++++++++++++++
 include/linux/efi.h                |  19 +++
 10 files changed, 229 insertions(+), 6 deletions(-)
 create mode 100644 drivers/firmware/efi/memattr.c

-- 
2.5.0

[PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/include/asm/efi.h |  1 +
 arch/arm/kernel/efi.c      | 41 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72deb87..b0c341d7ceee 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -22,6 +22,7 @@
 void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 
 #define efi_call_virt(f, ...)						\
 ({									\
diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
index ff8a9d8acfac..9f43ba012d10 100644
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -11,6 +11,41 @@
 #include <asm/mach/map.h>
 #include <asm/mmu_context.h>
 
+static int __init set_permissions(pte_t *ptep, pgtable_t token,
+				  unsigned long addr, void *data)
+{
+	efi_memory_desc_t *md = data;
+	pte_t pte = *ptep;
+
+	if (md->attribute & EFI_MEMORY_RO)
+		pte = set_pte_bit(pte, __pgprot(L_PTE_RDONLY));
+	if (md->attribute & EFI_MEMORY_XP)
+		pte = set_pte_bit(pte, __pgprot(L_PTE_XN));
+	set_pte_ext(ptep, pte, PTE_EXT_NG);
+	return 0;
+}
+
+int __init efi_set_mapping_permissions(struct mm_struct *mm,
+				       efi_memory_desc_t *md)
+{
+	unsigned long base, size;
+
+	base = md->virt_addr;
+	size = md->num_pages << EFI_PAGE_SHIFT;
+
+	/*
+	 * We can only use apply_to_page_range() if we can guarantee that the
+	 * entire region was mapped using pages. This should be the case if the
+	 * region does not cover any naturally aligned SECTION_SIZE sized
+	 * blocks.
+	 */
+	if (round_down(base + size, SECTION_SIZE) <
+	    round_up(base, SECTION_SIZE) + SECTION_SIZE)
+		return apply_to_page_range(mm, base, size, set_permissions, md);
+
+	return 0;
+}
+
 int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 {
 	struct map_desc desc = {
@@ -34,5 +69,11 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 		desc.type = MT_DEVICE;
 
 	create_mapping_late(mm, &desc, true);
+
+	/*
+	 * If stricter permissions were specified, apply them now.
+	 */
+	if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
+		return efi_set_mapping_permissions(mm, md);
 	return 0;
 }
-- 
2.5.0

[PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[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 <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/efi.h |  1 +
 arch/arm/kernel/efi.c      | 41 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72deb87..b0c341d7ceee 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -22,6 +22,7 @@
 void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
+int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 
 #define efi_call_virt(f, ...)						\
 ({									\
diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
index ff8a9d8acfac..9f43ba012d10 100644
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -11,6 +11,41 @@
 #include <asm/mach/map.h>
 #include <asm/mmu_context.h>
 
+static int __init set_permissions(pte_t *ptep, pgtable_t token,
+				  unsigned long addr, void *data)
+{
+	efi_memory_desc_t *md = data;
+	pte_t pte = *ptep;
+
+	if (md->attribute & EFI_MEMORY_RO)
+		pte = set_pte_bit(pte, __pgprot(L_PTE_RDONLY));
+	if (md->attribute & EFI_MEMORY_XP)
+		pte = set_pte_bit(pte, __pgprot(L_PTE_XN));
+	set_pte_ext(ptep, pte, PTE_EXT_NG);
+	return 0;
+}
+
+int __init efi_set_mapping_permissions(struct mm_struct *mm,
+				       efi_memory_desc_t *md)
+{
+	unsigned long base, size;
+
+	base = md->virt_addr;
+	size = md->num_pages << EFI_PAGE_SHIFT;
+
+	/*
+	 * We can only use apply_to_page_range() if we can guarantee that the
+	 * entire region was mapped using pages. This should be the case if the
+	 * region does not cover any naturally aligned SECTION_SIZE sized
+	 * blocks.
+	 */
+	if (round_down(base + size, SECTION_SIZE) <
+	    round_up(base, SECTION_SIZE) + SECTION_SIZE)
+		return apply_to_page_range(mm, base, size, set_permissions, md);
+
+	return 0;
+}
+
 int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 {
 	struct map_desc desc = {
@@ -34,5 +69,11 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 		desc.type = MT_DEVICE;
 
 	create_mapping_late(mm, &desc, true);
+
+	/*
+	 * If stricter permissions were specified, apply them now.
+	 */
+	if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
+		return efi_set_mapping_permissions(mm, md);
 	return 0;
 }
-- 
2.5.0

[PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index b6abc852f2a1..3364408c154f 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -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);
+	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,
-- 
2.5.0

[PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index b6abc852f2a1..3364408c154f 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -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);
+	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,
-- 
2.5.0

[PATCH 3/5] efi: add support for the EFI_MEMORY_ATTRIBUTES_TABLE config table

[Ard Biesheuvel]

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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/efi.c |  2 ++
 include/linux/efi.h        | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5ecfcb..5683865baa92 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -43,6 +43,7 @@ struct efi __read_mostly efi = {
 	.config_table		= EFI_INVALID_TABLE_ADDR,
 	.esrt			= EFI_INVALID_TABLE_ADDR,
 	.properties_table	= EFI_INVALID_TABLE_ADDR,
+	.mem_attr_table		= EFI_INVALID_TABLE_ADDR,
 };
 EXPORT_SYMBOL(efi);
 
@@ -338,6 +339,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
 	{UGA_IO_PROTOCOL_GUID, "UGA", &efi.uga},
 	{EFI_SYSTEM_RESOURCE_TABLE_GUID, "ESRT", &efi.esrt},
 	{EFI_PROPERTIES_TABLE_GUID, "PROP", &efi.properties_table},
+	{EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
 	{NULL_GUID, NULL, NULL},
 };
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 42be9c92fdf0..808d97299c70 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -619,6 +619,10 @@ void efi_native_runtime_setup(void);
 	EFI_GUID(0x880aaca3, 0x4adc, 0x4a04, \
 		 0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5)
 
+#define EFI_MEMORY_ATTRIBUTES_TABLE_GUID \
+	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f, \
+		 0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;
@@ -843,6 +847,14 @@ typedef struct {
 
 #define EFI_INVALID_TABLE_ADDR		(~0UL)
 
+typedef struct {
+	u32 version;
+	u32 num_entries;
+	u32 desc_size;
+	u32 reserved;
+	efi_memory_desc_t entry[0];
+} efi_memory_attributes_table_t;
+
 /*
  * All runtime access to EFI goes through this structure:
  */
@@ -864,6 +876,7 @@ extern struct efi {
 	unsigned long config_table;	/* config tables */
 	unsigned long esrt;		/* ESRT table */
 	unsigned long properties_table;	/* properties table */
+	unsigned long mem_attr_table;	/* memory attributes table */
 	efi_get_time_t *get_time;
 	efi_set_time_t *set_time;
 	efi_get_wakeup_time_t *get_wakeup_time;
-- 
2.5.0

[PATCH 3/5] efi: add support for the EFI_MEMORY_ATTRIBUTES_TABLE config table

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c |  2 ++
 include/linux/efi.h        | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3a69ed5ecfcb..5683865baa92 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -43,6 +43,7 @@ struct efi __read_mostly efi = {
 	.config_table		= EFI_INVALID_TABLE_ADDR,
 	.esrt			= EFI_INVALID_TABLE_ADDR,
 	.properties_table	= EFI_INVALID_TABLE_ADDR,
+	.mem_attr_table		= EFI_INVALID_TABLE_ADDR,
 };
 EXPORT_SYMBOL(efi);
 
@@ -338,6 +339,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
 	{UGA_IO_PROTOCOL_GUID, "UGA", &efi.uga},
 	{EFI_SYSTEM_RESOURCE_TABLE_GUID, "ESRT", &efi.esrt},
 	{EFI_PROPERTIES_TABLE_GUID, "PROP", &efi.properties_table},
+	{EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
 	{NULL_GUID, NULL, NULL},
 };
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 42be9c92fdf0..808d97299c70 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -619,6 +619,10 @@ void efi_native_runtime_setup(void);
 	EFI_GUID(0x880aaca3, 0x4adc, 0x4a04, \
 		 0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5)
 
+#define EFI_MEMORY_ATTRIBUTES_TABLE_GUID \
+	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f, \
+		 0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;
@@ -843,6 +847,14 @@ typedef struct {
 
 #define EFI_INVALID_TABLE_ADDR		(~0UL)
 
+typedef struct {
+	u32 version;
+	u32 num_entries;
+	u32 desc_size;
+	u32 reserved;
+	efi_memory_desc_t entry[0];
+} efi_memory_attributes_table_t;
+
 /*
  * All runtime access to EFI goes through this structure:
  */
@@ -864,6 +876,7 @@ extern struct efi {
 	unsigned long config_table;	/* config tables */
 	unsigned long esrt;		/* ESRT table */
 	unsigned long properties_table;	/* properties table */
+	unsigned long mem_attr_table;	/* memory attributes table */
 	efi_get_time_t *get_time;
 	efi_set_time_t *set_time;
 	efi_get_wakeup_time_t *get_wakeup_time;
-- 
2.5.0

[PATCH 4/5] efi: implement generic support for the Memory Attributes table

[Ard Biesheuvel]

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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/Makefile  |   2 +-
 drivers/firmware/efi/memattr.c | 137 ++++++++++++++++++++
 include/linux/efi.h            |   6 +
 3 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 62e654f255f4..d5be62399130 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -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
diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
new file mode 100644
index 000000000000..059cf4522a7d
--- /dev/null
+++ b/drivers/firmware/efi/memattr.c
@@ -0,0 +1,137 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd. <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#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) {
+		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
+		       efi.mem_attr_table);
+		return -ENOMEM;
+	}
+
+	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
+	memblock_reserve(efi.mem_attr_table, tbl_size);
+	early_memunmap(tbl, sizeof(*tbl));
+	return 0;
+}
+
+/*
+ * Returns a copy @out of the UEFI memory descriptor @in if it is covered
+ * entirely by a UEFI memory map entry with matching attributes. The virtual
+ * address of @out is set according to the matching entry that was found.
+ */
+static bool validate_entry(const efi_memory_desc_t *in, efi_memory_desc_t *out)
+{
+	u64 in_paddr = in->phys_addr;
+	u64 in_size = in->num_pages << EFI_PAGE_SHIFT;
+	efi_memory_desc_t *md;
+
+	if (in->type != EFI_RUNTIME_SERVICES_CODE &&
+	    in->type != EFI_RUNTIME_SERVICES_DATA) {
+		pr_warn("MEMATTR table entry type should be RuntimeServiceCode or RuntimeServicesData\n");
+		return false;
+	}
+	if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) {
+		pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n");
+		return false;
+	}
+	if (PAGE_SIZE > EFI_PAGE_SIZE &&
+	    (!PAGE_ALIGNED(in->phys_addr) ||
+	     !PAGE_ALIGNED(in->num_pages << EFI_PAGE_SHIFT))) {
+		pr_warn("MEMATTR table entry misaligned\n");
+		return false;
+	}
+	for_each_efi_memory_desc(&memmap, md) {
+		u64 md_paddr = md->phys_addr;
+		u64 md_size = md->num_pages << EFI_PAGE_SHIFT;
+
+		if (!(md->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+		if (md->virt_addr == 0)
+			/* no virtual mapping has been installed by the stub */
+			break;
+		if (md_paddr <= in_paddr && (in_paddr - md_paddr) < md_size) {
+			/*
+			 * This entry covers the start of @in, check whether
+			 * it covers the end as well.
+			 */
+			if (md_paddr + md_size < in_paddr + in_size) {
+				pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n");
+				return false;
+			}
+			if (md->type != in->type) {
+				pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n");
+				return false;
+			}
+			*out = *in;
+			out->virt_addr = in_paddr +
+					 (md->virt_addr - md->phys_addr);
+			return true;
+		}
+	}
+	return false;
+}
+
+int __init efi_memattr_apply_permissions(struct mm_struct *mm,
+					 efi_memattr_perm_setter fn)
+{
+	efi_memory_attributes_table_t *tbl;
+	int i, ret;
+
+	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;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 808d97299c70..94e41872e5e6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -966,6 +966,12 @@ extern void __init efi_fake_memmap(void);
 static inline void efi_fake_memmap(void) { }
 #endif
 
+typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
+
+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;						   \
-- 
2.5.0

[PATCH 4/5] efi: implement generic support for the Memory Attributes table

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/Makefile  |   2 +-
 drivers/firmware/efi/memattr.c | 137 ++++++++++++++++++++
 include/linux/efi.h            |   6 +
 3 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 62e654f255f4..d5be62399130 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -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
diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
new file mode 100644
index 000000000000..059cf4522a7d
--- /dev/null
+++ b/drivers/firmware/efi/memattr.c
@@ -0,0 +1,137 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#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) {
+		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
+		       efi.mem_attr_table);
+		return -ENOMEM;
+	}
+
+	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
+	memblock_reserve(efi.mem_attr_table, tbl_size);
+	early_memunmap(tbl, sizeof(*tbl));
+	return 0;
+}
+
+/*
+ * Returns a copy @out of the UEFI memory descriptor @in if it is covered
+ * entirely by a UEFI memory map entry with matching attributes. The virtual
+ * address of @out is set according to the matching entry that was found.
+ */
+static bool validate_entry(const efi_memory_desc_t *in, efi_memory_desc_t *out)
+{
+	u64 in_paddr = in->phys_addr;
+	u64 in_size = in->num_pages << EFI_PAGE_SHIFT;
+	efi_memory_desc_t *md;
+
+	if (in->type != EFI_RUNTIME_SERVICES_CODE &&
+	    in->type != EFI_RUNTIME_SERVICES_DATA) {
+		pr_warn("MEMATTR table entry type should be RuntimeServiceCode or RuntimeServicesData\n");
+		return false;
+	}
+	if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) {
+		pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n");
+		return false;
+	}
+	if (PAGE_SIZE > EFI_PAGE_SIZE &&
+	    (!PAGE_ALIGNED(in->phys_addr) ||
+	     !PAGE_ALIGNED(in->num_pages << EFI_PAGE_SHIFT))) {
+		pr_warn("MEMATTR table entry misaligned\n");
+		return false;
+	}
+	for_each_efi_memory_desc(&memmap, md) {
+		u64 md_paddr = md->phys_addr;
+		u64 md_size = md->num_pages << EFI_PAGE_SHIFT;
+
+		if (!(md->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+		if (md->virt_addr == 0)
+			/* no virtual mapping has been installed by the stub */
+			break;
+		if (md_paddr <= in_paddr && (in_paddr - md_paddr) < md_size) {
+			/*
+			 * This entry covers the start of @in, check whether
+			 * it covers the end as well.
+			 */
+			if (md_paddr + md_size < in_paddr + in_size) {
+				pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n");
+				return false;
+			}
+			if (md->type != in->type) {
+				pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n");
+				return false;
+			}
+			*out = *in;
+			out->virt_addr = in_paddr +
+					 (md->virt_addr - md->phys_addr);
+			return true;
+		}
+	}
+	return false;
+}
+
+int __init efi_memattr_apply_permissions(struct mm_struct *mm,
+					 efi_memattr_perm_setter fn)
+{
+	efi_memory_attributes_table_t *tbl;
+	int i, ret;
+
+	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;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 808d97299c70..94e41872e5e6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -966,6 +966,12 @@ extern void __init efi_fake_memmap(void);
 static inline void efi_fake_memmap(void) { }
 #endif
 
+typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
+
+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;						   \
-- 
2.5.0

[PATCH 5/5] arm*: efi: take the Memory Attributes table into account

[Ard Biesheuvel]

Call into the generic memory attributes table support code at the
appropriate times during the init sequence so that the UEFI Runtime
Services region are mapped according to the strict permissions it
specifies.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/include/asm/efi.h       | 2 ++
 drivers/firmware/efi/arm-init.c    | 1 +
 drivers/firmware/efi/arm-runtime.c | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a696c9cb..4dafc89f373a 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -14,6 +14,8 @@ extern void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 
+#define efi_set_mapping_permissions	efi_create_mapping
+
 #define efi_call_virt(f, ...)						\
 ({									\
 	efi_##f##_t *__f;						\
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index aa1f743152a2..022f11157acd 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -202,6 +202,7 @@ void __init efi_init(void)
 		return;
 
 	reserve_regions();
+	efi_memattr_init();
 	early_memunmap(memmap.map, params.mmap_size);
 	memblock_mark_nomap(params.mmap & PAGE_MASK,
 			    PAGE_ALIGN(params.mmap_size +
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 3a3911641049..848ede1587dc 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -65,6 +65,9 @@ static bool __init efi_virtmap_init(void)
 			return false;
 		}
 	}
+
+	if (efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions))
+		return false;
 	return true;
 }
 
-- 
2.5.0

[PATCH 5/5] arm*: efi: take the Memory Attributes table into account

[Ard Biesheuvel]

Call into the generic memory attributes table support code at the
appropriate times during the init sequence so that the UEFI Runtime
Services region are mapped according to the strict permissions it
specifies.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/efi.h       | 2 ++
 drivers/firmware/efi/arm-init.c    | 1 +
 drivers/firmware/efi/arm-runtime.c | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a696c9cb..4dafc89f373a 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -14,6 +14,8 @@ extern void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 
+#define efi_set_mapping_permissions	efi_create_mapping
+
 #define efi_call_virt(f, ...)						\
 ({									\
 	efi_##f##_t *__f;						\
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index aa1f743152a2..022f11157acd 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -202,6 +202,7 @@ void __init efi_init(void)
 		return;
 
 	reserve_regions();
+	efi_memattr_init();
 	early_memunmap(memmap.map, params.mmap_size);
 	memblock_mark_nomap(params.mmap & PAGE_MASK,
 			    PAGE_ALIGN(params.mmap_size +
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 3a3911641049..848ede1587dc 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -65,6 +65,9 @@ static bool __init efi_virtmap_init(void)
 			return false;
 		}
 	}
+
+	if (efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions))
+		return false;
 	return true;
 }
 
-- 
2.5.0

Re: [PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[Matt Fleming]

On Mon, 22 Feb, at 03:25:54PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  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.

[PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[Matt Fleming]

On Mon, 22 Feb, at 03:25:54PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  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.

Re: [PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[Matt Fleming]

On Mon, 22 Feb, at 03:25:55PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index b6abc852f2a1..3364408c154f 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -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);
> +	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,

---

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 3364408c154f..33a6da160a50 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -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,

[PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[Matt Fleming]

On Mon, 22 Feb, at 03:25:55PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index b6abc852f2a1..3364408c154f 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -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);
> +	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,

---

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 3364408c154f..33a6da160a50 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -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,

Re: [PATCH 3/5] efi: add support for the EFI_MEMORY_ATTRIBUTES_TABLE config table

[Matt Fleming]

On Mon, 22 Feb, at 03:25:56PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/efi.c |  2 ++
>  include/linux/efi.h        | 13 +++++++++++++
>  2 files changed, 15 insertions(+)

Looks good Ard.

[PATCH 3/5] efi: add support for the EFI_MEMORY_ATTRIBUTES_TABLE config table

[Matt Fleming]

On Mon, 22 Feb, at 03:25:56PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/efi.c |  2 ++
>  include/linux/efi.h        | 13 +++++++++++++
>  2 files changed, 15 insertions(+)

Looks good Ard.

Re: [PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[Ard Biesheuvel]

On 2 March 2016 at 12:49, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Mon, 22 Feb, at 03:25:54PM, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  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.

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)

Thanks,
Ard.

[PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[Ard Biesheuvel]

On 2 March 2016 at 12:49, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Mon, 22 Feb, at 03:25:54PM, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  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.

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)

Thanks,
Ard.

Re: [PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[Ard Biesheuvel]

On 2 March 2016 at 13:10, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Mon, 22 Feb, at 03:25:55PM, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index b6abc852f2a1..3364408c154f 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -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);
>> +     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,
>

Sure, that looks a lot better. I will fold that into v2

> ---
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 3364408c154f..33a6da160a50 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -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,

[PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[Ard Biesheuvel]

On 2 March 2016 at 13:10, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Mon, 22 Feb, at 03:25:55PM, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index b6abc852f2a1..3364408c154f 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -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);
>> +     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,
>

Sure, that looks a lot better. I will fold that into v2

> ---
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 3364408c154f..33a6da160a50 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -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,

Re: [PATCH 4/5] efi: implement generic support for the Memory Attributes table

[Matt Fleming]

On Mon, 22 Feb, at 03:29:14PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/Makefile  |   2 +-
>  drivers/firmware/efi/memattr.c | 137 ++++++++++++++++++++
>  include/linux/efi.h            |   6 +
>  3 files changed, 144 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 62e654f255f4..d5be62399130 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -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
> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> new file mode 100644
> index 000000000000..059cf4522a7d
> --- /dev/null
> +++ b/drivers/firmware/efi/memattr.c
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2016 Linaro Ltd. <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +

Please include,

#define pr_fmt(fmt)	"efi: " fmt

here. I've seen in the past that reporters will sometimes miss error
messages when searching for EFI failures if they are not prefixed with
"efi: ".

> +#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) {
> +		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
> +		       efi.mem_attr_table);
> +		return -ENOMEM;
> +	}
> +
> +	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
> +	memblock_reserve(efi.mem_attr_table, tbl_size);
> +	early_memunmap(tbl, sizeof(*tbl));
> +	return 0;
> +}

We should really be checking tbl::version here so that if the version
gets bumped in the future, perhaps due to a change in the structure
layout, we don't try and reserve a crazy amount of memory.

> +/*
> + * Returns a copy @out of the UEFI memory descriptor @in if it is covered
> + * entirely by a UEFI memory map entry with matching attributes. The virtual
> + * address of @out is set according to the matching entry that was found.
> + */
> +static bool validate_entry(const efi_memory_desc_t *in, efi_memory_desc_t *out)
> +{

Might I suggest "entry_is_valid" or "valid_entry" for this function? A
boolean return value implies we're asking a question.

> +	u64 in_paddr = in->phys_addr;
> +	u64 in_size = in->num_pages << EFI_PAGE_SHIFT;
> +	efi_memory_desc_t *md;
> +
> +	if (in->type != EFI_RUNTIME_SERVICES_CODE &&
> +	    in->type != EFI_RUNTIME_SERVICES_DATA) {
> +		pr_warn("MEMATTR table entry type should be RuntimeServiceCode or RuntimeServicesData\n");

Hehe, I'm not a slave to the 80-column rule but 106-columns is pretty
extreme. Could we reword this to shorten the string length?

> +		return false;
> +	}

Newline here please.

> +	if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) {
> +		pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n");
> +		return false;
> +	}

Newline here please.

> +	if (PAGE_SIZE > EFI_PAGE_SIZE &&
> +	    (!PAGE_ALIGNED(in->phys_addr) ||
> +	     !PAGE_ALIGNED(in->num_pages << EFI_PAGE_SHIFT))) {
> +		pr_warn("MEMATTR table entry misaligned\n");
> +		return false;
> +	}

Could you provide a comment for this check? I realise it's checking
conformance to the region alignment rules for arm64, but a comment to
that effect would be useful.

> +	for_each_efi_memory_desc(&memmap, md) {
> +		u64 md_paddr = md->phys_addr;
> +		u64 md_size = md->num_pages << EFI_PAGE_SHIFT;
> +
> +		if (!(md->attribute & EFI_MEMORY_RUNTIME))
> +			continue;

Newline here please.

> +		if (md->virt_addr == 0)
> +			/* no virtual mapping has been installed by the stub */
> +			break;

Please use braces when there are multiple lines under the if().

> +		if (md_paddr <= in_paddr && (in_paddr - md_paddr) < md_size) {

Let's save ourselves one level of indentation here and invert this
check, continuing if the inverted check is false.

> +			/*
> +			 * This entry covers the start of @in, check whether
> +			 * it covers the end as well.
> +			 */
> +			if (md_paddr + md_size < in_paddr + in_size) {
> +				pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n");
> +				return false;
> +			}

Newline please.

> +			if (md->type != in->type) {
> +				pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n");
> +				return false;
> +			}

Newline please.

> +			*out = *in;
> +			out->virt_addr = in_paddr +
> +					 (md->virt_addr - md->phys_addr);
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +int __init efi_memattr_apply_permissions(struct mm_struct *mm,
> +					 efi_memattr_perm_setter fn)
> +{
> +	efi_memory_attributes_table_t *tbl;
> +	int i, ret;

It might be worth a comment above this function explaining that it
should only be invoked after the machine has entered virtual mode and
SetVirtualAddressMap() has been called.

> +
> +	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;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 808d97299c70..94e41872e5e6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -966,6 +966,12 @@ extern void __init efi_fake_memmap(void);
>  static inline void efi_fake_memmap(void) { }
>  #endif
>  
> +typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
> +

Please include a comment explaining what an 'efi_memattr_perm_setter'
is and the rules (if any) constraining the implementation.

> +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;						   \
> -- 
> 2.5.0
> 

[PATCH 4/5] efi: implement generic support for the Memory Attributes table

[Matt Fleming]

On Mon, 22 Feb, at 03:29:14PM, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/Makefile  |   2 +-
>  drivers/firmware/efi/memattr.c | 137 ++++++++++++++++++++
>  include/linux/efi.h            |   6 +
>  3 files changed, 144 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 62e654f255f4..d5be62399130 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -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
> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> new file mode 100644
> index 000000000000..059cf4522a7d
> --- /dev/null
> +++ b/drivers/firmware/efi/memattr.c
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2016 Linaro Ltd. <ard.biesheuvel@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +

Please include,

#define pr_fmt(fmt)	"efi: " fmt

here. I've seen in the past that reporters will sometimes miss error
messages when searching for EFI failures if they are not prefixed with
"efi: ".

> +#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) {
> +		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
> +		       efi.mem_attr_table);
> +		return -ENOMEM;
> +	}
> +
> +	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
> +	memblock_reserve(efi.mem_attr_table, tbl_size);
> +	early_memunmap(tbl, sizeof(*tbl));
> +	return 0;
> +}

We should really be checking tbl::version here so that if the version
gets bumped in the future, perhaps due to a change in the structure
layout, we don't try and reserve a crazy amount of memory.

> +/*
> + * Returns a copy @out of the UEFI memory descriptor @in if it is covered
> + * entirely by a UEFI memory map entry with matching attributes. The virtual
> + * address of @out is set according to the matching entry that was found.
> + */
> +static bool validate_entry(const efi_memory_desc_t *in, efi_memory_desc_t *out)
> +{

Might I suggest "entry_is_valid" or "valid_entry" for this function? A
boolean return value implies we're asking a question.

> +	u64 in_paddr = in->phys_addr;
> +	u64 in_size = in->num_pages << EFI_PAGE_SHIFT;
> +	efi_memory_desc_t *md;
> +
> +	if (in->type != EFI_RUNTIME_SERVICES_CODE &&
> +	    in->type != EFI_RUNTIME_SERVICES_DATA) {
> +		pr_warn("MEMATTR table entry type should be RuntimeServiceCode or RuntimeServicesData\n");

Hehe, I'm not a slave to the 80-column rule but 106-columns is pretty
extreme. Could we reword this to shorten the string length?

> +		return false;
> +	}

Newline here please.

> +	if (!(in->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))) {
> +		pr_warn("MEMATTR table entry attributes invalid: RO and XP bits both cleared\n");
> +		return false;
> +	}

Newline here please.

> +	if (PAGE_SIZE > EFI_PAGE_SIZE &&
> +	    (!PAGE_ALIGNED(in->phys_addr) ||
> +	     !PAGE_ALIGNED(in->num_pages << EFI_PAGE_SHIFT))) {
> +		pr_warn("MEMATTR table entry misaligned\n");
> +		return false;
> +	}

Could you provide a comment for this check? I realise it's checking
conformance to the region alignment rules for arm64, but a comment to
that effect would be useful.

> +	for_each_efi_memory_desc(&memmap, md) {
> +		u64 md_paddr = md->phys_addr;
> +		u64 md_size = md->num_pages << EFI_PAGE_SHIFT;
> +
> +		if (!(md->attribute & EFI_MEMORY_RUNTIME))
> +			continue;

Newline here please.

> +		if (md->virt_addr == 0)
> +			/* no virtual mapping has been installed by the stub */
> +			break;

Please use braces when there are multiple lines under the if().

> +		if (md_paddr <= in_paddr && (in_paddr - md_paddr) < md_size) {

Let's save ourselves one level of indentation here and invert this
check, continuing if the inverted check is false.

> +			/*
> +			 * This entry covers the start of @in, check whether
> +			 * it covers the end as well.
> +			 */
> +			if (md_paddr + md_size < in_paddr + in_size) {
> +				pr_warn("MEMATTR table entry covers multiple UEFI memory map regions\n");
> +				return false;
> +			}

Newline please.

> +			if (md->type != in->type) {
> +				pr_warn("MEMATTR table entry type deviates from UEFI memory map region type\n");
> +				return false;
> +			}

Newline please.

> +			*out = *in;
> +			out->virt_addr = in_paddr +
> +					 (md->virt_addr - md->phys_addr);
> +			return true;
> +		}
> +	}
> +	return false;
> +}
> +
> +int __init efi_memattr_apply_permissions(struct mm_struct *mm,
> +					 efi_memattr_perm_setter fn)
> +{
> +	efi_memory_attributes_table_t *tbl;
> +	int i, ret;

It might be worth a comment above this function explaining that it
should only be invoked after the machine has entered virtual mode and
SetVirtualAddressMap() has been called.

> +
> +	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;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 808d97299c70..94e41872e5e6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -966,6 +966,12 @@ extern void __init efi_fake_memmap(void);
>  static inline void efi_fake_memmap(void) { }
>  #endif
>  
> +typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
> +

Please include a comment explaining what an 'efi_memattr_perm_setter'
is and the rules (if any) constraining the implementation.

> +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;						   \
> -- 
> 2.5.0
> 

Re: [PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[Matt Fleming]

On Wed, 02 Mar, at 02:07:36PM, Ard Biesheuvel wrote:
> On 2 March 2016 at 12:49, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > On Mon, 22 Feb, at 03:25:54PM, Ard Biesheuvel wrote:
> >> 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 <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  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.
> 
> 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)

Thanks for the reminder. I'll definitely hold off on applying this for
now.

[PATCH 1/5] ARM: efi: apply strict permissons for UEFI Runtime Services regions

[Matt Fleming]

On Wed, 02 Mar, at 02:07:36PM, Ard Biesheuvel wrote:
> On 2 March 2016 at 12:49, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Mon, 22 Feb, at 03:25:54PM, Ard Biesheuvel wrote:
> >> 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 <ard.biesheuvel@linaro.org>
> >> ---
> >>  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.
> 
> 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)

Thanks for the reminder. I'll definitely hold off on applying this for
now.