[PATCH v2 0/5] UEFI: memory attribute table support

[PATCH v2 0/5] UEFI: 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 going 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.

Changes since v1:
- incorporate feedback from Matt (patches #2 and #4)
- rebased onto v4.6-rc1

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/488476

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            |  54 +++++--
 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     | 170 ++++++++++++++++++++
 include/linux/efi.h                |  26 +++
 10 files changed, 287 insertions(+), 15 deletions(-)
 create mode 100644 drivers/firmware/efi/memattr.c

-- 
2.5.0

[PATCH v2 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.

Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
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 v2 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.

Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm64/kernel/efi.c | 54 +++++++++++++++-----
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index b6abc852f2a1..33a6da160a50 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -17,22 +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.
-	 */
-	if ((md->attribute & EFI_MEMORY_WB) == 0)
-		prot_val = PROT_DEVICE_nGnRE;
-	else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
-		 !PAGE_ALIGNED(md->phys_addr))
-		prot_val = pgprot_val(PAGE_KERNEL_EXEC);
-	else
-		prot_val = pgprot_val(PAGE_KERNEL);
+	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.
+		 */
+		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,
-- 
2.5.0

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

[Shanker Donthineni]

Hi Ard,

This patch causing the kernel boot fail using the UEFI firmware on
Qualcomm Technologies QDF2XXX server platforms.

+	/* RW- */
+	if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE)
+		return pgprot_val(PAGE_KERNEL);
+

Changing above condition from 'or' to 'and' fixed the problem.Are we
missing something here or intentionally implemented this logic? At least
we need some pr_warn here if UEFI firmware passes EFI_RUNTIME_SEVRICE_CODE
region that has an attribute EFI_MEMORY_XP.



On 03/30/2016 11:38 AM, 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.
>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/efi.c | 54 +++++++++++++++-----
>  1 file changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index b6abc852f2a1..33a6da160a50 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -17,22 +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.
> -	 */
> -	if ((md->attribute & EFI_MEMORY_WB) == 0)
> -		prot_val = PROT_DEVICE_nGnRE;
> -	else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> -		 !PAGE_ALIGNED(md->phys_addr))
> -		prot_val = pgprot_val(PAGE_KERNEL_EXEC);
> -	else
> -		prot_val = pgprot_val(PAGE_KERNEL);
> +	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.
> +		 */
> +		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,

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

[Ard Biesheuvel]

On 18 May 2016 at 02:40, Shanker Donthineni <shankerd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Hi Ard,
>
> This patch causing the kernel boot fail using the UEFI firmware on
> Qualcomm Technologies QDF2XXX server platforms.
>
> +       /* RW- */
> +       if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE)
> +               return pgprot_val(PAGE_KERNEL);
> +
>
> Changing above condition from 'or' to 'and' fixed the problem.Are we
> missing something here or intentionally implemented this logic?

The logic is sound, I think. If you need execute permissions on a
non-code region with the XP bit set, there is something wrong in the
firmware, unless you are hitting this case for an MMIO region perhaps?
Could you share the kernel log with efi=debug enabled?

> At least
> we need some pr_warn here if UEFI firmware passes EFI_RUNTIME_SEVRICE_CODE
> region that has an attribute EFI_MEMORY_XP.
>

No. DXE_RUNTIME_DRIVER modules are implemented as PE/COFF binaries,
which are covered completely by EfiRuntimeServicesCode regions.
However, a PE/COFF image consists of .text and .data sections, which
require different permissions, so PE/COFF binaries are represented by
several memory map entries in the memory attributes table, all of
which have a code type, but differ in the permission bits.

>
>
> On 03/30/2016 11:38 AM, 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.
>>
>> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  arch/arm64/kernel/efi.c | 54 +++++++++++++++-----
>>  1 file changed, 40 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index b6abc852f2a1..33a6da160a50 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -17,22 +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.
>> -      */
>> -     if ((md->attribute & EFI_MEMORY_WB) == 0)
>> -             prot_val = PROT_DEVICE_nGnRE;
>> -     else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> -              !PAGE_ALIGNED(md->phys_addr))
>> -             prot_val = pgprot_val(PAGE_KERNEL_EXEC);
>> -     else
>> -             prot_val = pgprot_val(PAGE_KERNEL);
>> +     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.
>> +              */
>> +             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,
>
> --
> Shanker Donthineni
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>

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

[Shanker Donthineni]

Hi Ard,

We are not using the permission attribute table and all the Runtime
Data/Code regions are marked with 'XP' bit. Do you think something
wrong in our UEFI implementation or Linux has to map RuntimeCode
regions with executable permission always irrespective of an attribute
flag EFI_MEMORY_XP?

[    0.000000] efi: Processing EFI memory map:
[    0.000000] efi:   0x000000100000-0x00000010ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x00003b830000-0x00003b83ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x00003c000000-0x00003fffffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x000000100000-0x00000010ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x00003b830000-0x00003b83ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x00003c000000-0x00003fffffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
[    0.000000] efi:   0x004000820000-0x00400085ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x004003080000-0x00400308ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047f9960000-0x0047f997ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fa9c0000-0x0047fa9cffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047faa30000-0x0047faa3ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fab90000-0x0047fab9ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047faba0000-0x0047fabaffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fab90000-0x0047fab9ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047faba0000-0x0047fabaffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fabb0000-0x0047fabbffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fabc0000-0x0047fad9ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fada0000-0x0047fae2ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047faf50000-0x0047faf7ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fafa0000-0x0047fafbffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047faf50000-0x0047faf7ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fafa0000-0x0047fafbffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fff90000-0x0047fff9ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
[    0.000000] efi:   0x0047fffb0000-0x0047fffdffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*


On 05/18/2016 01:18 AM, Ard Biesheuvel wrote:
> On 18 May 2016 at 02:40, Shanker Donthineni <shankerd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> Hi Ard,
>>
>> This patch causing the kernel boot fail using the UEFI firmware on
>> Qualcomm Technologies QDF2XXX server platforms.
>>
>> +       /* RW- */
>> +       if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE)
>> +               return pgprot_val(PAGE_KERNEL);
>> +
>>
>> Changing above condition from 'or' to 'and' fixed the problem.Are we
>> missing something here or intentionally implemented this logic?
> The logic is sound, I think. If you need execute permissions on a
> non-code region with the XP bit set, there is something wrong in the
> firmware, unless you are hitting this case for an MMIO region perhaps?
> Could you share the kernel log with efi=debug enabled?
>
>> At least
>> we need some pr_warn here if UEFI firmware passes EFI_RUNTIME_SEVRICE_CODE
>> region that has an attribute EFI_MEMORY_XP.
>>
> No. DXE_RUNTIME_DRIVER modules are implemented as PE/COFF binaries,
> which are covered completely by EfiRuntimeServicesCode regions.
> However, a PE/COFF image consists of .text and .data sections, which
> require different permissions, so PE/COFF binaries are represented by
> several memory map entries in the memory attributes table, all of
> which have a code type, but differ in the permission bits.
>
>>
>> On 03/30/2016 11:38 AM, 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.
>>>
>>> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>  arch/arm64/kernel/efi.c | 54 +++++++++++++++-----
>>>  1 file changed, 40 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>>> index b6abc852f2a1..33a6da160a50 100644
>>> --- a/arch/arm64/kernel/efi.c
>>> +++ b/arch/arm64/kernel/efi.c
>>> @@ -17,22 +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.
>>> -      */
>>> -     if ((md->attribute & EFI_MEMORY_WB) == 0)
>>> -             prot_val = PROT_DEVICE_nGnRE;
>>> -     else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>>> -              !PAGE_ALIGNED(md->phys_addr))
>>> -             prot_val = pgprot_val(PAGE_KERNEL_EXEC);
>>> -     else
>>> -             prot_val = pgprot_val(PAGE_KERNEL);
>>> +     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.
>>> +              */
>>> +             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,
>> --
>> Shanker Donthineni
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

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

[Ard Biesheuvel]

On 18 May 2016 at 15:06, Shanker Donthineni <shankerd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Hi Ard,
>
> We are not using the permission attribute table and all the Runtime
> Data/Code regions are marked with 'XP' bit. Do you think something
> wrong in our UEFI implementation or Linux has to map RuntimeCode
> regions with executable permission always irrespective of an attribute
> flag EFI_MEMORY_XP?
>
> [    0.000000] efi: Processing EFI memory map:
> [    0.000000] efi:   0x000000100000-0x00000010ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
> [    0.000000] efi:   0x00003b830000-0x00003b83ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
> [    0.000000] efi:   0x00003c000000-0x00003fffffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
> [    0.000000] efi:   0x000000100000-0x00000010ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
> [    0.000000] efi:   0x00003b830000-0x00003b83ffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
> [    0.000000] efi:   0x00003c000000-0x00003fffffff [Memory Mapped I/O  |RUN|  |  |  |  |  |  |   |  |  |  |UC]
> [    0.000000] efi:   0x004000820000-0x00400085ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x004003080000-0x00400308ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047f9960000-0x0047f997ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fa9c0000-0x0047fa9cffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047faa30000-0x0047faa3ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fab90000-0x0047fab9ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047faba0000-0x0047fabaffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fab90000-0x0047fab9ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047faba0000-0x0047fabaffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fabb0000-0x0047fabbffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fabc0000-0x0047fad9ffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fada0000-0x0047fae2ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047faf50000-0x0047faf7ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fafa0000-0x0047fafbffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047faf50000-0x0047faf7ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fafa0000-0x0047fafbffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fff90000-0x0047fff9ffff [Runtime Code       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
> [    0.000000] efi:   0x0047fffb0000-0x0047fffdffff [Runtime Data       |RUN|  |  |XP|  |  |  |   |WB|WT|WC|UC]*
>

This memory map looks utterly broken. Assuming you are using
Tianocore, does it work when setting
gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable to FALSE in
your platform?

[PATCH v2 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 1626474567ac..346d01ad7cca 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -623,6 +623,10 @@ void efi_native_runtime_setup(void);
 	EFI_GUID(0x3152bca5, 0xeade, 0x433d, \
 		 0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
 
+#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;
@@ -847,6 +851,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:
  */
@@ -868,6 +880,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 v2 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 | 170 ++++++++++++++++++++
 include/linux/efi.h            |  13 ++
 3 files changed, 184 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..c55683937f4a
--- /dev/null
+++ b/drivers/firmware/efi/memattr.c
@@ -0,0 +1,170 @@
+/*
+ * 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.
+ */
+
+#define pr_fmt(fmt)	"efi: " fmt
+
+#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;
+	}
+
+	if (tbl->version > 1) {
+		pr_warn("Unexpected EFI Memory Attribute table version %d\n",
+			tbl->version);
+		tbl_size = 0;
+		goto unmap;
+	}
+
+	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
+	memblock_reserve(efi.mem_attr_table, tbl_size);
+
+unmap:
+	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 entry_is_valid(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/Data\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))) {
+		/*
+		 * Since arm64 may execute with page sizes of up to 64 KB, the
+		 * UEFI spec mandates that RuntimeServices memory regions must
+		 * be 64 KB aligned. We need to validate this here since we will
+		 * not be able to tighten permissions on such regions without
+		 * affecting adjacent regions.
+		 */
+		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)
+			continue;
+
+		/*
+		 * 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;
+}
+
+/*
+ * To be called after the EFI page tables have been populated. If a memory
+ * attributes table is available, its contents will be used to update the
+ * mappings with tightened permissions as described by the table.
+ * This requires the UEFI memory map to have already been populated with
+ * virtual addresses.
+ */
+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 = entry_is_valid((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 346d01ad7cca..277a9bb4e587 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -970,6 +970,19 @@ extern void __init efi_fake_memmap(void);
 static inline void efi_fake_memmap(void) { }
 #endif
 
+/*
+ * efi_memattr_perm_setter - arch specific callback function passed into
+ *                           efi_memattr_apply_permissions() that updates the
+ *                           mapping permissions described by the second
+ *                           argument in the page tables referrred to by the
+ *                           first argument
+ */
+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

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

[Ard Biesheuvel]

On 30 March 2016 at 18:38, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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 | 170 ++++++++++++++++++++
>  include/linux/efi.h            |  13 ++
>  3 files changed, 184 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..c55683937f4a
> --- /dev/null
> +++ b/drivers/firmware/efi/memattr.c
> @@ -0,0 +1,170 @@
> +/*
> + * 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.
> + */
> +
> +#define pr_fmt(fmt)    "efi: " fmt
> +
> +#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;
> +       }
> +
> +       if (tbl->version > 1) {
> +               pr_warn("Unexpected EFI Memory Attribute table version %d\n",
> +                       tbl->version);
> +               tbl_size = 0;
> +               goto unmap;
> +       }
> +
> +       tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
> +       memblock_reserve(efi.mem_attr_table, tbl_size);
> +
> +unmap:
> +       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 entry_is_valid(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/Data\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))) {
> +               /*
> +                * Since arm64 may execute with page sizes of up to 64 KB, the
> +                * UEFI spec mandates that RuntimeServices memory regions must
> +                * be 64 KB aligned. We need to validate this here since we will
> +                * not be able to tighten permissions on such regions without
> +                * affecting adjacent regions.
> +                */
> +               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)
> +                       continue;
> +
> +               /*
> +                * 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;
> +}
> +
> +/*
> + * To be called after the EFI page tables have been populated. If a memory
> + * attributes table is available, its contents will be used to update the
> + * mappings with tightened permissions as described by the table.
> + * This requires the UEFI memory map to have already been populated with
> + * virtual addresses.
> + */
> +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 = entry_is_valid((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 346d01ad7cca..277a9bb4e587 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -970,6 +970,19 @@ extern void __init efi_fake_memmap(void);
>  static inline void efi_fake_memmap(void) { }
>  #endif
>
> +/*
> + * efi_memattr_perm_setter - arch specific callback function passed into
> + *                           efi_memattr_apply_permissions() that updates the
> + *                           mapping permissions described by the second
> + *                           argument in the page tables referrred to by the
> + *                           first argument
> + */
> +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
>

This patch needs the following folded on top to deal with tables where
the memory attribute table entry has not counterpart in the memory map

diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index c55683937f4a..4dfef78e64d8 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -61,6 +61,8 @@ static bool entry_is_valid(const efi_memory_desc_t
*in, efi_memory_desc_t *out)
        u64 in_size = in->num_pages << EFI_PAGE_SHIFT;
        efi_memory_desc_t *md;

+       *out = *in;
+
        if (in->type != EFI_RUNTIME_SERVICES_CODE &&
            in->type != EFI_RUNTIME_SERVICES_DATA) {
                pr_warn("MEMATTR table entry type should be
RuntimeServiceCode/Data\n");
@@ -114,11 +116,11 @@ static bool entry_is_valid(const
efi_memory_desc_t *in, efi_memory_desc_t *out)
                        return false;
                }

-               *out = *in;
                out->virt_addr = in_paddr +
                                 (md->virt_addr - md->phys_addr);
                return true;
        }
+       pr_warn("MEMATTR table entry does not having a matching entry
in the memory map\n");
        return false;
 }

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

[Matt Fleming]

On Wed, 30 Mar, at 06:38:52PM, 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 | 170 ++++++++++++++++++++
>  include/linux/efi.h            |  13 ++
>  3 files changed, 184 insertions(+), 1 deletion(-)
 
This commit breaks ia64 because it doesn't provide the global 'memmap'
variable,

  drivers/built-in.o: In function `efi_memattr_apply_permissions':
  (.init.text+0x1f2b0): undefined reference to `memmap'
  drivers/built-in.o: In function `efi_memattr_apply_permissions':
  (.init.text+0x1f2b1): undefined reference to `memmap'

I was surprised that the kbuild bot didn't let you know, but then I
noticed that LKML isn't on Cc. This is yet another reminder that we
need to drop 'memmap' on the floor. I'll send out the two patches I
have to do that. 

Apart from that, this looks pretty good. There are a few comments
below, but they're trivial changes that, if you agree, I can just fold
in when applying this patch. If not, feel free to rework it.

> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> new file mode 100644
> index 000000000000..c55683937f4a
> --- /dev/null
> +++ b/drivers/firmware/efi/memattr.c
> @@ -0,0 +1,170 @@
> +/*
> + * 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.
> + */
> +
> +#define pr_fmt(fmt)	"efi: " fmt
> +
> +#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;
> +	}
> +
> +	if (tbl->version > 1) {
> +		pr_warn("Unexpected EFI Memory Attribute table version %d\n",
> +			tbl->version);
> +		tbl_size = 0;

Isn't it unnecessary to reset 'tbl_size'?

> +		goto unmap;
> +	}

We should check that the desc_size values match between
efi_memory_attributes_table_t and efi_memory_desc_t while we're here
validating the table anyway.

> +
> +	tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
> +	memblock_reserve(efi.mem_attr_table, tbl_size);
> +
> +unmap:
> +	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 entry_is_valid(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/Data\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))) {
> +		/*
> +		 * Since arm64 may execute with page sizes of up to 64 KB, the
> +		 * UEFI spec mandates that RuntimeServices memory regions must
> +		 * be 64 KB aligned. We need to validate this here since we will
> +		 * not be able to tighten permissions on such regions without
> +		 * affecting adjacent regions.
> +		 */
> +		pr_warn("MEMATTR table entry misaligned\n");
> +		return false;
> +	}

Out of curiosity did you check whether the compiler optimises this out
when PAGE_SIZE <= EFI_PAGE_SIZE? I would expect so, and if you haven't
already done it already I can check.

> +
> +	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)
> +			continue;
> +
> +		/*
> +		 * 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;
> +		}

Here's an interesting tidbit: We never use "UEFI" in strings on x86,
we always use "EFI", and of particular relevance to this feature is
that "EFI" is used to describe both the memory and memory attribute
table in the spec.

How do you feel about the patch at the end of this email?

While I'm OK with breaking the 80 column rule in exceptional
circumstances, it was pretty straight forward to update the pr_fmt()
macro and cut out a few words from the strings to keep this all within
80 columns.

> +
> +		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);

For the benefit of future search-and-replace patches, we should use
md_paddr here instead of md->phys_addr.

> +		return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * To be called after the EFI page tables have been populated. If a memory
> + * attributes table is available, its contents will be used to update the
> + * mappings with tightened permissions as described by the table.
> + * This requires the UEFI memory map to have already been populated with
> + * virtual addresses.
> + */
> +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;

We need to check efi_enabled(EFI_MEMMAP) before we start traversing
the memory map in entry_is_valid().
 
> +
> +	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 = entry_is_valid((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));

This needs to include some indication of whether we printed this
because it was invalid or debug EFI_DBG is set. I've inserted a "!" at
the start of the line if it's invalid but if you've got other ideas
let me know.

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 346d01ad7cca..277a9bb4e587 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -970,6 +970,19 @@ extern void __init efi_fake_memmap(void);
>  static inline void efi_fake_memmap(void) { }
>  #endif
>  
> +/*
> + * efi_memattr_perm_setter - arch specific callback function passed into
> + *                           efi_memattr_apply_permissions() that updates the
> + *                           mapping permissions described by the second
> + *                           argument in the page tables referrred to by the

Too many 'r's in referred.

---

diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
index 23973868e186..a983c1b63f07 100644
--- a/drivers/firmware/efi/memattr.c
+++ b/drivers/firmware/efi/memattr.c
@@ -6,7 +6,7 @@
  * published by the Free Software Foundation.
  */
 
-#define pr_fmt(fmt)	"efi: " fmt
+#define pr_fmt(fmt)	"efi: memattr: " fmt
 
 #include <linux/efi.h>
 #include <linux/init.h>
@@ -30,15 +30,20 @@ int __init efi_memattr_init(void)
 
 	tbl = early_memremap(efi.mem_attr_table, sizeof(*tbl));
 	if (!tbl) {
-		pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
+		pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n",
 		       efi.mem_attr_table);
 		return -ENOMEM;
 	}
 
 	if (tbl->version > 1) {
-		pr_warn("Unexpected EFI Memory Attribute table version %d\n",
+		pr_warn("Unexpected EFI Memory Attributes table version %d\n",
 			tbl->version);
-		tbl_size = 0;
+		goto unmap;
+	}
+
+	if (tbl->desc_size != efi.memmap->desc_size) {
+		pr_warn("Descriptor size different from EFI memory map %d\n",
+			tbl->desc_size);
 		goto unmap;
 	}
 
@@ -65,12 +70,12 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)
 
 	if (in->type != EFI_RUNTIME_SERVICES_CODE &&
 	    in->type != EFI_RUNTIME_SERVICES_DATA) {
-		pr_warn("MEMATTR table entry type should be RuntimeServiceCode/Data\n");
+		pr_warn("Entry type should be RuntimeServiceCode/Data\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");
+		pr_warn("Entry attributes invalid: RO and XP bits both cleared\n");
 		return false;
 	}
 
@@ -84,7 +89,7 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)
 		 * not be able to tighten permissions on such regions without
 		 * affecting adjacent regions.
 		 */
-		pr_warn("MEMATTR table entry misaligned\n");
+		pr_warn("Entry address region misaligned\n");
 		return false;
 	}
 
@@ -107,22 +112,21 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)
 		 * 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");
+			pr_warn("Entry covers multiple EFI 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");
+			pr_warn("Entry type deviates from EFI memory map region type\n");
 			return false;
 		}
 
-		out->virt_addr = in_paddr +
-				 (md->virt_addr - md->phys_addr);
+		out->virt_addr = in_paddr + (md->virt_addr - md_paddr);
+
 		return true;
 	}
 
-	pr_warn("MEMATTR table entry does not having a matching entry in the memory map\n");
-
+	pr_warn("No matching entry found in the EFI memory map\n");
 	return false;
 }
 
@@ -142,15 +146,24 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 	if (tbl_size <= sizeof(*tbl))
 		return 0;
 
+	/*
+	 * We need the EFI memory map to be setup so we can use it to
+	 * lookup the virtual addresses of all entries in the  of EFI
+	 * Memory Attributes table. If it isn't available, this
+	 * function should not be called.
+	 */
+	if (WARN_ON(!efi_enabled(EFI_MEMMAP)))
+		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",
+		pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n",
 		       efi.mem_attr_table);
 		return -ENOMEM;
 	}
 
 	if (efi_enabled(EFI_DBG))
-		pr_info("Processing UEFI Memory Attributes table:\n");
+		pr_info("Processing EFI Memory Attributes table:\n");
 
 	for (i = ret = 0; ret == 0 && i < tbl->num_entries; i++) {
 		efi_memory_desc_t md;
@@ -162,7 +175,8 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
 				       &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,
+			pr_info("%s 0x%012llx-0x%012llx %s\n",
+				valid ? "" : "!", md.phys_addr,
 				md.phys_addr + size - 1,
 				efi_md_typeattr_format(buf, sizeof(buf), &md));
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index bb404979f4bd..c80895e4e505 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -974,8 +974,8 @@ static inline void efi_fake_memmap(void) { }
  * efi_memattr_perm_setter - arch specific callback function passed into
  *                           efi_memattr_apply_permissions() that updates the
  *                           mapping permissions described by the second
- *                           argument in the page tables referrred to by the
- *                           first argument
+ *                           argument in the page tables referred to by the
+ *                           first argument.
  */
 typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
 

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

[Ard Biesheuvel]

On 8 April 2016 at 17:56, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Wed, 30 Mar, at 06:38:52PM, 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 | 170 ++++++++++++++++++++
>>  include/linux/efi.h            |  13 ++
>>  3 files changed, 184 insertions(+), 1 deletion(-)
>
> This commit breaks ia64 because it doesn't provide the global 'memmap'
> variable,
>
>   drivers/built-in.o: In function `efi_memattr_apply_permissions':
>   (.init.text+0x1f2b0): undefined reference to `memmap'
>   drivers/built-in.o: In function `efi_memattr_apply_permissions':
>   (.init.text+0x1f2b1): undefined reference to `memmap'
>
> I was surprised that the kbuild bot didn't let you know, but then I
> noticed that LKML isn't on Cc. This is yet another reminder that we
> need to drop 'memmap' on the floor. I'll send out the two patches I
> have to do that.
>
> Apart from that, this looks pretty good. There are a few comments
> below, but they're trivial changes that, if you agree, I can just fold
> in when applying this patch. If not, feel free to rework it.
>
>> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
>> new file mode 100644
>> index 000000000000..c55683937f4a
>> --- /dev/null
>> +++ b/drivers/firmware/efi/memattr.c
>> @@ -0,0 +1,170 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +#define pr_fmt(fmt)  "efi: " fmt
>> +
>> +#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;
>> +     }
>> +
>> +     if (tbl->version > 1) {
>> +             pr_warn("Unexpected EFI Memory Attribute table version %d\n",
>> +                     tbl->version);
>> +             tbl_size = 0;
>
> Isn't it unnecessary to reset 'tbl_size'?
>

Indeed.

>> +             goto unmap;
>> +     }
>
> We should check that the desc_size values match between
> efi_memory_attributes_table_t and efi_memory_desc_t while we're here
> validating the table anyway.
>

The spec does not actually mandate that, and I do know that the
Tianocore code deliberately uses a larger value for desc_size in
GetMemoryMap() to catch inadvertent uses of sizeof(). I am not sure if
the memory attribute table code does the same, and it seems dangerous
to assume that to be the case in general.

>> +
>> +     tbl_size = sizeof(*tbl) + tbl->num_entries * tbl->desc_size;
>> +     memblock_reserve(efi.mem_attr_table, tbl_size);
>> +
>> +unmap:
>> +     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 entry_is_valid(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/Data\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))) {
>> +             /*
>> +              * Since arm64 may execute with page sizes of up to 64 KB, the
>> +              * UEFI spec mandates that RuntimeServices memory regions must
>> +              * be 64 KB aligned. We need to validate this here since we will
>> +              * not be able to tighten permissions on such regions without
>> +              * affecting adjacent regions.
>> +              */
>> +             pr_warn("MEMATTR table entry misaligned\n");
>> +             return false;
>> +     }
>
> Out of curiosity did you check whether the compiler optimises this out
> when PAGE_SIZE <= EFI_PAGE_SIZE? I would expect so, and if you haven't
> already done it already I can check.
>

Yes, it does.

>> +
>> +     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)
>> +                     continue;
>> +
>> +             /*
>> +              * 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;
>> +             }
>
> Here's an interesting tidbit: We never use "UEFI" in strings on x86,
> we always use "EFI", and of particular relevance to this feature is
> that "EFI" is used to describe both the memory and memory attribute
> table in the spec.
>
> How do you feel about the patch at the end of this email?
>

The patch looks fine to me, other than my concern above.

> While I'm OK with breaking the 80 column rule in exceptional
> circumstances, it was pretty straight forward to update the pr_fmt()
> macro and cut out a few words from the strings to keep this all within
> 80 columns.
>

I think there may be a cultural difference here between team-ARM and
team-x86 (i.e., your upstream). I don't remember ever getting any
complaints about overlong lines, even for actual code, and for user
visible strings, we tend to follow the documented coding style to
never break them up for the sole purpose of line length.

But I don't mind the shorter strings at all, so please don't hesitate
to fold in your changes.

>> +
>> +             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);
>
> For the benefit of future search-and-replace patches, we should use
> md_paddr here instead of md->phys_addr.
>

OK

>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>> +/*
>> + * To be called after the EFI page tables have been populated. If a memory
>> + * attributes table is available, its contents will be used to update the
>> + * mappings with tightened permissions as described by the table.
>> + * This requires the UEFI memory map to have already been populated with
>> + * virtual addresses.
>> + */
>> +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;
>
> We need to check efi_enabled(EFI_MEMMAP) before we start traversing
> the memory map in entry_is_valid().
>
>> +
>> +     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 = entry_is_valid((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));
>
> This needs to include some indication of whether we printed this
> because it was invalid or debug EFI_DBG is set. I've inserted a "!" at
> the start of the line if it's invalid but if you've got other ideas
> let me know.
>

OK

>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 346d01ad7cca..277a9bb4e587 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -970,6 +970,19 @@ extern void __init efi_fake_memmap(void);
>>  static inline void efi_fake_memmap(void) { }
>>  #endif
>>
>> +/*
>> + * efi_memattr_perm_setter - arch specific callback function passed into
>> + *                           efi_memattr_apply_permissions() that updates the
>> + *                           mapping permissions described by the second
>> + *                           argument in the page tables referrred to by the
>
> Too many 'r's in referred.
>
> ---
>
> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> index 23973868e186..a983c1b63f07 100644
> --- a/drivers/firmware/efi/memattr.c
> +++ b/drivers/firmware/efi/memattr.c
> @@ -6,7 +6,7 @@
>   * published by the Free Software Foundation.
>   */
>
> -#define pr_fmt(fmt)    "efi: " fmt
> +#define pr_fmt(fmt)    "efi: memattr: " fmt
>
>  #include <linux/efi.h>
>  #include <linux/init.h>
> @@ -30,15 +30,20 @@ int __init efi_memattr_init(void)
>
>         tbl = early_memremap(efi.mem_attr_table, sizeof(*tbl));
>         if (!tbl) {
> -               pr_err("Failed to map EFI Memory Attribute table @ 0x%lx\n",
> +               pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n",
>                        efi.mem_attr_table);
>                 return -ENOMEM;
>         }
>
>         if (tbl->version > 1) {
> -               pr_warn("Unexpected EFI Memory Attribute table version %d\n",
> +               pr_warn("Unexpected EFI Memory Attributes table version %d\n",
>                         tbl->version);
> -               tbl_size = 0;
> +               goto unmap;
> +       }
> +
> +       if (tbl->desc_size != efi.memmap->desc_size) {
> +               pr_warn("Descriptor size different from EFI memory map %d\n",
> +                       tbl->desc_size);
>                 goto unmap;
>         }
>
> @@ -65,12 +70,12 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)
>
>         if (in->type != EFI_RUNTIME_SERVICES_CODE &&
>             in->type != EFI_RUNTIME_SERVICES_DATA) {
> -               pr_warn("MEMATTR table entry type should be RuntimeServiceCode/Data\n");
> +               pr_warn("Entry type should be RuntimeServiceCode/Data\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");
> +               pr_warn("Entry attributes invalid: RO and XP bits both cleared\n");
>                 return false;
>         }
>
> @@ -84,7 +89,7 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)
>                  * not be able to tighten permissions on such regions without
>                  * affecting adjacent regions.
>                  */
> -               pr_warn("MEMATTR table entry misaligned\n");
> +               pr_warn("Entry address region misaligned\n");
>                 return false;
>         }
>
> @@ -107,22 +112,21 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)
>                  * 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");
> +                       pr_warn("Entry covers multiple EFI 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");
> +                       pr_warn("Entry type deviates from EFI memory map region type\n");
>                         return false;
>                 }
>
> -               out->virt_addr = in_paddr +
> -                                (md->virt_addr - md->phys_addr);
> +               out->virt_addr = in_paddr + (md->virt_addr - md_paddr);
> +
>                 return true;
>         }
>
> -       pr_warn("MEMATTR table entry does not having a matching entry in the memory map\n");
> -
> +       pr_warn("No matching entry found in the EFI memory map\n");
>         return false;
>  }
>
> @@ -142,15 +146,24 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
>         if (tbl_size <= sizeof(*tbl))
>                 return 0;
>
> +       /*
> +        * We need the EFI memory map to be setup so we can use it to
> +        * lookup the virtual addresses of all entries in the  of EFI
> +        * Memory Attributes table. If it isn't available, this
> +        * function should not be called.
> +        */
> +       if (WARN_ON(!efi_enabled(EFI_MEMMAP)))
> +               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",
> +               pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n",
>                        efi.mem_attr_table);
>                 return -ENOMEM;
>         }
>
>         if (efi_enabled(EFI_DBG))
> -               pr_info("Processing UEFI Memory Attributes table:\n");
> +               pr_info("Processing EFI Memory Attributes table:\n");
>
>         for (i = ret = 0; ret == 0 && i < tbl->num_entries; i++) {
>                 efi_memory_desc_t md;
> @@ -162,7 +175,8 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
>                                        &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,
> +                       pr_info("%s 0x%012llx-0x%012llx %s\n",
> +                               valid ? "" : "!", md.phys_addr,
>                                 md.phys_addr + size - 1,
>                                 efi_md_typeattr_format(buf, sizeof(buf), &md));
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index bb404979f4bd..c80895e4e505 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -974,8 +974,8 @@ static inline void efi_fake_memmap(void) { }
>   * efi_memattr_perm_setter - arch specific callback function passed into
>   *                           efi_memattr_apply_permissions() that updates the
>   *                           mapping permissions described by the second
> - *                           argument in the page tables referrred to by the
> - *                           first argument
> + *                           argument in the page tables referred to by the
> + *                           first argument.
>   */
>  typedef int (*efi_memattr_perm_setter)(struct mm_struct *, efi_memory_desc_t *);
>

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

[Matt Fleming]

On Mon, 11 Apr, at 04:09:11PM, Ard Biesheuvel wrote:
> 
> The spec does not actually mandate that, and I do know that the
> Tianocore code deliberately uses a larger value for desc_size in
> GetMemoryMap() to catch inadvertent uses of sizeof(). I am not sure if
> the memory attribute table code does the same, and it seems dangerous
> to assume that to be the case in general.
 
The spec may not mandate that, but this code will explode horribly if
efi_memory_desc_t does not accurately describe the entries in either
the EFI Memory Attributes table or the EFI memory map.

How do we ensure that doing,

static bool entry_is_valid(...)
{
	*out = *in;
	...

keeps working? Are we using the table version to guarantee that?

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

[Ard Biesheuvel]

On 12 April 2016 at 21:57, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Mon, 11 Apr, at 04:09:11PM, Ard Biesheuvel wrote:
>>
>> The spec does not actually mandate that, and I do know that the
>> Tianocore code deliberately uses a larger value for desc_size in
>> GetMemoryMap() to catch inadvertent uses of sizeof(). I am not sure if
>> the memory attribute table code does the same, and it seems dangerous
>> to assume that to be the case in general.
>
> The spec may not mandate that, but this code will explode horribly if
> efi_memory_desc_t does not accurately describe the entries in either
> the EFI Memory Attributes table or the EFI memory map.
>
> How do we ensure that doing,
>
> static bool entry_is_valid(...)
> {
>         *out = *in;
>         ...
>
> keeps working? Are we using the table version to guarantee that?

I think it is implied by the spec that this table and the one returned
by GetMemoryMap() use mutually compatible definitions of
EFI_MEMORY_DESCRIPTOR. However, since our definition of the struct
type is based on version 1, we should perhaps add a check for that
separately

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

[Matt Fleming]

On Wed, 13 Apr, at 10:29:23AM, Ard Biesheuvel wrote:
> 
> I think it is implied by the spec that this table and the one returned
> by GetMemoryMap() use mutually compatible definitions of
> EFI_MEMORY_DESCRIPTOR. However, since our definition of the struct
> type is based on version 1, we should perhaps add a check for that
> separately

That makes sense. Could you send a patch for that on top of 'next'
(I've dropped the desc_size check now)?

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

[Ard Biesheuvel]

On 13 April 2016 at 12:24, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Wed, 13 Apr, at 10:29:23AM, Ard Biesheuvel wrote:
>>
>> I think it is implied by the spec that this table and the one returned
>> by GetMemoryMap() use mutually compatible definitions of
>> EFI_MEMORY_DESCRIPTOR. However, since our definition of the struct
>> type is based on version 1, we should perhaps add a check for that
>> separately
>
> That makes sense. Could you send a patch for that on top of 'next'
> (I've dropped the desc_size check now)?
>

OK

[PATCH v2 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 d5df9849544c..9941a4f2f66f 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -204,6 +204,7 @@ void __init efi_init(void)
 		return;
 
 	reserve_regions();
+	efi_memattr_init();
 	early_memunmap(memmap.map, params.mmap_size);
 	memblock_reserve(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 b0cfe208c14c..bf1f6ffc9218 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -75,6 +75,9 @@ static bool __init efi_virtmap_init(void)
 			set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 		}
 	}
+
+	if (efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions))
+		return false;
 	return true;
 }
 
-- 
2.5.0