[PATCH] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[PATCH] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

The new Properties Table feature introduced in UEFIv2.5 may split
memory regions that cover PE/COFF memory images into separate code
and data regions.

Since the relative offset of PE/COFF .text and .data segments cannot
be changed on the fly, this means that we can no longer pad out those
regions to be mappable using 64 KB pages.
Unfortunately, there is no annotation in the UEFI memory map that
identifies data regions that were split off from a code region, so we
must apply this logic to all adjacent runtime regions whose attributes
only differ in the permission bits.

So instead of rounding each memory region to 64 KB alignment at both
ends, only round down regions that are not directly preceded by another
runtime region with the same type attributes. Since the UEFI spec does
not mandate that the memory map be sorted, this means we also need to
sort it first.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

As discussed off list, this is the arm64 side of what we should backport
to stable to prevent firmware that adheres to the current version of the
UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
upon the first OS call into the runtime services.

For arm64, we already map things in order, but since the spec does not mandate
a sorted memory map, we need to sort it to be sure. This also allows us to
easily find adjacent regions with < 64 KB granularity, which the current version
of the spec allows if they only differ in permission bits (which the spec says
are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').

@Matt: I don't know what your timeline is for fixing this on the x86 side, but
perhaps it makes sense to keep those patches together? Your call.

 drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index e29560e6b40b..cb4e9c4de952 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/efi.h>
+#include <linux/sort.h>
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -305,6 +306,13 @@ fail:
  */
 #define EFI_RT_VIRTUAL_BASE	0x40000000
 
+static int cmp_mem_desc(const void *a, const void *b)
+{
+	const efi_memory_desc_t *left = a, *right = b;
+
+	return (left->phys_addr > right->phys_addr) ? 1 : -1;
+}
+
 /*
  * efi_get_virtmap() - create a virtual mapping for the EFI memory map
  *
@@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
 		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
 		     int *count)
 {
+	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
+					 EFI_MEMORY_WC | EFI_MEMORY_UC |
+					 EFI_MEMORY_RUNTIME;
+
 	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
-	efi_memory_desc_t *out = runtime_map;
+	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
 	int l;
 
-	for (l = 0; l < map_size; l += desc_size) {
-		efi_memory_desc_t *in = (void *)memory_map + l;
+	/*
+	 * To work around potential issues with the Properties Table feature
+	 * introduced in UEFI 2.5, which may split PE/COFF executable images
+	 * in memory into several RuntimeServicesCode and RuntimeServicesData
+	 * regions, we need to preserve the relative offsets between adjacent
+	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
+	 * The easiest way to find adjacent regions is to sort the memory map
+	 * before traversing it.
+	 */
+	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
+
+	for (l = 0; l < map_size; l += desc_size, prev = in) {
 		u64 paddr, size;
 
+		in = (void *)memory_map + l;
 		if (!(in->attribute & EFI_MEMORY_RUNTIME))
 			continue;
 
+		paddr = in->phys_addr;
+		size = in->num_pages * EFI_PAGE_SIZE;
+
 		/*
 		 * Make the mapping compatible with 64k pages: this allows
 		 * a 4k page size kernel to kexec a 64k page size kernel and
 		 * vice versa.
 		 */
-		paddr = round_down(in->phys_addr, SZ_64K);
-		size = round_up(in->num_pages * EFI_PAGE_SIZE +
-				in->phys_addr - paddr, SZ_64K);
-
-		/*
-		 * Avoid wasting memory on PTEs by choosing a virtual base that
-		 * is compatible with section mappings if this region has the
-		 * appropriate size and physical alignment. (Sections are 2 MB
-		 * on 4k granule kernels)
-		 */
-		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
-			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+		if (!prev ||
+		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
+		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
+
+			paddr = round_down(in->phys_addr, SZ_64K);
+			size += in->phys_addr - paddr;
+
+			/*
+			 * Avoid wasting memory on PTEs by choosing a virtual
+			 * base that is compatible with section mappings if this
+			 * region has the appropriate size and physical
+			 * alignment. (Sections are 2 MB on 4k granule kernels)
+			 */
+			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+				efi_virt_base = round_up(efi_virt_base, SZ_2M);
+			else
+				efi_virt_base = round_up(efi_virt_base, SZ_64K);
+		}
 
 		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
 		efi_virt_base += size;
-- 
1.9.1

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

The new Properties Table feature introduced in UEFIv2.5 may split
memory regions that cover PE/COFF memory images into separate code
and data regions. Since these regions only differ in the type (runtime
code vs runtime data) and the permission bits, but not in the memory
type attributes (UC/WC/WT/WB), the spec does not require them to be
aligned to 64 KB.

As the relative offset of PE/COFF .text and .data segments cannot be
changed on the fly, this means that we can no longer pad out those
regions to be mappable using 64 KB pages.
Unfortunately, there is no annotation in the UEFI memory map that
identifies data regions that were split off from a code region, so we
must apply this logic to all adjacent runtime regions whose attributes
only differ in the permission bits.

So instead of rounding each memory region to 64 KB alignment at both
ends, only round down regions that are not directly preceded by another
runtime region with the same type attributes. Since the UEFI spec does
not mandate that the memory map be sorted, this means we also need to
sort it first.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

As discussed off list, this is the arm64 side of what we should backport
to stable to prevent firmware that adheres to the current version of the
UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
upon the first OS call into the runtime services.

For arm64, we already map things in order, but since the spec does not mandate
a sorted memory map, we need to sort it to be sure. This also allows us to
easily find adjacent regions with < 64 KB granularity, which the current version
of the spec allows if they only differ in permission bits (which the spec says
are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').

Changes since v1:
- Ensure that we don't inadvertently set the XN bit on the preceding region at
  mapping time if we the OS is running with >4 KB pages.
  
 arch/arm64/kernel/efi.c                 |  3 +-
 drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6eaedd02..13671a9cf016 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
 		 */
 		if (!is_normal_ram(md))
 			prot = __pgprot(PROT_DEVICE_nGnRE);
-		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
+			 !PAGE_ALIGNED(md->phys_addr))
 			prot = PAGE_KERNEL_EXEC;
 		else
 			prot = PAGE_KERNEL;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index e29560e6b40b..cb4e9c4de952 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/efi.h>
+#include <linux/sort.h>
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -305,6 +306,13 @@ fail:
  */
 #define EFI_RT_VIRTUAL_BASE	0x40000000
 
+static int cmp_mem_desc(const void *a, const void *b)
+{
+	const efi_memory_desc_t *left = a, *right = b;
+
+	return (left->phys_addr > right->phys_addr) ? 1 : -1;
+}
+
 /*
  * efi_get_virtmap() - create a virtual mapping for the EFI memory map
  *
@@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
 		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
 		     int *count)
 {
+	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
+					 EFI_MEMORY_WC | EFI_MEMORY_UC |
+					 EFI_MEMORY_RUNTIME;
+
 	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
-	efi_memory_desc_t *out = runtime_map;
+	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
 	int l;
 
-	for (l = 0; l < map_size; l += desc_size) {
-		efi_memory_desc_t *in = (void *)memory_map + l;
+	/*
+	 * To work around potential issues with the Properties Table feature
+	 * introduced in UEFI 2.5, which may split PE/COFF executable images
+	 * in memory into several RuntimeServicesCode and RuntimeServicesData
+	 * regions, we need to preserve the relative offsets between adjacent
+	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
+	 * The easiest way to find adjacent regions is to sort the memory map
+	 * before traversing it.
+	 */
+	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
+
+	for (l = 0; l < map_size; l += desc_size, prev = in) {
 		u64 paddr, size;
 
+		in = (void *)memory_map + l;
 		if (!(in->attribute & EFI_MEMORY_RUNTIME))
 			continue;
 
+		paddr = in->phys_addr;
+		size = in->num_pages * EFI_PAGE_SIZE;
+
 		/*
 		 * Make the mapping compatible with 64k pages: this allows
 		 * a 4k page size kernel to kexec a 64k page size kernel and
 		 * vice versa.
 		 */
-		paddr = round_down(in->phys_addr, SZ_64K);
-		size = round_up(in->num_pages * EFI_PAGE_SIZE +
-				in->phys_addr - paddr, SZ_64K);
-
-		/*
-		 * Avoid wasting memory on PTEs by choosing a virtual base that
-		 * is compatible with section mappings if this region has the
-		 * appropriate size and physical alignment. (Sections are 2 MB
-		 * on 4k granule kernels)
-		 */
-		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
-			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+		if (!prev ||
+		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
+		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
+
+			paddr = round_down(in->phys_addr, SZ_64K);
+			size += in->phys_addr - paddr;
+
+			/*
+			 * Avoid wasting memory on PTEs by choosing a virtual
+			 * base that is compatible with section mappings if this
+			 * region has the appropriate size and physical
+			 * alignment. (Sections are 2 MB on 4k granule kernels)
+			 */
+			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+				efi_virt_base = round_up(efi_virt_base, SZ_2M);
+			else
+				efi_virt_base = round_up(efi_virt_base, SZ_64K);
+		}
 
 		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
 		efi_virt_base += size;
-- 
1.9.1

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

The new Properties Table feature introduced in UEFIv2.5 may split
memory regions that cover PE/COFF memory images into separate code
and data regions. Since these regions only differ in the type (runtime
code vs runtime data) and the permission bits, but not in the memory
type attributes (UC/WC/WT/WB), the spec does not require them to be
aligned to 64 KB.

As the relative offset of PE/COFF .text and .data segments cannot be
changed on the fly, this means that we can no longer pad out those
regions to be mappable using 64 KB pages.
Unfortunately, there is no annotation in the UEFI memory map that
identifies data regions that were split off from a code region, so we
must apply this logic to all adjacent runtime regions whose attributes
only differ in the permission bits.

So instead of rounding each memory region to 64 KB alignment at both
ends, only round down regions that are not directly preceded by another
runtime region with the same type attributes. Since the UEFI spec does
not mandate that the memory map be sorted, this means we also need to
sort it first.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

As discussed off list, this is the arm64 side of what we should backport
to stable to prevent firmware that adheres to the current version of the
UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
upon the first OS call into the runtime services.

For arm64, we already map things in order, but since the spec does not mandate
a sorted memory map, we need to sort it to be sure. This also allows us to
easily find adjacent regions with < 64 KB granularity, which the current version
of the spec allows if they only differ in permission bits (which the spec says
are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').

Changes since v1:
- Ensure that we don't inadvertently set the XN bit on the preceding region at
  mapping time if we the OS is running with >4 KB pages.
  
 arch/arm64/kernel/efi.c                 |  3 +-
 drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6eaedd02..13671a9cf016 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
 		 */
 		if (!is_normal_ram(md))
 			prot = __pgprot(PROT_DEVICE_nGnRE);
-		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
+			 !PAGE_ALIGNED(md->phys_addr))
 			prot = PAGE_KERNEL_EXEC;
 		else
 			prot = PAGE_KERNEL;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index e29560e6b40b..cb4e9c4de952 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/efi.h>
+#include <linux/sort.h>
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -305,6 +306,13 @@ fail:
  */
 #define EFI_RT_VIRTUAL_BASE	0x40000000
 
+static int cmp_mem_desc(const void *a, const void *b)
+{
+	const efi_memory_desc_t *left = a, *right = b;
+
+	return (left->phys_addr > right->phys_addr) ? 1 : -1;
+}
+
 /*
  * efi_get_virtmap() - create a virtual mapping for the EFI memory map
  *
@@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
 		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
 		     int *count)
 {
+	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
+					 EFI_MEMORY_WC | EFI_MEMORY_UC |
+					 EFI_MEMORY_RUNTIME;
+
 	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
-	efi_memory_desc_t *out = runtime_map;
+	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
 	int l;
 
-	for (l = 0; l < map_size; l += desc_size) {
-		efi_memory_desc_t *in = (void *)memory_map + l;
+	/*
+	 * To work around potential issues with the Properties Table feature
+	 * introduced in UEFI 2.5, which may split PE/COFF executable images
+	 * in memory into several RuntimeServicesCode and RuntimeServicesData
+	 * regions, we need to preserve the relative offsets between adjacent
+	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
+	 * The easiest way to find adjacent regions is to sort the memory map
+	 * before traversing it.
+	 */
+	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
+
+	for (l = 0; l < map_size; l += desc_size, prev = in) {
 		u64 paddr, size;
 
+		in = (void *)memory_map + l;
 		if (!(in->attribute & EFI_MEMORY_RUNTIME))
 			continue;
 
+		paddr = in->phys_addr;
+		size = in->num_pages * EFI_PAGE_SIZE;
+
 		/*
 		 * Make the mapping compatible with 64k pages: this allows
 		 * a 4k page size kernel to kexec a 64k page size kernel and
 		 * vice versa.
 		 */
-		paddr = round_down(in->phys_addr, SZ_64K);
-		size = round_up(in->num_pages * EFI_PAGE_SIZE +
-				in->phys_addr - paddr, SZ_64K);
-
-		/*
-		 * Avoid wasting memory on PTEs by choosing a virtual base that
-		 * is compatible with section mappings if this region has the
-		 * appropriate size and physical alignment. (Sections are 2 MB
-		 * on 4k granule kernels)
-		 */
-		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
-			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+		if (!prev ||
+		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
+		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
+
+			paddr = round_down(in->phys_addr, SZ_64K);
+			size += in->phys_addr - paddr;
+
+			/*
+			 * Avoid wasting memory on PTEs by choosing a virtual
+			 * base that is compatible with section mappings if this
+			 * region has the appropriate size and physical
+			 * alignment. (Sections are 2 MB on 4k granule kernels)
+			 */
+			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+				efi_virt_base = round_up(efi_virt_base, SZ_2M);
+			else
+				efi_virt_base = round_up(efi_virt_base, SZ_64K);
+		}
 
 		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
 		efi_virt_base += size;
-- 
1.9.1

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

(snip linux-efi + matt)

On 9 September 2015 at 09:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
>
> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
>
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
>

The x86 counterpart is under discussion here:
http://thread.gmane.org/gmane.linux.kernel.efi/6219

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Matt Fleming]

On Wed, 09 Sep, at 09:06:54AM, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
> 
> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
> 
> For arm64, we already map things in order, but since the spec does not mandate
> a sorted memory map, we need to sort it to be sure. This also allows us to
> easily find adjacent regions with < 64 KB granularity, which the current version
> of the spec allows if they only differ in permission bits (which the spec says
> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
>   
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>  2 files changed, 49 insertions(+), 16 deletions(-)

Thanks Ard. I've picked this up in my 'urgent' queue and tagged it for
stable, but it would be really good if folks on the Cc list could
provdide ACKs and Tested-by tags because this is the kind of kernel
code where dragons lurk.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Matt Fleming]

On Wed, 09 Sep, at 09:06:54AM, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
> 
> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
> 
> For arm64, we already map things in order, but since the spec does not mandate
> a sorted memory map, we need to sort it to be sure. This also allows us to
> easily find adjacent regions with < 64 KB granularity, which the current version
> of the spec allows if they only differ in permission bits (which the spec says
> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
>   
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>  2 files changed, 49 insertions(+), 16 deletions(-)

Thanks Ard. I've picked this up in my 'urgent' queue and tagged it for
stable, but it would be really good if folks on the Cc list could
provdide ACKs and Tested-by tags because this is the kind of kernel
code where dragons lurk.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Salter]

On Wed, 2015-09-09 at 09:06 +0200, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
> 
> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---

Tested-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> 
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
> 
> For arm64, we already map things in order, but since the spec does not mandate
> a sorted memory map, we need to sort it to be sure. This also allows us to
> easily find adjacent regions with < 64 KB granularity, which the current version
> of the spec allows if they only differ in permission bits (which the spec says
> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
>   
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..13671a9cf016 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>  		 */
>  		if (!is_normal_ram(md))
>  			prot = __pgprot(PROT_DEVICE_nGnRE);
> -		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> +			 !PAGE_ALIGNED(md->phys_addr))
>  			prot = PAGE_KERNEL_EXEC;
>  		else
>  			prot = PAGE_KERNEL;
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e29560e6b40b..cb4e9c4de952 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/efi.h>
> +#include <linux/sort.h>
>  #include <asm/efi.h>
>  
>  #include "efistub.h"
> @@ -305,6 +306,13 @@ fail:
>   */
>  #define EFI_RT_VIRTUAL_BASE	0x40000000
>  
> +static int cmp_mem_desc(const void *a, const void *b)
> +{
> +	const efi_memory_desc_t *left = a, *right = b;
> +
> +	return (left->phys_addr > right->phys_addr) ? 1 : -1;
> +}
> +
>  /*
>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>   *
> @@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
>  		     int *count)
>  {
> +	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
> +					 EFI_MEMORY_WC | EFI_MEMORY_UC |
> +					 EFI_MEMORY_RUNTIME;
> +
>  	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
> -	efi_memory_desc_t *out = runtime_map;
> +	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>  	int l;
>  
> -	for (l = 0; l < map_size; l += desc_size) {
> -		efi_memory_desc_t *in = (void *)memory_map + l;
> +	/*
> +	 * To work around potential issues with the Properties Table feature
> +	 * introduced in UEFI 2.5, which may split PE/COFF executable images
> +	 * in memory into several RuntimeServicesCode and RuntimeServicesData
> +	 * regions, we need to preserve the relative offsets between adjacent
> +	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
> +	 * The easiest way to find adjacent regions is to sort the memory map
> +	 * before traversing it.
> +	 */
> +	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> +
> +	for (l = 0; l < map_size; l += desc_size, prev = in) {
>  		u64 paddr, size;
>  
> +		in = (void *)memory_map + l;
>  		if (!(in->attribute & EFI_MEMORY_RUNTIME))
>  			continue;
>  
> +		paddr = in->phys_addr;
> +		size = in->num_pages * EFI_PAGE_SIZE;
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
>  		 * vice versa.
>  		 */
> -		paddr = round_down(in->phys_addr, SZ_64K);
> -		size = round_up(in->num_pages * EFI_PAGE_SIZE +
> -				in->phys_addr - paddr, SZ_64K);
> -
> -		/*
> -		 * Avoid wasting memory on PTEs by choosing a virtual base that
> -		 * is compatible with section mappings if this region has the
> -		 * appropriate size and physical alignment. (Sections are 2 MB
> -		 * on 4k granule kernels)
> -		 */
> -		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> -			efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +		if (!prev ||
> +		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
> +		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
> +
> +			paddr = round_down(in->phys_addr, SZ_64K);
> +			size += in->phys_addr - paddr;
> +
> +			/*
> +			 * Avoid wasting memory on PTEs by choosing a virtual
> +			 * base that is compatible with section mappings if this
> +			 * region has the appropriate size and physical
> +			 * alignment. (Sections are 2 MB on 4k granule kernels)
> +			 */
> +			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> +				efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +			else
> +				efi_virt_base = round_up(efi_virt_base, SZ_64K);
> +		}
>  
>  		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
>  		efi_virt_base += size;

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Salter]

On Wed, 2015-09-09 at 09:06 +0200, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
> 
> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---

Tested-by: Mark Salter <msalter@redhat.com>
Reviewed-by: Mark Salter <msalter@redhat.com>

> 
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
> 
> For arm64, we already map things in order, but since the spec does not mandate
> a sorted memory map, we need to sort it to be sure. This also allows us to
> easily find adjacent regions with < 64 KB granularity, which the current version
> of the spec allows if they only differ in permission bits (which the spec says
> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
>   
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..13671a9cf016 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>  		 */
>  		if (!is_normal_ram(md))
>  			prot = __pgprot(PROT_DEVICE_nGnRE);
> -		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> +			 !PAGE_ALIGNED(md->phys_addr))
>  			prot = PAGE_KERNEL_EXEC;
>  		else
>  			prot = PAGE_KERNEL;
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e29560e6b40b..cb4e9c4de952 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/efi.h>
> +#include <linux/sort.h>
>  #include <asm/efi.h>
>  
>  #include "efistub.h"
> @@ -305,6 +306,13 @@ fail:
>   */
>  #define EFI_RT_VIRTUAL_BASE	0x40000000
>  
> +static int cmp_mem_desc(const void *a, const void *b)
> +{
> +	const efi_memory_desc_t *left = a, *right = b;
> +
> +	return (left->phys_addr > right->phys_addr) ? 1 : -1;
> +}
> +
>  /*
>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>   *
> @@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
>  		     int *count)
>  {
> +	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
> +					 EFI_MEMORY_WC | EFI_MEMORY_UC |
> +					 EFI_MEMORY_RUNTIME;
> +
>  	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
> -	efi_memory_desc_t *out = runtime_map;
> +	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>  	int l;
>  
> -	for (l = 0; l < map_size; l += desc_size) {
> -		efi_memory_desc_t *in = (void *)memory_map + l;
> +	/*
> +	 * To work around potential issues with the Properties Table feature
> +	 * introduced in UEFI 2.5, which may split PE/COFF executable images
> +	 * in memory into several RuntimeServicesCode and RuntimeServicesData
> +	 * regions, we need to preserve the relative offsets between adjacent
> +	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
> +	 * The easiest way to find adjacent regions is to sort the memory map
> +	 * before traversing it.
> +	 */
> +	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> +
> +	for (l = 0; l < map_size; l += desc_size, prev = in) {
>  		u64 paddr, size;
>  
> +		in = (void *)memory_map + l;
>  		if (!(in->attribute & EFI_MEMORY_RUNTIME))
>  			continue;
>  
> +		paddr = in->phys_addr;
> +		size = in->num_pages * EFI_PAGE_SIZE;
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
>  		 * vice versa.
>  		 */
> -		paddr = round_down(in->phys_addr, SZ_64K);
> -		size = round_up(in->num_pages * EFI_PAGE_SIZE +
> -				in->phys_addr - paddr, SZ_64K);
> -
> -		/*
> -		 * Avoid wasting memory on PTEs by choosing a virtual base that
> -		 * is compatible with section mappings if this region has the
> -		 * appropriate size and physical alignment. (Sections are 2 MB
> -		 * on 4k granule kernels)
> -		 */
> -		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> -			efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +		if (!prev ||
> +		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
> +		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
> +
> +			paddr = round_down(in->phys_addr, SZ_64K);
> +			size += in->phys_addr - paddr;
> +
> +			/*
> +			 * Avoid wasting memory on PTEs by choosing a virtual
> +			 * base that is compatible with section mappings if this
> +			 * region has the appropriate size and physical
> +			 * alignment. (Sections are 2 MB on 4k granule kernels)
> +			 */
> +			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> +				efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +			else
> +				efi_virt_base = round_up(efi_virt_base, SZ_64K);
> +		}
>  
>  		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
>  		efi_virt_base += size;

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

Hi,

FWIW I gave this a spin on Seattle and Juno and saw no regressions (both
are pre-2.5 EFI though).

I have some concerns below.

On Wed, Sep 09, 2015 at 08:06:54AM +0100, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.

We should require those to be 64k-aligned for permissions too. I can
imagine vendors getting permissions wrong but things happening to work
for a 64k kernel (where I assume we have to use the superset of all
permissions within a 64k page).

> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
> 
> For arm64, we already map things in order, but since the spec does not mandate
> a sorted memory map, we need to sort it to be sure. This also allows us to
> easily find adjacent regions with < 64 KB granularity, which the current version
> of the spec allows if they only differ in permission bits (which the spec says
> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
>   
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..13671a9cf016 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>  		 */
>  		if (!is_normal_ram(md))
>  			prot = __pgprot(PROT_DEVICE_nGnRE);
> -		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> +			 !PAGE_ALIGNED(md->phys_addr))
>  			prot = PAGE_KERNEL_EXEC;

This looks coarser than necessary. For memory organised like:

0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE 
0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA

We should be able to make the last 64K non-executable, but with this all
128K is executable, unless I've missed something?

Maybe we could do a two-step pass, first mapping the data as
not-executable, then mapping any code pages executable (overriding any
overlapping portions, but only for the overlapping parts).

>  		else
>  			prot = PAGE_KERNEL;
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e29560e6b40b..cb4e9c4de952 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/efi.h>
> +#include <linux/sort.h>

Sort isn't an inline in this header. I thought it wasn't safe to call
arbitary kernel functions from the stub?

>  #include <asm/efi.h>
>  
>  #include "efistub.h"
> @@ -305,6 +306,13 @@ fail:
>   */
>  #define EFI_RT_VIRTUAL_BASE	0x40000000
>  
> +static int cmp_mem_desc(const void *a, const void *b)
> +{
> +	const efi_memory_desc_t *left = a, *right = b;
> +
> +	return (left->phys_addr > right->phys_addr) ? 1 : -1;
> +}

Nit: please chose names to make the relationship between these clearer.
e.g. s/left/mem_a/, s/right/mem_b/.

> +
>  /*
>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>   *
> @@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
>  		     int *count)
>  {
> +	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
> +					 EFI_MEMORY_WC | EFI_MEMORY_UC |
> +					 EFI_MEMORY_RUNTIME;
> +
>  	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
> -	efi_memory_desc_t *out = runtime_map;
> +	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>  	int l;
>  
> -	for (l = 0; l < map_size; l += desc_size) {
> -		efi_memory_desc_t *in = (void *)memory_map + l;
> +	/*
> +	 * To work around potential issues with the Properties Table feature
> +	 * introduced in UEFI 2.5, which may split PE/COFF executable images
> +	 * in memory into several RuntimeServicesCode and RuntimeServicesData
> +	 * regions, we need to preserve the relative offsets between adjacent
> +	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
> +	 * The easiest way to find adjacent regions is to sort the memory map
> +	 * before traversing it.
> +	 */
> +	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> +
> +	for (l = 0; l < map_size; l += desc_size, prev = in) {
>  		u64 paddr, size;
>  
> +		in = (void *)memory_map + l;
>  		if (!(in->attribute & EFI_MEMORY_RUNTIME))
>  			continue;
>  
> +		paddr = in->phys_addr;
> +		size = in->num_pages * EFI_PAGE_SIZE;
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
>  		 * vice versa.
>  		 */
> -		paddr = round_down(in->phys_addr, SZ_64K);
> -		size = round_up(in->num_pages * EFI_PAGE_SIZE +
> -				in->phys_addr - paddr, SZ_64K);
> -
> -		/*
> -		 * Avoid wasting memory on PTEs by choosing a virtual base that
> -		 * is compatible with section mappings if this region has the
> -		 * appropriate size and physical alignment. (Sections are 2 MB
> -		 * on 4k granule kernels)
> -		 */
> -		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> -			efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +		if (!prev ||
> +		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
> +		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
> +

This looks correct, though slightly painful to read. It might be nicer
with helpers helpers like descs_have_same_attrs and
descs_are_contiguous.

Thanks,
Mark.

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

Hi,

FWIW I gave this a spin on Seattle and Juno and saw no regressions (both
are pre-2.5 EFI though).

I have some concerns below.

On Wed, Sep 09, 2015 at 08:06:54AM +0100, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.

We should require those to be 64k-aligned for permissions too. I can
imagine vendors getting permissions wrong but things happening to work
for a 64k kernel (where I assume we have to use the superset of all
permissions within a 64k page).

> As the relative offset of PE/COFF .text and .data segments cannot be
> changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> As discussed off list, this is the arm64 side of what we should backport
> to stable to prevent firmware that adheres to the current version of the
> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
> upon the first OS call into the runtime services.
> 
> For arm64, we already map things in order, but since the spec does not mandate
> a sorted memory map, we need to sort it to be sure. This also allows us to
> easily find adjacent regions with < 64 KB granularity, which the current version
> of the spec allows if they only differ in permission bits (which the spec says
> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
>   
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..13671a9cf016 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>  		 */
>  		if (!is_normal_ram(md))
>  			prot = __pgprot(PROT_DEVICE_nGnRE);
> -		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> +			 !PAGE_ALIGNED(md->phys_addr))
>  			prot = PAGE_KERNEL_EXEC;

This looks coarser than necessary. For memory organised like:

0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE 
0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA

We should be able to make the last 64K non-executable, but with this all
128K is executable, unless I've missed something?

Maybe we could do a two-step pass, first mapping the data as
not-executable, then mapping any code pages executable (overriding any
overlapping portions, but only for the overlapping parts).

>  		else
>  			prot = PAGE_KERNEL;
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e29560e6b40b..cb4e9c4de952 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/efi.h>
> +#include <linux/sort.h>

Sort isn't an inline in this header. I thought it wasn't safe to call
arbitary kernel functions from the stub?

>  #include <asm/efi.h>
>  
>  #include "efistub.h"
> @@ -305,6 +306,13 @@ fail:
>   */
>  #define EFI_RT_VIRTUAL_BASE	0x40000000
>  
> +static int cmp_mem_desc(const void *a, const void *b)
> +{
> +	const efi_memory_desc_t *left = a, *right = b;
> +
> +	return (left->phys_addr > right->phys_addr) ? 1 : -1;
> +}

Nit: please chose names to make the relationship between these clearer.
e.g. s/left/mem_a/, s/right/mem_b/.

> +
>  /*
>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>   *
> @@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		     unsigned long desc_size, efi_memory_desc_t *runtime_map,
>  		     int *count)
>  {
> +	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
> +					 EFI_MEMORY_WC | EFI_MEMORY_UC |
> +					 EFI_MEMORY_RUNTIME;
> +
>  	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
> -	efi_memory_desc_t *out = runtime_map;
> +	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>  	int l;
>  
> -	for (l = 0; l < map_size; l += desc_size) {
> -		efi_memory_desc_t *in = (void *)memory_map + l;
> +	/*
> +	 * To work around potential issues with the Properties Table feature
> +	 * introduced in UEFI 2.5, which may split PE/COFF executable images
> +	 * in memory into several RuntimeServicesCode and RuntimeServicesData
> +	 * regions, we need to preserve the relative offsets between adjacent
> +	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
> +	 * The easiest way to find adjacent regions is to sort the memory map
> +	 * before traversing it.
> +	 */
> +	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> +
> +	for (l = 0; l < map_size; l += desc_size, prev = in) {
>  		u64 paddr, size;
>  
> +		in = (void *)memory_map + l;
>  		if (!(in->attribute & EFI_MEMORY_RUNTIME))
>  			continue;
>  
> +		paddr = in->phys_addr;
> +		size = in->num_pages * EFI_PAGE_SIZE;
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
>  		 * vice versa.
>  		 */
> -		paddr = round_down(in->phys_addr, SZ_64K);
> -		size = round_up(in->num_pages * EFI_PAGE_SIZE +
> -				in->phys_addr - paddr, SZ_64K);
> -
> -		/*
> -		 * Avoid wasting memory on PTEs by choosing a virtual base that
> -		 * is compatible with section mappings if this region has the
> -		 * appropriate size and physical alignment. (Sections are 2 MB
> -		 * on 4k granule kernels)
> -		 */
> -		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> -			efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +		if (!prev ||
> +		    ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
> +		    paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
> +

This looks correct, though slightly painful to read. It might be nicer
with helpers helpers like descs_have_same_attrs and
descs_are_contiguous.

Thanks,
Mark.

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

On 10 September 2015 at 15:22, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi,
>
> FWIW I gave this a spin on Seattle and Juno and saw no regressions (both
> are pre-2.5 EFI though).
>

Thanks!

> I have some concerns below.
>
> On Wed, Sep 09, 2015 at 08:06:54AM +0100, Ard Biesheuvel wrote:
>> The new Properties Table feature introduced in UEFIv2.5 may split
>> memory regions that cover PE/COFF memory images into separate code
>> and data regions. Since these regions only differ in the type (runtime
>> code vs runtime data) and the permission bits, but not in the memory
>> type attributes (UC/WC/WT/WB), the spec does not require them to be
>> aligned to 64 KB.
>
> We should require those to be 64k-aligned for permissions too. I can
> imagine vendors getting permissions wrong but things happening to work
> for a 64k kernel (where I assume we have to use the superset of all
> permissions within a 64k page).
>

Indeed. But note that this patch is /only/ about interoperability with
the current version of the spec. This is not the venue for discussing
improvements in upcoming versions.

>> As the relative offset of PE/COFF .text and .data segments cannot be
>> changed on the fly, this means that we can no longer pad out those
>> regions to be mappable using 64 KB pages.
>> Unfortunately, there is no annotation in the UEFI memory map that
>> identifies data regions that were split off from a code region, so we
>> must apply this logic to all adjacent runtime regions whose attributes
>> only differ in the permission bits.
>>
>> So instead of rounding each memory region to 64 KB alignment at both
>> ends, only round down regions that are not directly preceded by another
>> runtime region with the same type attributes. Since the UEFI spec does
>> not mandate that the memory map be sorted, this means we also need to
>> sort it first.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>
>> As discussed off list, this is the arm64 side of what we should backport
>> to stable to prevent firmware that adheres to the current version of the
>> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
>> upon the first OS call into the runtime services.
>>
>> For arm64, we already map things in order, but since the spec does not mandate
>> a sorted memory map, we need to sort it to be sure. This also allows us to
>> easily find adjacent regions with < 64 KB granularity, which the current version
>> of the spec allows if they only differ in permission bits (which the spec says
>> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
>>
>> Changes since v1:
>> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>>   mapping time if we the OS is running with >4 KB pages.
>>
>>  arch/arm64/kernel/efi.c                 |  3 +-
>>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index e8ca6eaedd02..13671a9cf016 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>>                */
>>               if (!is_normal_ram(md))
>>                       prot = __pgprot(PROT_DEVICE_nGnRE);
>> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
>> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> +                      !PAGE_ALIGNED(md->phys_addr))
>>                       prot = PAGE_KERNEL_EXEC;
>
> This looks coarser than necessary. For memory organised like:
>
> 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE
> 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA
>
> We should be able to make the last 64K non-executable, but with this all
> 128K is executable, unless I've missed something?
>

In theory, yes. But considering that

a) this only affects 64 KB pages kernels, and
b) this patch is intended for -stable

I chose to keep it simple and ignore this, and just relax the
permissions for any region that is not aligned to 64 KB.

Since these regions are only mapped during Runtime Services calls, the
window for abuse is not that large.

> Maybe we could do a two-step pass, first mapping the data as
> not-executable, then mapping any code pages executable (overriding any
> overlapping portions, but only for the overlapping parts).
>

Let me have a go at that.

>>               else
>>                       prot = PAGE_KERNEL;
>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> index e29560e6b40b..cb4e9c4de952 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/efi.h>
>> +#include <linux/sort.h>
>
> Sort isn't an inline in this header. I thought it wasn't safe to call
> arbitary kernel functions from the stub?
>

We call string functions, cache maintenance functions, libfdt
functions etc etc so it seems not everyone got the memo :-)

I agree that treating vmlinux both as a static library and as a
payload from the stub's pov is a bit sloppy, and I do remember
discussing this, but for the life of me, I can't remember the exact
issue, other than the use of adrp/add and adrp/ldr pairs, which we
fixed by setting the PE/COFF section alignment to 4 KB.

>>  #include <asm/efi.h>
>>
>>  #include "efistub.h"
>> @@ -305,6 +306,13 @@ fail:
>>   */
>>  #define EFI_RT_VIRTUAL_BASE  0x40000000
>>
>> +static int cmp_mem_desc(const void *a, const void *b)
>> +{
>> +     const efi_memory_desc_t *left = a, *right = b;
>> +
>> +     return (left->phys_addr > right->phys_addr) ? 1 : -1;
>> +}
>
> Nit: please chose names to make the relationship between these clearer.
> e.g. s/left/mem_a/, s/right/mem_b/.
>

OK, I can do that.

>> +
>>  /*
>>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>>   *
>> @@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>>                    unsigned long desc_size, efi_memory_desc_t *runtime_map,
>>                    int *count)
>>  {
>> +     static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
>> +                                      EFI_MEMORY_WC | EFI_MEMORY_UC |
>> +                                      EFI_MEMORY_RUNTIME;
>> +
>>       u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
>> -     efi_memory_desc_t *out = runtime_map;
>> +     efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>>       int l;
>>
>> -     for (l = 0; l < map_size; l += desc_size) {
>> -             efi_memory_desc_t *in = (void *)memory_map + l;
>> +     /*
>> +      * To work around potential issues with the Properties Table feature
>> +      * introduced in UEFI 2.5, which may split PE/COFF executable images
>> +      * in memory into several RuntimeServicesCode and RuntimeServicesData
>> +      * regions, we need to preserve the relative offsets between adjacent
>> +      * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
>> +      * The easiest way to find adjacent regions is to sort the memory map
>> +      * before traversing it.
>> +      */
>> +     sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
>> +
>> +     for (l = 0; l < map_size; l += desc_size, prev = in) {
>>               u64 paddr, size;
>>
>> +             in = (void *)memory_map + l;
>>               if (!(in->attribute & EFI_MEMORY_RUNTIME))
>>                       continue;
>>
>> +             paddr = in->phys_addr;
>> +             size = in->num_pages * EFI_PAGE_SIZE;
>> +
>>               /*
>>                * Make the mapping compatible with 64k pages: this allows
>>                * a 4k page size kernel to kexec a 64k page size kernel and
>>                * vice versa.
>>                */
>> -             paddr = round_down(in->phys_addr, SZ_64K);
>> -             size = round_up(in->num_pages * EFI_PAGE_SIZE +
>> -                             in->phys_addr - paddr, SZ_64K);
>> -
>> -             /*
>> -              * Avoid wasting memory on PTEs by choosing a virtual base that
>> -              * is compatible with section mappings if this region has the
>> -              * appropriate size and physical alignment. (Sections are 2 MB
>> -              * on 4k granule kernels)
>> -              */
>> -             if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
>> -                     efi_virt_base = round_up(efi_virt_base, SZ_2M);
>> +             if (!prev ||
>> +                 ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
>> +                 paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
>> +
>
> This looks correct, though slightly painful to read. It might be nicer
> with helpers helpers like descs_have_same_attrs and
> descs_are_contiguous.
>

I can do that too. I agree it reads a bit more difficult than necessary.

Thanks,
Ard.

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

On 10 September 2015 at 15:22, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> FWIW I gave this a spin on Seattle and Juno and saw no regressions (both
> are pre-2.5 EFI though).
>

Thanks!

> I have some concerns below.
>
> On Wed, Sep 09, 2015 at 08:06:54AM +0100, Ard Biesheuvel wrote:
>> The new Properties Table feature introduced in UEFIv2.5 may split
>> memory regions that cover PE/COFF memory images into separate code
>> and data regions. Since these regions only differ in the type (runtime
>> code vs runtime data) and the permission bits, but not in the memory
>> type attributes (UC/WC/WT/WB), the spec does not require them to be
>> aligned to 64 KB.
>
> We should require those to be 64k-aligned for permissions too. I can
> imagine vendors getting permissions wrong but things happening to work
> for a 64k kernel (where I assume we have to use the superset of all
> permissions within a 64k page).
>

Indeed. But note that this patch is /only/ about interoperability with
the current version of the spec. This is not the venue for discussing
improvements in upcoming versions.

>> As the relative offset of PE/COFF .text and .data segments cannot be
>> changed on the fly, this means that we can no longer pad out those
>> regions to be mappable using 64 KB pages.
>> Unfortunately, there is no annotation in the UEFI memory map that
>> identifies data regions that were split off from a code region, so we
>> must apply this logic to all adjacent runtime regions whose attributes
>> only differ in the permission bits.
>>
>> So instead of rounding each memory region to 64 KB alignment at both
>> ends, only round down regions that are not directly preceded by another
>> runtime region with the same type attributes. Since the UEFI spec does
>> not mandate that the memory map be sorted, this means we also need to
>> sort it first.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> As discussed off list, this is the arm64 side of what we should backport
>> to stable to prevent firmware that adheres to the current version of the
>> UEFI v2.5 spec with the memprotect feature enabled from blowing up the system
>> upon the first OS call into the runtime services.
>>
>> For arm64, we already map things in order, but since the spec does not mandate
>> a sorted memory map, we need to sort it to be sure. This also allows us to
>> easily find adjacent regions with < 64 KB granularity, which the current version
>> of the spec allows if they only differ in permission bits (which the spec says
>> are 'unused' on AArch64, which could be interpreted as 'allowed but ignored').
>>
>> Changes since v1:
>> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>>   mapping time if we the OS is running with >4 KB pages.
>>
>>  arch/arm64/kernel/efi.c                 |  3 +-
>>  drivers/firmware/efi/libstub/arm-stub.c | 62 +++++++++++++++-----
>>  2 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index e8ca6eaedd02..13671a9cf016 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>>                */
>>               if (!is_normal_ram(md))
>>                       prot = __pgprot(PROT_DEVICE_nGnRE);
>> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
>> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> +                      !PAGE_ALIGNED(md->phys_addr))
>>                       prot = PAGE_KERNEL_EXEC;
>
> This looks coarser than necessary. For memory organised like:
>
> 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE
> 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA
>
> We should be able to make the last 64K non-executable, but with this all
> 128K is executable, unless I've missed something?
>

In theory, yes. But considering that

a) this only affects 64 KB pages kernels, and
b) this patch is intended for -stable

I chose to keep it simple and ignore this, and just relax the
permissions for any region that is not aligned to 64 KB.

Since these regions are only mapped during Runtime Services calls, the
window for abuse is not that large.

> Maybe we could do a two-step pass, first mapping the data as
> not-executable, then mapping any code pages executable (overriding any
> overlapping portions, but only for the overlapping parts).
>

Let me have a go at that.

>>               else
>>                       prot = PAGE_KERNEL;
>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> index e29560e6b40b..cb4e9c4de952 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/efi.h>
>> +#include <linux/sort.h>
>
> Sort isn't an inline in this header. I thought it wasn't safe to call
> arbitary kernel functions from the stub?
>

We call string functions, cache maintenance functions, libfdt
functions etc etc so it seems not everyone got the memo :-)

I agree that treating vmlinux both as a static library and as a
payload from the stub's pov is a bit sloppy, and I do remember
discussing this, but for the life of me, I can't remember the exact
issue, other than the use of adrp/add and adrp/ldr pairs, which we
fixed by setting the PE/COFF section alignment to 4 KB.

>>  #include <asm/efi.h>
>>
>>  #include "efistub.h"
>> @@ -305,6 +306,13 @@ fail:
>>   */
>>  #define EFI_RT_VIRTUAL_BASE  0x40000000
>>
>> +static int cmp_mem_desc(const void *a, const void *b)
>> +{
>> +     const efi_memory_desc_t *left = a, *right = b;
>> +
>> +     return (left->phys_addr > right->phys_addr) ? 1 : -1;
>> +}
>
> Nit: please chose names to make the relationship between these clearer.
> e.g. s/left/mem_a/, s/right/mem_b/.
>

OK, I can do that.

>> +
>>  /*
>>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>>   *
>> @@ -316,34 +324,58 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>>                    unsigned long desc_size, efi_memory_desc_t *runtime_map,
>>                    int *count)
>>  {
>> +     static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
>> +                                      EFI_MEMORY_WC | EFI_MEMORY_UC |
>> +                                      EFI_MEMORY_RUNTIME;
>> +
>>       u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
>> -     efi_memory_desc_t *out = runtime_map;
>> +     efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>>       int l;
>>
>> -     for (l = 0; l < map_size; l += desc_size) {
>> -             efi_memory_desc_t *in = (void *)memory_map + l;
>> +     /*
>> +      * To work around potential issues with the Properties Table feature
>> +      * introduced in UEFI 2.5, which may split PE/COFF executable images
>> +      * in memory into several RuntimeServicesCode and RuntimeServicesData
>> +      * regions, we need to preserve the relative offsets between adjacent
>> +      * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
>> +      * The easiest way to find adjacent regions is to sort the memory map
>> +      * before traversing it.
>> +      */
>> +     sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
>> +
>> +     for (l = 0; l < map_size; l += desc_size, prev = in) {
>>               u64 paddr, size;
>>
>> +             in = (void *)memory_map + l;
>>               if (!(in->attribute & EFI_MEMORY_RUNTIME))
>>                       continue;
>>
>> +             paddr = in->phys_addr;
>> +             size = in->num_pages * EFI_PAGE_SIZE;
>> +
>>               /*
>>                * Make the mapping compatible with 64k pages: this allows
>>                * a 4k page size kernel to kexec a 64k page size kernel and
>>                * vice versa.
>>                */
>> -             paddr = round_down(in->phys_addr, SZ_64K);
>> -             size = round_up(in->num_pages * EFI_PAGE_SIZE +
>> -                             in->phys_addr - paddr, SZ_64K);
>> -
>> -             /*
>> -              * Avoid wasting memory on PTEs by choosing a virtual base that
>> -              * is compatible with section mappings if this region has the
>> -              * appropriate size and physical alignment. (Sections are 2 MB
>> -              * on 4k granule kernels)
>> -              */
>> -             if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
>> -                     efi_virt_base = round_up(efi_virt_base, SZ_2M);
>> +             if (!prev ||
>> +                 ((prev->attribute ^ in->attribute) & mem_type_mask) != 0 ||
>> +                 paddr != (prev->phys_addr + prev->num_pages * EFI_PAGE_SIZE)) {
>> +
>
> This looks correct, though slightly painful to read. It might be nicer
> with helpers helpers like descs_have_same_attrs and
> descs_are_contiguous.
>

I can do that too. I agree it reads a bit more difficult than necessary.

Thanks,
Ard.

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

> >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> >> index e8ca6eaedd02..13671a9cf016 100644
> >> --- a/arch/arm64/kernel/efi.c
> >> +++ b/arch/arm64/kernel/efi.c
> >> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
> >>                */
> >>               if (!is_normal_ram(md))
> >>                       prot = __pgprot(PROT_DEVICE_nGnRE);
> >> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> >> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> >> +                      !PAGE_ALIGNED(md->phys_addr))
> >>                       prot = PAGE_KERNEL_EXEC;
> >
> > This looks coarser than necessary. For memory organised like:
> >
> > 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE
> > 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA
> >
> > We should be able to make the last 64K non-executable, but with this all
> > 128K is executable, unless I've missed something?
> >
> 
> In theory, yes. But considering that
> 
> a) this only affects 64 KB pages kernels, and
> b) this patch is intended for -stable
> 
> I chose to keep it simple and ignore this, and just relax the
> permissions for any region that is not aligned to 64 KB.
> 
> Since these regions are only mapped during Runtime Services calls, the
> window for abuse is not that large.

Ok, that does sound reasonable.

> > Maybe we could do a two-step pass, first mapping the data as
> > not-executable, then mapping any code pages executable (overriding any
> > overlapping portions, but only for the overlapping parts).
> >
> 
> Let me have a go at that.

Cheers!

> >>               else
> >>                       prot = PAGE_KERNEL;
> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> >> index e29560e6b40b..cb4e9c4de952 100644
> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> >> @@ -13,6 +13,7 @@
> >>   */
> >>
> >>  #include <linux/efi.h>
> >> +#include <linux/sort.h>
> >
> > Sort isn't an inline in this header. I thought it wasn't safe to call
> > arbitary kernel functions from the stub?
> >
> 
> We call string functions, cache maintenance functions, libfdt
> functions etc etc so it seems not everyone got the memo :-)
> 
> I agree that treating vmlinux both as a static library and as a
> payload from the stub's pov is a bit sloppy, and I do remember
> discussing this, but for the life of me, I can't remember the exact
> issue, other than the use of adrp/add and adrp/ldr pairs, which we
> fixed by setting the PE/COFF section alignment to 4 KB.

I only had a vague recollection that there was a problem, which I
thought was more to do with potential use of absolute kernel virtual
addresses, which would be incorrect in the context of an EFI
application.

Digging a bit, the stub code itself is safe due to commit
f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that
isn't necessarily true of anything it calls (libfdt uses callbacks in
several places). I think the cache functions we call are all raw asm
which is position-oblivious.

We do seem to be ok so far, however. Maybe we just need to keep an eye
out.

Thanks,
Mark.

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

> >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> >> index e8ca6eaedd02..13671a9cf016 100644
> >> --- a/arch/arm64/kernel/efi.c
> >> +++ b/arch/arm64/kernel/efi.c
> >> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
> >>                */
> >>               if (!is_normal_ram(md))
> >>                       prot = __pgprot(PROT_DEVICE_nGnRE);
> >> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> >> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> >> +                      !PAGE_ALIGNED(md->phys_addr))
> >>                       prot = PAGE_KERNEL_EXEC;
> >
> > This looks coarser than necessary. For memory organised like:
> >
> > 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE
> > 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA
> >
> > We should be able to make the last 64K non-executable, but with this all
> > 128K is executable, unless I've missed something?
> >
> 
> In theory, yes. But considering that
> 
> a) this only affects 64 KB pages kernels, and
> b) this patch is intended for -stable
> 
> I chose to keep it simple and ignore this, and just relax the
> permissions for any region that is not aligned to 64 KB.
> 
> Since these regions are only mapped during Runtime Services calls, the
> window for abuse is not that large.

Ok, that does sound reasonable.

> > Maybe we could do a two-step pass, first mapping the data as
> > not-executable, then mapping any code pages executable (overriding any
> > overlapping portions, but only for the overlapping parts).
> >
> 
> Let me have a go at that.

Cheers!

> >>               else
> >>                       prot = PAGE_KERNEL;
> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> >> index e29560e6b40b..cb4e9c4de952 100644
> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> >> @@ -13,6 +13,7 @@
> >>   */
> >>
> >>  #include <linux/efi.h>
> >> +#include <linux/sort.h>
> >
> > Sort isn't an inline in this header. I thought it wasn't safe to call
> > arbitary kernel functions from the stub?
> >
> 
> We call string functions, cache maintenance functions, libfdt
> functions etc etc so it seems not everyone got the memo :-)
> 
> I agree that treating vmlinux both as a static library and as a
> payload from the stub's pov is a bit sloppy, and I do remember
> discussing this, but for the life of me, I can't remember the exact
> issue, other than the use of adrp/add and adrp/ldr pairs, which we
> fixed by setting the PE/COFF section alignment to 4 KB.

I only had a vague recollection that there was a problem, which I
thought was more to do with potential use of absolute kernel virtual
addresses, which would be incorrect in the context of an EFI
application.

Digging a bit, the stub code itself is safe due to commit
f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that
isn't necessarily true of anything it calls (libfdt uses callbacks in
several places). I think the cache functions we call are all raw asm
which is position-oblivious.

We do seem to be ok so far, however. Maybe we just need to keep an eye
out.

Thanks,
Mark.

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

On 10 September 2015 at 16:04, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> >> index e8ca6eaedd02..13671a9cf016 100644
>> >> --- a/arch/arm64/kernel/efi.c
>> >> +++ b/arch/arm64/kernel/efi.c
>> >> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>> >>                */
>> >>               if (!is_normal_ram(md))
>> >>                       prot = __pgprot(PROT_DEVICE_nGnRE);
>> >> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
>> >> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> >> +                      !PAGE_ALIGNED(md->phys_addr))
>> >>                       prot = PAGE_KERNEL_EXEC;
>> >
>> > This looks coarser than necessary. For memory organised like:
>> >
>> > 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE
>> > 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA
>> >
>> > We should be able to make the last 64K non-executable, but with this all
>> > 128K is executable, unless I've missed something?
>> >
>>
>> In theory, yes. But considering that
>>
>> a) this only affects 64 KB pages kernels, and
>> b) this patch is intended for -stable
>>
>> I chose to keep it simple and ignore this, and just relax the
>> permissions for any region that is not aligned to 64 KB.
>>
>> Since these regions are only mapped during Runtime Services calls, the
>> window for abuse is not that large.
>
> Ok, that does sound reasonable.
>
>> > Maybe we could do a two-step pass, first mapping the data as
>> > not-executable, then mapping any code pages executable (overriding any
>> > overlapping portions, but only for the overlapping parts).
>> >
>>
>> Let me have a go at that.
>
> Cheers!
>

OK so what we could do is the following:

------------8<--------------
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6eaedd02..39fa2a70a7f1 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -233,6 +233,7 @@ void __init efi_init(void)
 static bool __init efi_virtmap_init(void)
 {
        efi_memory_desc_t *md;
+       u64 prev_end = 0;

        for_each_efi_memory_desc(&memmap, md) {
                u64 paddr, npages, size;
@@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void)
                 * executable, everything else can be mapped with the XN bits
                 * set.
                 */
-               if (!is_normal_ram(md))
+               if (!is_normal_ram(md)) {
                        prot = __pgprot(PROT_DEVICE_nGnRE);
-               else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+               } else if (md->type == EFI_RUNTIME_SERVICES_CODE) {
                        prot = PAGE_KERNEL_EXEC;
-               else
+               } else {
+                       /*
+                        * If we are running with >4 KB pages and the current
+                        * region shares a page frame with the preceding one,
+                        * we should not map the leading page again since doing
+                        * so may take its executable permissions away.
+                        */
+                       if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) {
+                               paddr += PAGE_SIZE;
+                               size -= PAGE_SIZE;
+                               if (!size)
+                                       continue;
+                       }
                        prot = PAGE_KERNEL;
-
+               }
+               prev_end = paddr + size;
                create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
        }
        return true;
------------8<--------------

This will ensure that only the pages that are shared between 2 or more
regions may have their permissions upgraded, but only if any of these
regions requires it.

I prefer the much simpler previous version, though, and I think it is
more suitable for -stable. I can always follow up with an improvement
like this for v4.3-late.

>> >>               else
>> >>                       prot = PAGE_KERNEL;
>> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> >> index e29560e6b40b..cb4e9c4de952 100644
>> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> >> @@ -13,6 +13,7 @@
>> >>   */
>> >>
>> >>  #include <linux/efi.h>
>> >> +#include <linux/sort.h>
>> >
>> > Sort isn't an inline in this header. I thought it wasn't safe to call
>> > arbitary kernel functions from the stub?
>> >
>>
>> We call string functions, cache maintenance functions, libfdt
>> functions etc etc so it seems not everyone got the memo :-)
>>
>> I agree that treating vmlinux both as a static library and as a
>> payload from the stub's pov is a bit sloppy, and I do remember
>> discussing this, but for the life of me, I can't remember the exact
>> issue, other than the use of adrp/add and adrp/ldr pairs, which we
>> fixed by setting the PE/COFF section alignment to 4 KB.
>
> I only had a vague recollection that there was a problem, which I
> thought was more to do with potential use of absolute kernel virtual
> addresses, which would be incorrect in the context of an EFI
> application.
>

That was it, of course. Unlike the x86 stub, which is built with -fPIC
(as is the ARM decompressor, btw), the arm64 kernel is position
dependent. Fortunately, the small code model is mostly position
independent by default, but it would be good if we could spot any
problems at build time.

> Digging a bit, the stub code itself is safe due to commit
> f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that

libstub is linked into vmlinux so that does not make a different at all

> isn't necessarily true of anything it calls (libfdt uses callbacks in
> several places). I think the cache functions we call are all raw asm
> which is position-oblivious.
>

I remember looking into this when doing the BE port.

> We do seem to be ok so far, however. Maybe we just need to keep an eye
> out.
>

I'd much rather restrict the code that goes into the stub somehow than
deal with any absolute references. Perhaps we could reuse some of the
section mismatch code in some way to tag certain code as stub-safe and
do a verification pass on the binary.

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

On 10 September 2015 at 16:04, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> >> index e8ca6eaedd02..13671a9cf016 100644
>> >> --- a/arch/arm64/kernel/efi.c
>> >> +++ b/arch/arm64/kernel/efi.c
>> >> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>> >>                */
>> >>               if (!is_normal_ram(md))
>> >>                       prot = __pgprot(PROT_DEVICE_nGnRE);
>> >> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
>> >> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> >> +                      !PAGE_ALIGNED(md->phys_addr))
>> >>                       prot = PAGE_KERNEL_EXEC;
>> >
>> > This looks coarser than necessary. For memory organised like:
>> >
>> > 0x00000000 - 0x0000F000 (60KiB) : EFI_RUNTIME_SERVICES_CODE
>> > 0x0000F000 - 0x00020000 (68KiB) : EFI_RUNTIME_SERVICES_DATA
>> >
>> > We should be able to make the last 64K non-executable, but with this all
>> > 128K is executable, unless I've missed something?
>> >
>>
>> In theory, yes. But considering that
>>
>> a) this only affects 64 KB pages kernels, and
>> b) this patch is intended for -stable
>>
>> I chose to keep it simple and ignore this, and just relax the
>> permissions for any region that is not aligned to 64 KB.
>>
>> Since these regions are only mapped during Runtime Services calls, the
>> window for abuse is not that large.
>
> Ok, that does sound reasonable.
>
>> > Maybe we could do a two-step pass, first mapping the data as
>> > not-executable, then mapping any code pages executable (overriding any
>> > overlapping portions, but only for the overlapping parts).
>> >
>>
>> Let me have a go at that.
>
> Cheers!
>

OK so what we could do is the following:

------------8<--------------
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6eaedd02..39fa2a70a7f1 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -233,6 +233,7 @@ void __init efi_init(void)
 static bool __init efi_virtmap_init(void)
 {
        efi_memory_desc_t *md;
+       u64 prev_end = 0;

        for_each_efi_memory_desc(&memmap, md) {
                u64 paddr, npages, size;
@@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void)
                 * executable, everything else can be mapped with the XN bits
                 * set.
                 */
-               if (!is_normal_ram(md))
+               if (!is_normal_ram(md)) {
                        prot = __pgprot(PROT_DEVICE_nGnRE);
-               else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+               } else if (md->type == EFI_RUNTIME_SERVICES_CODE) {
                        prot = PAGE_KERNEL_EXEC;
-               else
+               } else {
+                       /*
+                        * If we are running with >4 KB pages and the current
+                        * region shares a page frame with the preceding one,
+                        * we should not map the leading page again since doing
+                        * so may take its executable permissions away.
+                        */
+                       if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) {
+                               paddr += PAGE_SIZE;
+                               size -= PAGE_SIZE;
+                               if (!size)
+                                       continue;
+                       }
                        prot = PAGE_KERNEL;
-
+               }
+               prev_end = paddr + size;
                create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
        }
        return true;
------------8<--------------

This will ensure that only the pages that are shared between 2 or more
regions may have their permissions upgraded, but only if any of these
regions requires it.

I prefer the much simpler previous version, though, and I think it is
more suitable for -stable. I can always follow up with an improvement
like this for v4.3-late.

>> >>               else
>> >>                       prot = PAGE_KERNEL;
>> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> >> index e29560e6b40b..cb4e9c4de952 100644
>> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> >> @@ -13,6 +13,7 @@
>> >>   */
>> >>
>> >>  #include <linux/efi.h>
>> >> +#include <linux/sort.h>
>> >
>> > Sort isn't an inline in this header. I thought it wasn't safe to call
>> > arbitary kernel functions from the stub?
>> >
>>
>> We call string functions, cache maintenance functions, libfdt
>> functions etc etc so it seems not everyone got the memo :-)
>>
>> I agree that treating vmlinux both as a static library and as a
>> payload from the stub's pov is a bit sloppy, and I do remember
>> discussing this, but for the life of me, I can't remember the exact
>> issue, other than the use of adrp/add and adrp/ldr pairs, which we
>> fixed by setting the PE/COFF section alignment to 4 KB.
>
> I only had a vague recollection that there was a problem, which I
> thought was more to do with potential use of absolute kernel virtual
> addresses, which would be incorrect in the context of an EFI
> application.
>

That was it, of course. Unlike the x86 stub, which is built with -fPIC
(as is the ARM decompressor, btw), the arm64 kernel is position
dependent. Fortunately, the small code model is mostly position
independent by default, but it would be good if we could spot any
problems at build time.

> Digging a bit, the stub code itself is safe due to commit
> f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that

libstub is linked into vmlinux so that does not make a different at all

> isn't necessarily true of anything it calls (libfdt uses callbacks in
> several places). I think the cache functions we call are all raw asm
> which is position-oblivious.
>

I remember looking into this when doing the BE port.

> We do seem to be ok so far, however. Maybe we just need to keep an eye
> out.
>

I'd much rather restrict the code that goes into the stub somehow than
deal with any absolute references. Perhaps we could reuse some of the
section mismatch code in some way to tag certain code as stub-safe and
do a verification pass on the binary.

Re: [PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

> OK so what we could do is the following:
> 
> ------------8<--------------
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..39fa2a70a7f1 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -233,6 +233,7 @@ void __init efi_init(void)
>  static bool __init efi_virtmap_init(void)
>  {
>         efi_memory_desc_t *md;
> +       u64 prev_end = 0;
> 
>         for_each_efi_memory_desc(&memmap, md) {
>                 u64 paddr, npages, size;
> @@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void)
>                  * executable, everything else can be mapped with the XN bits
>                  * set.
>                  */
> -               if (!is_normal_ram(md))
> +               if (!is_normal_ram(md)) {
>                         prot = __pgprot(PROT_DEVICE_nGnRE);
> -               else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +               } else if (md->type == EFI_RUNTIME_SERVICES_CODE) {
>                         prot = PAGE_KERNEL_EXEC;
> -               else
> +               } else {
> +                       /*
> +                        * If we are running with >4 KB pages and the current
> +                        * region shares a page frame with the preceding one,
> +                        * we should not map the leading page again since doing
> +                        * so may take its executable permissions away.
> +                        */
> +                       if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) {
> +                               paddr += PAGE_SIZE;
> +                               size -= PAGE_SIZE;
> +                               if (!size)
> +                                       continue;
> +                       }
>                         prot = PAGE_KERNEL;
> -
> +               }
> +               prev_end = paddr + size;
>                 create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
>         }
>         return true;
> ------------8<--------------
> 
> This will ensure that only the pages that are shared between 2 or more
> regions may have their permissions upgraded, but only if any of these
> regions requires it.
> 
> I prefer the much simpler previous version, though, and I think it is
> more suitable for -stable. I can always follow up with an improvement
> like this for v4.3-late.

Ok. Let's go with your previous version for now.

Could you put something in the commit message about this limitation, so
that we don't forget?

> >> >>               else
> >> >>                       prot = PAGE_KERNEL;
> >> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> >> >> index e29560e6b40b..cb4e9c4de952 100644
> >> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
> >> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> >> >> @@ -13,6 +13,7 @@
> >> >>   */
> >> >>
> >> >>  #include <linux/efi.h>
> >> >> +#include <linux/sort.h>
> >> >
> >> > Sort isn't an inline in this header. I thought it wasn't safe to call
> >> > arbitary kernel functions from the stub?
> >> >
> >>
> >> We call string functions, cache maintenance functions, libfdt
> >> functions etc etc so it seems not everyone got the memo :-)
> >>
> >> I agree that treating vmlinux both as a static library and as a
> >> payload from the stub's pov is a bit sloppy, and I do remember
> >> discussing this, but for the life of me, I can't remember the exact
> >> issue, other than the use of adrp/add and adrp/ldr pairs, which we
> >> fixed by setting the PE/COFF section alignment to 4 KB.
> >
> > I only had a vague recollection that there was a problem, which I
> > thought was more to do with potential use of absolute kernel virtual
> > addresses, which would be incorrect in the context of an EFI
> > application.
> >
> 
> That was it, of course. Unlike the x86 stub, which is built with -fPIC
> (as is the ARM decompressor, btw), the arm64 kernel is position
> dependent. Fortunately, the small code model is mostly position
> independent by default, but it would be good if we could spot any
> problems at build time.
> 
> > Digging a bit, the stub code itself is safe due to commit
> > f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that
> 
> libstub is linked into vmlinux so that does not make a different at all

Ok. I assumed that inter-stub references would still be relative, but I
have no idea what the linker would do.

> > We do seem to be ok so far, however. Maybe we just need to keep an eye
> > out.
> >
> 
> I'd much rather restrict the code that goes into the stub somehow than
> deal with any absolute references. Perhaps we could reuse some of the
> section mismatch code in some way to tag certain code as stub-safe and
> do a verification pass on the binary.

It would be great if we could, though I'm not really sure how that would
work.

Imagine stub_foo calls fdt_blah, which calls kern_baz by absolute
address. Normally the latter call is fine, but not when the original
call comes from the stub. I don't think the section mismatch code has
any way to contextualize calls like that, and I don't know what criteria
we could use for annotating code.

Let's not have that delay this patch, however. That's a bigger can of
worms.

Thanks,
Mark.

[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

> OK so what we could do is the following:
> 
> ------------8<--------------
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..39fa2a70a7f1 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -233,6 +233,7 @@ void __init efi_init(void)
>  static bool __init efi_virtmap_init(void)
>  {
>         efi_memory_desc_t *md;
> +       u64 prev_end = 0;
> 
>         for_each_efi_memory_desc(&memmap, md) {
>                 u64 paddr, npages, size;
> @@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void)
>                  * executable, everything else can be mapped with the XN bits
>                  * set.
>                  */
> -               if (!is_normal_ram(md))
> +               if (!is_normal_ram(md)) {
>                         prot = __pgprot(PROT_DEVICE_nGnRE);
> -               else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +               } else if (md->type == EFI_RUNTIME_SERVICES_CODE) {
>                         prot = PAGE_KERNEL_EXEC;
> -               else
> +               } else {
> +                       /*
> +                        * If we are running with >4 KB pages and the current
> +                        * region shares a page frame with the preceding one,
> +                        * we should not map the leading page again since doing
> +                        * so may take its executable permissions away.
> +                        */
> +                       if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) {
> +                               paddr += PAGE_SIZE;
> +                               size -= PAGE_SIZE;
> +                               if (!size)
> +                                       continue;
> +                       }
>                         prot = PAGE_KERNEL;
> -
> +               }
> +               prev_end = paddr + size;
>                 create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
>         }
>         return true;
> ------------8<--------------
> 
> This will ensure that only the pages that are shared between 2 or more
> regions may have their permissions upgraded, but only if any of these
> regions requires it.
> 
> I prefer the much simpler previous version, though, and I think it is
> more suitable for -stable. I can always follow up with an improvement
> like this for v4.3-late.

Ok. Let's go with your previous version for now.

Could you put something in the commit message about this limitation, so
that we don't forget?

> >> >>               else
> >> >>                       prot = PAGE_KERNEL;
> >> >> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> >> >> index e29560e6b40b..cb4e9c4de952 100644
> >> >> --- a/drivers/firmware/efi/libstub/arm-stub.c
> >> >> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> >> >> @@ -13,6 +13,7 @@
> >> >>   */
> >> >>
> >> >>  #include <linux/efi.h>
> >> >> +#include <linux/sort.h>
> >> >
> >> > Sort isn't an inline in this header. I thought it wasn't safe to call
> >> > arbitary kernel functions from the stub?
> >> >
> >>
> >> We call string functions, cache maintenance functions, libfdt
> >> functions etc etc so it seems not everyone got the memo :-)
> >>
> >> I agree that treating vmlinux both as a static library and as a
> >> payload from the stub's pov is a bit sloppy, and I do remember
> >> discussing this, but for the life of me, I can't remember the exact
> >> issue, other than the use of adrp/add and adrp/ldr pairs, which we
> >> fixed by setting the PE/COFF section alignment to 4 KB.
> >
> > I only had a vague recollection that there was a problem, which I
> > thought was more to do with potential use of absolute kernel virtual
> > addresses, which would be incorrect in the context of an EFI
> > application.
> >
> 
> That was it, of course. Unlike the x86 stub, which is built with -fPIC
> (as is the ARM decompressor, btw), the arm64 kernel is position
> dependent. Fortunately, the small code model is mostly position
> independent by default, but it would be good if we could spot any
> problems at build time.
> 
> > Digging a bit, the stub code itself is safe due to commit
> > f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but that
> 
> libstub is linked into vmlinux so that does not make a different at all

Ok. I assumed that inter-stub references would still be relative, but I
have no idea what the linker would do.

> > We do seem to be ok so far, however. Maybe we just need to keep an eye
> > out.
> >
> 
> I'd much rather restrict the code that goes into the stub somehow than
> deal with any absolute references. Perhaps we could reuse some of the
> section mismatch code in some way to tag certain code as stub-safe and
> do a verification pass on the binary.

It would be great if we could, though I'm not really sure how that would
work.

Imagine stub_foo calls fdt_blah, which calls kern_baz by absolute
address. Normally the latter call is fine, but not when the original
call comes from the stub. I don't think the section mismatch code has
any way to contextualize calls like that, and I don't know what criteria
we could use for annotating code.

Let's not have that delay this patch, however. That's a bigger can of
worms.

Thanks,
Mark.

[PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

The new Properties Table feature introduced in UEFIv2.5 may split
memory regions that cover PE/COFF memory images into separate code
and data regions. Since these regions only differ in the type (runtime
code vs runtime data) and the permission bits, but not in the memory
type attributes (UC/WC/WT/WB), the spec does not require them to be
aligned to 64 KB.

Since the relative offset of PE/COFF .text and .data segments cannot
be changed on the fly, this means that we can no longer pad out those
regions to be mappable using 64 KB pages.
Unfortunately, there is no annotation in the UEFI memory map that
identifies data regions that were split off from a code region, so we
must apply this logic to all adjacent runtime regions whose attributes
only differ in the permission bits.

So instead of rounding each memory region to 64 KB alignment at both
ends, only round down regions that are not directly preceded by another
runtime region with the same type attributes. Since the UEFI spec does
not mandate that the memory map be sorted, this means we also need to
sort it first.

Note that this change will result in all EFI_MEMORY_RUNTIME regions whose
start addresses are not aligned to the OS page size to be mapped with
executable permissions (i.e., on kernels compiled with 64 KB pages).
However, since these mappings are only active during the time that UEFI
Runtime Services are being invoked, the window for abuse is rather small.

Tested-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Changes since v2:
- break down complex if() condition into helper functions to test for adjacency
  and compatible memory types
- update commit log to emphasize that this patch may result in data regions to
  be mapped with exec permissions on 64k pages kernels.
- no functional changes

Changes since v1:
- Ensure that we don't inadvertently set the XN bit on the preceding region at
  mapping time if we the OS is running with >4 KB pages.

 arch/arm64/kernel/efi.c                 |  3 +-
 drivers/firmware/efi/libstub/arm-stub.c | 88 ++++++++++++++++----
 2 files changed, 75 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6eaedd02..13671a9cf016 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
 		 */
 		if (!is_normal_ram(md))
 			prot = __pgprot(PROT_DEVICE_nGnRE);
-		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
+			 !PAGE_ALIGNED(md->phys_addr))
 			prot = PAGE_KERNEL_EXEC;
 		else
 			prot = PAGE_KERNEL;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index e29560e6b40b..950c87f5d279 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/efi.h>
+#include <linux/sort.h>
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -305,6 +306,44 @@ fail:
  */
 #define EFI_RT_VIRTUAL_BASE	0x40000000
 
+static int cmp_mem_desc(const void *l, const void *r)
+{
+	const efi_memory_desc_t *left = l, *right = r;
+
+	return (left->phys_addr > right->phys_addr) ? 1 : -1;
+}
+
+/*
+ * Returns whether region @left ends exactly where region @right starts,
+ * or false if either argument is NULL.
+ */
+static bool regions_are_adjacent(efi_memory_desc_t *left,
+				 efi_memory_desc_t *right)
+{
+	u64 left_end;
+
+	if (left == NULL || right == NULL)
+		return false;
+
+	left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE;
+
+	return left_end == right->phys_addr;
+}
+
+/*
+ * Returns whether region @left and region @right have compatible memory type
+ * mapping attributes, and are both EFI_MEMORY_RUNTIME regions.
+ */
+static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left,
+						      efi_memory_desc_t *right)
+{
+	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
+					 EFI_MEMORY_WC | EFI_MEMORY_UC |
+					 EFI_MEMORY_RUNTIME;
+
+	return ((left->attribute ^ right->attribute) & mem_type_mask) == 0;
+}
+
 /*
  * efi_get_virtmap() - create a virtual mapping for the EFI memory map
  *
@@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
 		     int *count)
 {
 	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
-	efi_memory_desc_t *out = runtime_map;
+	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
 	int l;
 
-	for (l = 0; l < map_size; l += desc_size) {
-		efi_memory_desc_t *in = (void *)memory_map + l;
+	/*
+	 * To work around potential issues with the Properties Table feature
+	 * introduced in UEFI 2.5, which may split PE/COFF executable images
+	 * in memory into several RuntimeServicesCode and RuntimeServicesData
+	 * regions, we need to preserve the relative offsets between adjacent
+	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
+	 * The easiest way to find adjacent regions is to sort the memory map
+	 * before traversing it.
+	 */
+	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
+
+	for (l = 0; l < map_size; l += desc_size, prev = in) {
 		u64 paddr, size;
 
+		in = (void *)memory_map + l;
 		if (!(in->attribute & EFI_MEMORY_RUNTIME))
 			continue;
 
+		paddr = in->phys_addr;
+		size = in->num_pages * EFI_PAGE_SIZE;
+
 		/*
 		 * Make the mapping compatible with 64k pages: this allows
 		 * a 4k page size kernel to kexec a 64k page size kernel and
 		 * vice versa.
 		 */
-		paddr = round_down(in->phys_addr, SZ_64K);
-		size = round_up(in->num_pages * EFI_PAGE_SIZE +
-				in->phys_addr - paddr, SZ_64K);
-
-		/*
-		 * Avoid wasting memory on PTEs by choosing a virtual base that
-		 * is compatible with section mappings if this region has the
-		 * appropriate size and physical alignment. (Sections are 2 MB
-		 * on 4k granule kernels)
-		 */
-		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
-			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+		if (!regions_are_adjacent(prev, in) ||
+		    !regions_have_compatible_memory_type_attrs(prev, in)) {
+
+			paddr = round_down(in->phys_addr, SZ_64K);
+			size += in->phys_addr - paddr;
+
+			/*
+			 * Avoid wasting memory on PTEs by choosing a virtual
+			 * base that is compatible with section mappings if this
+			 * region has the appropriate size and physical
+			 * alignment. (Sections are 2 MB on 4k granule kernels)
+			 */
+			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+				efi_virt_base = round_up(efi_virt_base, SZ_2M);
+			else
+				efi_virt_base = round_up(efi_virt_base, SZ_64K);
+		}
 
 		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
 		efi_virt_base += size;
-- 
1.9.1

[PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

The new Properties Table feature introduced in UEFIv2.5 may split
memory regions that cover PE/COFF memory images into separate code
and data regions. Since these regions only differ in the type (runtime
code vs runtime data) and the permission bits, but not in the memory
type attributes (UC/WC/WT/WB), the spec does not require them to be
aligned to 64 KB.

Since the relative offset of PE/COFF .text and .data segments cannot
be changed on the fly, this means that we can no longer pad out those
regions to be mappable using 64 KB pages.
Unfortunately, there is no annotation in the UEFI memory map that
identifies data regions that were split off from a code region, so we
must apply this logic to all adjacent runtime regions whose attributes
only differ in the permission bits.

So instead of rounding each memory region to 64 KB alignment at both
ends, only round down regions that are not directly preceded by another
runtime region with the same type attributes. Since the UEFI spec does
not mandate that the memory map be sorted, this means we also need to
sort it first.

Note that this change will result in all EFI_MEMORY_RUNTIME regions whose
start addresses are not aligned to the OS page size to be mapped with
executable permissions (i.e., on kernels compiled with 64 KB pages).
However, since these mappings are only active during the time that UEFI
Runtime Services are being invoked, the window for abuse is rather small.

Tested-by: Mark Salter <msalter@redhat.com>
Reviewed-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Changes since v2:
- break down complex if() condition into helper functions to test for adjacency
  and compatible memory types
- update commit log to emphasize that this patch may result in data regions to
  be mapped with exec permissions on 64k pages kernels.
- no functional changes

Changes since v1:
- Ensure that we don't inadvertently set the XN bit on the preceding region at
  mapping time if we the OS is running with >4 KB pages.

 arch/arm64/kernel/efi.c                 |  3 +-
 drivers/firmware/efi/libstub/arm-stub.c | 88 ++++++++++++++++----
 2 files changed, 75 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e8ca6eaedd02..13671a9cf016 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
 		 */
 		if (!is_normal_ram(md))
 			prot = __pgprot(PROT_DEVICE_nGnRE);
-		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
+			 !PAGE_ALIGNED(md->phys_addr))
 			prot = PAGE_KERNEL_EXEC;
 		else
 			prot = PAGE_KERNEL;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index e29560e6b40b..950c87f5d279 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/efi.h>
+#include <linux/sort.h>
 #include <asm/efi.h>
 
 #include "efistub.h"
@@ -305,6 +306,44 @@ fail:
  */
 #define EFI_RT_VIRTUAL_BASE	0x40000000
 
+static int cmp_mem_desc(const void *l, const void *r)
+{
+	const efi_memory_desc_t *left = l, *right = r;
+
+	return (left->phys_addr > right->phys_addr) ? 1 : -1;
+}
+
+/*
+ * Returns whether region @left ends exactly where region @right starts,
+ * or false if either argument is NULL.
+ */
+static bool regions_are_adjacent(efi_memory_desc_t *left,
+				 efi_memory_desc_t *right)
+{
+	u64 left_end;
+
+	if (left == NULL || right == NULL)
+		return false;
+
+	left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE;
+
+	return left_end == right->phys_addr;
+}
+
+/*
+ * Returns whether region @left and region @right have compatible memory type
+ * mapping attributes, and are both EFI_MEMORY_RUNTIME regions.
+ */
+static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left,
+						      efi_memory_desc_t *right)
+{
+	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
+					 EFI_MEMORY_WC | EFI_MEMORY_UC |
+					 EFI_MEMORY_RUNTIME;
+
+	return ((left->attribute ^ right->attribute) & mem_type_mask) == 0;
+}
+
 /*
  * efi_get_virtmap() - create a virtual mapping for the EFI memory map
  *
@@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
 		     int *count)
 {
 	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
-	efi_memory_desc_t *out = runtime_map;
+	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
 	int l;
 
-	for (l = 0; l < map_size; l += desc_size) {
-		efi_memory_desc_t *in = (void *)memory_map + l;
+	/*
+	 * To work around potential issues with the Properties Table feature
+	 * introduced in UEFI 2.5, which may split PE/COFF executable images
+	 * in memory into several RuntimeServicesCode and RuntimeServicesData
+	 * regions, we need to preserve the relative offsets between adjacent
+	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
+	 * The easiest way to find adjacent regions is to sort the memory map
+	 * before traversing it.
+	 */
+	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
+
+	for (l = 0; l < map_size; l += desc_size, prev = in) {
 		u64 paddr, size;
 
+		in = (void *)memory_map + l;
 		if (!(in->attribute & EFI_MEMORY_RUNTIME))
 			continue;
 
+		paddr = in->phys_addr;
+		size = in->num_pages * EFI_PAGE_SIZE;
+
 		/*
 		 * Make the mapping compatible with 64k pages: this allows
 		 * a 4k page size kernel to kexec a 64k page size kernel and
 		 * vice versa.
 		 */
-		paddr = round_down(in->phys_addr, SZ_64K);
-		size = round_up(in->num_pages * EFI_PAGE_SIZE +
-				in->phys_addr - paddr, SZ_64K);
-
-		/*
-		 * Avoid wasting memory on PTEs by choosing a virtual base that
-		 * is compatible with section mappings if this region has the
-		 * appropriate size and physical alignment. (Sections are 2 MB
-		 * on 4k granule kernels)
-		 */
-		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
-			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+		if (!regions_are_adjacent(prev, in) ||
+		    !regions_have_compatible_memory_type_attrs(prev, in)) {
+
+			paddr = round_down(in->phys_addr, SZ_64K);
+			size += in->phys_addr - paddr;
+
+			/*
+			 * Avoid wasting memory on PTEs by choosing a virtual
+			 * base that is compatible with section mappings if this
+			 * region has the appropriate size and physical
+			 * alignment. (Sections are 2 MB on 4k granule kernels)
+			 */
+			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+				efi_virt_base = round_up(efi_virt_base, SZ_2M);
+			else
+				efi_virt_base = round_up(efi_virt_base, SZ_64K);
+		}
 
 		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
 		efi_virt_base += size;
-- 
1.9.1

Re: [PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

On Thu, Sep 10, 2015 at 04:41:39PM +0100, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
> 
> Since the relative offset of PE/COFF .text and .data segments cannot
> be changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Note that this change will result in all EFI_MEMORY_RUNTIME regions whose
> start addresses are not aligned to the OS page size to be mapped with
> executable permissions (i.e., on kernels compiled with 64 KB pages).
> However, since these mappings are only active during the time that UEFI
> Runtime Services are being invoked, the window for abuse is rather small.
> 
> Tested-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

I've given this a spin on Juno and Seattle, with both 4K and 64K pages.
Both are EFI 2.4, so that only tells us we haven't regressed things.

The code itself looks good to me. Feel free to add:

Reviewed-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Tested-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> [UEFI 2.4 only]

Thanks,
Mark.

> ---
> Changes since v2:
> - break down complex if() condition into helper functions to test for adjacency
>   and compatible memory types
> - update commit log to emphasize that this patch may result in data regions to
>   be mapped with exec permissions on 64k pages kernels.
> - no functional changes
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
> 
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 88 ++++++++++++++++----
>  2 files changed, 75 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..13671a9cf016 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>  		 */
>  		if (!is_normal_ram(md))
>  			prot = __pgprot(PROT_DEVICE_nGnRE);
> -		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> +			 !PAGE_ALIGNED(md->phys_addr))
>  			prot = PAGE_KERNEL_EXEC;
>  		else
>  			prot = PAGE_KERNEL;
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e29560e6b40b..950c87f5d279 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/efi.h>
> +#include <linux/sort.h>
>  #include <asm/efi.h>
>  
>  #include "efistub.h"
> @@ -305,6 +306,44 @@ fail:
>   */
>  #define EFI_RT_VIRTUAL_BASE	0x40000000
>  
> +static int cmp_mem_desc(const void *l, const void *r)
> +{
> +	const efi_memory_desc_t *left = l, *right = r;
> +
> +	return (left->phys_addr > right->phys_addr) ? 1 : -1;
> +}
> +
> +/*
> + * Returns whether region @left ends exactly where region @right starts,
> + * or false if either argument is NULL.
> + */
> +static bool regions_are_adjacent(efi_memory_desc_t *left,
> +				 efi_memory_desc_t *right)
> +{
> +	u64 left_end;
> +
> +	if (left == NULL || right == NULL)
> +		return false;
> +
> +	left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE;
> +
> +	return left_end == right->phys_addr;
> +}
> +
> +/*
> + * Returns whether region @left and region @right have compatible memory type
> + * mapping attributes, and are both EFI_MEMORY_RUNTIME regions.
> + */
> +static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left,
> +						      efi_memory_desc_t *right)
> +{
> +	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
> +					 EFI_MEMORY_WC | EFI_MEMORY_UC |
> +					 EFI_MEMORY_RUNTIME;
> +
> +	return ((left->attribute ^ right->attribute) & mem_type_mask) == 0;
> +}
> +
>  /*
>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>   *
> @@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		     int *count)
>  {
>  	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
> -	efi_memory_desc_t *out = runtime_map;
> +	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>  	int l;
>  
> -	for (l = 0; l < map_size; l += desc_size) {
> -		efi_memory_desc_t *in = (void *)memory_map + l;
> +	/*
> +	 * To work around potential issues with the Properties Table feature
> +	 * introduced in UEFI 2.5, which may split PE/COFF executable images
> +	 * in memory into several RuntimeServicesCode and RuntimeServicesData
> +	 * regions, we need to preserve the relative offsets between adjacent
> +	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
> +	 * The easiest way to find adjacent regions is to sort the memory map
> +	 * before traversing it.
> +	 */
> +	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> +
> +	for (l = 0; l < map_size; l += desc_size, prev = in) {
>  		u64 paddr, size;
>  
> +		in = (void *)memory_map + l;
>  		if (!(in->attribute & EFI_MEMORY_RUNTIME))
>  			continue;
>  
> +		paddr = in->phys_addr;
> +		size = in->num_pages * EFI_PAGE_SIZE;
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
>  		 * vice versa.
>  		 */
> -		paddr = round_down(in->phys_addr, SZ_64K);
> -		size = round_up(in->num_pages * EFI_PAGE_SIZE +
> -				in->phys_addr - paddr, SZ_64K);
> -
> -		/*
> -		 * Avoid wasting memory on PTEs by choosing a virtual base that
> -		 * is compatible with section mappings if this region has the
> -		 * appropriate size and physical alignment. (Sections are 2 MB
> -		 * on 4k granule kernels)
> -		 */
> -		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> -			efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +		if (!regions_are_adjacent(prev, in) ||
> +		    !regions_have_compatible_memory_type_attrs(prev, in)) {
> +
> +			paddr = round_down(in->phys_addr, SZ_64K);
> +			size += in->phys_addr - paddr;
> +
> +			/*
> +			 * Avoid wasting memory on PTEs by choosing a virtual
> +			 * base that is compatible with section mappings if this
> +			 * region has the appropriate size and physical
> +			 * alignment. (Sections are 2 MB on 4k granule kernels)
> +			 */
> +			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> +				efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +			else
> +				efi_virt_base = round_up(efi_virt_base, SZ_64K);
> +		}
>  
>  		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
>  		efi_virt_base += size;
> -- 
> 1.9.1
> 

[PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Mark Rutland]

On Thu, Sep 10, 2015 at 04:41:39PM +0100, Ard Biesheuvel wrote:
> The new Properties Table feature introduced in UEFIv2.5 may split
> memory regions that cover PE/COFF memory images into separate code
> and data regions. Since these regions only differ in the type (runtime
> code vs runtime data) and the permission bits, but not in the memory
> type attributes (UC/WC/WT/WB), the spec does not require them to be
> aligned to 64 KB.
> 
> Since the relative offset of PE/COFF .text and .data segments cannot
> be changed on the fly, this means that we can no longer pad out those
> regions to be mappable using 64 KB pages.
> Unfortunately, there is no annotation in the UEFI memory map that
> identifies data regions that were split off from a code region, so we
> must apply this logic to all adjacent runtime regions whose attributes
> only differ in the permission bits.
> 
> So instead of rounding each memory region to 64 KB alignment at both
> ends, only round down regions that are not directly preceded by another
> runtime region with the same type attributes. Since the UEFI spec does
> not mandate that the memory map be sorted, this means we also need to
> sort it first.
> 
> Note that this change will result in all EFI_MEMORY_RUNTIME regions whose
> start addresses are not aligned to the OS page size to be mapped with
> executable permissions (i.e., on kernels compiled with 64 KB pages).
> However, since these mappings are only active during the time that UEFI
> Runtime Services are being invoked, the window for abuse is rather small.
> 
> Tested-by: Mark Salter <msalter@redhat.com>
> Reviewed-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

I've given this a spin on Juno and Seattle, with both 4K and 64K pages.
Both are EFI 2.4, so that only tells us we haven't regressed things.

The code itself looks good to me. Feel free to add:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com> [UEFI 2.4 only]

Thanks,
Mark.

> ---
> Changes since v2:
> - break down complex if() condition into helper functions to test for adjacency
>   and compatible memory types
> - update commit log to emphasize that this patch may result in data regions to
>   be mapped with exec permissions on 64k pages kernels.
> - no functional changes
> 
> Changes since v1:
> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>   mapping time if we the OS is running with >4 KB pages.
> 
>  arch/arm64/kernel/efi.c                 |  3 +-
>  drivers/firmware/efi/libstub/arm-stub.c | 88 ++++++++++++++++----
>  2 files changed, 75 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e8ca6eaedd02..13671a9cf016 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>  		 */
>  		if (!is_normal_ram(md))
>  			prot = __pgprot(PROT_DEVICE_nGnRE);
> -		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
> +		else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> +			 !PAGE_ALIGNED(md->phys_addr))
>  			prot = PAGE_KERNEL_EXEC;
>  		else
>  			prot = PAGE_KERNEL;
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index e29560e6b40b..950c87f5d279 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/efi.h>
> +#include <linux/sort.h>
>  #include <asm/efi.h>
>  
>  #include "efistub.h"
> @@ -305,6 +306,44 @@ fail:
>   */
>  #define EFI_RT_VIRTUAL_BASE	0x40000000
>  
> +static int cmp_mem_desc(const void *l, const void *r)
> +{
> +	const efi_memory_desc_t *left = l, *right = r;
> +
> +	return (left->phys_addr > right->phys_addr) ? 1 : -1;
> +}
> +
> +/*
> + * Returns whether region @left ends exactly where region @right starts,
> + * or false if either argument is NULL.
> + */
> +static bool regions_are_adjacent(efi_memory_desc_t *left,
> +				 efi_memory_desc_t *right)
> +{
> +	u64 left_end;
> +
> +	if (left == NULL || right == NULL)
> +		return false;
> +
> +	left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE;
> +
> +	return left_end == right->phys_addr;
> +}
> +
> +/*
> + * Returns whether region @left and region @right have compatible memory type
> + * mapping attributes, and are both EFI_MEMORY_RUNTIME regions.
> + */
> +static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left,
> +						      efi_memory_desc_t *right)
> +{
> +	static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
> +					 EFI_MEMORY_WC | EFI_MEMORY_UC |
> +					 EFI_MEMORY_RUNTIME;
> +
> +	return ((left->attribute ^ right->attribute) & mem_type_mask) == 0;
> +}
> +
>  /*
>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>   *
> @@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>  		     int *count)
>  {
>  	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
> -	efi_memory_desc_t *out = runtime_map;
> +	efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>  	int l;
>  
> -	for (l = 0; l < map_size; l += desc_size) {
> -		efi_memory_desc_t *in = (void *)memory_map + l;
> +	/*
> +	 * To work around potential issues with the Properties Table feature
> +	 * introduced in UEFI 2.5, which may split PE/COFF executable images
> +	 * in memory into several RuntimeServicesCode and RuntimeServicesData
> +	 * regions, we need to preserve the relative offsets between adjacent
> +	 * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
> +	 * The easiest way to find adjacent regions is to sort the memory map
> +	 * before traversing it.
> +	 */
> +	sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> +
> +	for (l = 0; l < map_size; l += desc_size, prev = in) {
>  		u64 paddr, size;
>  
> +		in = (void *)memory_map + l;
>  		if (!(in->attribute & EFI_MEMORY_RUNTIME))
>  			continue;
>  
> +		paddr = in->phys_addr;
> +		size = in->num_pages * EFI_PAGE_SIZE;
> +
>  		/*
>  		 * Make the mapping compatible with 64k pages: this allows
>  		 * a 4k page size kernel to kexec a 64k page size kernel and
>  		 * vice versa.
>  		 */
> -		paddr = round_down(in->phys_addr, SZ_64K);
> -		size = round_up(in->num_pages * EFI_PAGE_SIZE +
> -				in->phys_addr - paddr, SZ_64K);
> -
> -		/*
> -		 * Avoid wasting memory on PTEs by choosing a virtual base that
> -		 * is compatible with section mappings if this region has the
> -		 * appropriate size and physical alignment. (Sections are 2 MB
> -		 * on 4k granule kernels)
> -		 */
> -		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> -			efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +		if (!regions_are_adjacent(prev, in) ||
> +		    !regions_have_compatible_memory_type_attrs(prev, in)) {
> +
> +			paddr = round_down(in->phys_addr, SZ_64K);
> +			size += in->phys_addr - paddr;
> +
> +			/*
> +			 * Avoid wasting memory on PTEs by choosing a virtual
> +			 * base that is compatible with section mappings if this
> +			 * region has the appropriate size and physical
> +			 * alignment. (Sections are 2 MB on 4k granule kernels)
> +			 */
> +			if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
> +				efi_virt_base = round_up(efi_virt_base, SZ_2M);
> +			else
> +				efi_virt_base = round_up(efi_virt_base, SZ_64K);
> +		}
>  
>  		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
>  		efi_virt_base += size;
> -- 
> 1.9.1
> 

Re: [PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

On 10 September 2015 at 18:08, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Thu, Sep 10, 2015 at 04:41:39PM +0100, Ard Biesheuvel wrote:
>> The new Properties Table feature introduced in UEFIv2.5 may split
>> memory regions that cover PE/COFF memory images into separate code
>> and data regions. Since these regions only differ in the type (runtime
>> code vs runtime data) and the permission bits, but not in the memory
>> type attributes (UC/WC/WT/WB), the spec does not require them to be
>> aligned to 64 KB.
>>
>> Since the relative offset of PE/COFF .text and .data segments cannot
>> be changed on the fly, this means that we can no longer pad out those
>> regions to be mappable using 64 KB pages.
>> Unfortunately, there is no annotation in the UEFI memory map that
>> identifies data regions that were split off from a code region, so we
>> must apply this logic to all adjacent runtime regions whose attributes
>> only differ in the permission bits.
>>
>> So instead of rounding each memory region to 64 KB alignment at both
>> ends, only round down regions that are not directly preceded by another
>> runtime region with the same type attributes. Since the UEFI spec does
>> not mandate that the memory map be sorted, this means we also need to
>> sort it first.
>>
>> Note that this change will result in all EFI_MEMORY_RUNTIME regions whose
>> start addresses are not aligned to the OS page size to be mapped with
>> executable permissions (i.e., on kernels compiled with 64 KB pages).
>> However, since these mappings are only active during the time that UEFI
>> Runtime Services are being invoked, the window for abuse is rather small.
>>
>> Tested-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> I've given this a spin on Juno and Seattle, with both 4K and 64K pages.
> Both are EFI 2.4, so that only tells us we haven't regressed things.
>
> The code itself looks good to me. Feel free to add:
>
> Reviewed-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Tested-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> [UEFI 2.4 only]
>

Thanks Mark.

@Matt: could you please drop the version you have queued, and take
this one instead?
Also, this patch will not apply to anything before v4.0 so could you
please include that in the cc:stable tag?

Thanks,
Ard.



>> ---
>> Changes since v2:
>> - break down complex if() condition into helper functions to test for adjacency
>>   and compatible memory types
>> - update commit log to emphasize that this patch may result in data regions to
>>   be mapped with exec permissions on 64k pages kernels.
>> - no functional changes
>>
>> Changes since v1:
>> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>>   mapping time if we the OS is running with >4 KB pages.
>>
>>  arch/arm64/kernel/efi.c                 |  3 +-
>>  drivers/firmware/efi/libstub/arm-stub.c | 88 ++++++++++++++++----
>>  2 files changed, 75 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index e8ca6eaedd02..13671a9cf016 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>>                */
>>               if (!is_normal_ram(md))
>>                       prot = __pgprot(PROT_DEVICE_nGnRE);
>> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
>> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> +                      !PAGE_ALIGNED(md->phys_addr))
>>                       prot = PAGE_KERNEL_EXEC;
>>               else
>>                       prot = PAGE_KERNEL;
>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> index e29560e6b40b..950c87f5d279 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/efi.h>
>> +#include <linux/sort.h>
>>  #include <asm/efi.h>
>>
>>  #include "efistub.h"
>> @@ -305,6 +306,44 @@ fail:
>>   */
>>  #define EFI_RT_VIRTUAL_BASE  0x40000000
>>
>> +static int cmp_mem_desc(const void *l, const void *r)
>> +{
>> +     const efi_memory_desc_t *left = l, *right = r;
>> +
>> +     return (left->phys_addr > right->phys_addr) ? 1 : -1;
>> +}
>> +
>> +/*
>> + * Returns whether region @left ends exactly where region @right starts,
>> + * or false if either argument is NULL.
>> + */
>> +static bool regions_are_adjacent(efi_memory_desc_t *left,
>> +                              efi_memory_desc_t *right)
>> +{
>> +     u64 left_end;
>> +
>> +     if (left == NULL || right == NULL)
>> +             return false;
>> +
>> +     left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE;
>> +
>> +     return left_end == right->phys_addr;
>> +}
>> +
>> +/*
>> + * Returns whether region @left and region @right have compatible memory type
>> + * mapping attributes, and are both EFI_MEMORY_RUNTIME regions.
>> + */
>> +static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left,
>> +                                                   efi_memory_desc_t *right)
>> +{
>> +     static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
>> +                                      EFI_MEMORY_WC | EFI_MEMORY_UC |
>> +                                      EFI_MEMORY_RUNTIME;
>> +
>> +     return ((left->attribute ^ right->attribute) & mem_type_mask) == 0;
>> +}
>> +
>>  /*
>>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>>   *
>> @@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>>                    int *count)
>>  {
>>       u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
>> -     efi_memory_desc_t *out = runtime_map;
>> +     efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>>       int l;
>>
>> -     for (l = 0; l < map_size; l += desc_size) {
>> -             efi_memory_desc_t *in = (void *)memory_map + l;
>> +     /*
>> +      * To work around potential issues with the Properties Table feature
>> +      * introduced in UEFI 2.5, which may split PE/COFF executable images
>> +      * in memory into several RuntimeServicesCode and RuntimeServicesData
>> +      * regions, we need to preserve the relative offsets between adjacent
>> +      * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
>> +      * The easiest way to find adjacent regions is to sort the memory map
>> +      * before traversing it.
>> +      */
>> +     sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
>> +
>> +     for (l = 0; l < map_size; l += desc_size, prev = in) {
>>               u64 paddr, size;
>>
>> +             in = (void *)memory_map + l;
>>               if (!(in->attribute & EFI_MEMORY_RUNTIME))
>>                       continue;
>>
>> +             paddr = in->phys_addr;
>> +             size = in->num_pages * EFI_PAGE_SIZE;
>> +
>>               /*
>>                * Make the mapping compatible with 64k pages: this allows
>>                * a 4k page size kernel to kexec a 64k page size kernel and
>>                * vice versa.
>>                */
>> -             paddr = round_down(in->phys_addr, SZ_64K);
>> -             size = round_up(in->num_pages * EFI_PAGE_SIZE +
>> -                             in->phys_addr - paddr, SZ_64K);
>> -
>> -             /*
>> -              * Avoid wasting memory on PTEs by choosing a virtual base that
>> -              * is compatible with section mappings if this region has the
>> -              * appropriate size and physical alignment. (Sections are 2 MB
>> -              * on 4k granule kernels)
>> -              */
>> -             if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
>> -                     efi_virt_base = round_up(efi_virt_base, SZ_2M);
>> +             if (!regions_are_adjacent(prev, in) ||
>> +                 !regions_have_compatible_memory_type_attrs(prev, in)) {
>> +
>> +                     paddr = round_down(in->phys_addr, SZ_64K);
>> +                     size += in->phys_addr - paddr;
>> +
>> +                     /*
>> +                      * Avoid wasting memory on PTEs by choosing a virtual
>> +                      * base that is compatible with section mappings if this
>> +                      * region has the appropriate size and physical
>> +                      * alignment. (Sections are 2 MB on 4k granule kernels)
>> +                      */
>> +                     if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
>> +                             efi_virt_base = round_up(efi_virt_base, SZ_2M);
>> +                     else
>> +                             efi_virt_base = round_up(efi_virt_base, SZ_64K);
>> +             }
>>
>>               in->virt_addr = efi_virt_base + in->phys_addr - paddr;
>>               efi_virt_base += size;
>> --
>> 1.9.1
>>

[PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Ard Biesheuvel]

On 10 September 2015 at 18:08, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Sep 10, 2015 at 04:41:39PM +0100, Ard Biesheuvel wrote:
>> The new Properties Table feature introduced in UEFIv2.5 may split
>> memory regions that cover PE/COFF memory images into separate code
>> and data regions. Since these regions only differ in the type (runtime
>> code vs runtime data) and the permission bits, but not in the memory
>> type attributes (UC/WC/WT/WB), the spec does not require them to be
>> aligned to 64 KB.
>>
>> Since the relative offset of PE/COFF .text and .data segments cannot
>> be changed on the fly, this means that we can no longer pad out those
>> regions to be mappable using 64 KB pages.
>> Unfortunately, there is no annotation in the UEFI memory map that
>> identifies data regions that were split off from a code region, so we
>> must apply this logic to all adjacent runtime regions whose attributes
>> only differ in the permission bits.
>>
>> So instead of rounding each memory region to 64 KB alignment at both
>> ends, only round down regions that are not directly preceded by another
>> runtime region with the same type attributes. Since the UEFI spec does
>> not mandate that the memory map be sorted, this means we also need to
>> sort it first.
>>
>> Note that this change will result in all EFI_MEMORY_RUNTIME regions whose
>> start addresses are not aligned to the OS page size to be mapped with
>> executable permissions (i.e., on kernels compiled with 64 KB pages).
>> However, since these mappings are only active during the time that UEFI
>> Runtime Services are being invoked, the window for abuse is rather small.
>>
>> Tested-by: Mark Salter <msalter@redhat.com>
>> Reviewed-by: Mark Salter <msalter@redhat.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> I've given this a spin on Juno and Seattle, with both 4K and 64K pages.
> Both are EFI 2.4, so that only tells us we haven't regressed things.
>
> The code itself looks good to me. Feel free to add:
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com> [UEFI 2.4 only]
>

Thanks Mark.

@Matt: could you please drop the version you have queued, and take
this one instead?
Also, this patch will not apply to anything before v4.0 so could you
please include that in the cc:stable tag?

Thanks,
Ard.



>> ---
>> Changes since v2:
>> - break down complex if() condition into helper functions to test for adjacency
>>   and compatible memory types
>> - update commit log to emphasize that this patch may result in data regions to
>>   be mapped with exec permissions on 64k pages kernels.
>> - no functional changes
>>
>> Changes since v1:
>> - Ensure that we don't inadvertently set the XN bit on the preceding region at
>>   mapping time if we the OS is running with >4 KB pages.
>>
>>  arch/arm64/kernel/efi.c                 |  3 +-
>>  drivers/firmware/efi/libstub/arm-stub.c | 88 ++++++++++++++++----
>>  2 files changed, 75 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index e8ca6eaedd02..13671a9cf016 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -258,7 +258,8 @@ static bool __init efi_virtmap_init(void)
>>                */
>>               if (!is_normal_ram(md))
>>                       prot = __pgprot(PROT_DEVICE_nGnRE);
>> -             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
>> +             else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> +                      !PAGE_ALIGNED(md->phys_addr))
>>                       prot = PAGE_KERNEL_EXEC;
>>               else
>>                       prot = PAGE_KERNEL;
>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> index e29560e6b40b..950c87f5d279 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/efi.h>
>> +#include <linux/sort.h>
>>  #include <asm/efi.h>
>>
>>  #include "efistub.h"
>> @@ -305,6 +306,44 @@ fail:
>>   */
>>  #define EFI_RT_VIRTUAL_BASE  0x40000000
>>
>> +static int cmp_mem_desc(const void *l, const void *r)
>> +{
>> +     const efi_memory_desc_t *left = l, *right = r;
>> +
>> +     return (left->phys_addr > right->phys_addr) ? 1 : -1;
>> +}
>> +
>> +/*
>> + * Returns whether region @left ends exactly where region @right starts,
>> + * or false if either argument is NULL.
>> + */
>> +static bool regions_are_adjacent(efi_memory_desc_t *left,
>> +                              efi_memory_desc_t *right)
>> +{
>> +     u64 left_end;
>> +
>> +     if (left == NULL || right == NULL)
>> +             return false;
>> +
>> +     left_end = left->phys_addr + left->num_pages * EFI_PAGE_SIZE;
>> +
>> +     return left_end == right->phys_addr;
>> +}
>> +
>> +/*
>> + * Returns whether region @left and region @right have compatible memory type
>> + * mapping attributes, and are both EFI_MEMORY_RUNTIME regions.
>> + */
>> +static bool regions_have_compatible_memory_type_attrs(efi_memory_desc_t *left,
>> +                                                   efi_memory_desc_t *right)
>> +{
>> +     static const u64 mem_type_mask = EFI_MEMORY_WB | EFI_MEMORY_WT |
>> +                                      EFI_MEMORY_WC | EFI_MEMORY_UC |
>> +                                      EFI_MEMORY_RUNTIME;
>> +
>> +     return ((left->attribute ^ right->attribute) & mem_type_mask) == 0;
>> +}
>> +
>>  /*
>>   * efi_get_virtmap() - create a virtual mapping for the EFI memory map
>>   *
>> @@ -317,33 +356,52 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
>>                    int *count)
>>  {
>>       u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
>> -     efi_memory_desc_t *out = runtime_map;
>> +     efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
>>       int l;
>>
>> -     for (l = 0; l < map_size; l += desc_size) {
>> -             efi_memory_desc_t *in = (void *)memory_map + l;
>> +     /*
>> +      * To work around potential issues with the Properties Table feature
>> +      * introduced in UEFI 2.5, which may split PE/COFF executable images
>> +      * in memory into several RuntimeServicesCode and RuntimeServicesData
>> +      * regions, we need to preserve the relative offsets between adjacent
>> +      * EFI_MEMORY_RUNTIME regions with the same memory type attributes.
>> +      * The easiest way to find adjacent regions is to sort the memory map
>> +      * before traversing it.
>> +      */
>> +     sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
>> +
>> +     for (l = 0; l < map_size; l += desc_size, prev = in) {
>>               u64 paddr, size;
>>
>> +             in = (void *)memory_map + l;
>>               if (!(in->attribute & EFI_MEMORY_RUNTIME))
>>                       continue;
>>
>> +             paddr = in->phys_addr;
>> +             size = in->num_pages * EFI_PAGE_SIZE;
>> +
>>               /*
>>                * Make the mapping compatible with 64k pages: this allows
>>                * a 4k page size kernel to kexec a 64k page size kernel and
>>                * vice versa.
>>                */
>> -             paddr = round_down(in->phys_addr, SZ_64K);
>> -             size = round_up(in->num_pages * EFI_PAGE_SIZE +
>> -                             in->phys_addr - paddr, SZ_64K);
>> -
>> -             /*
>> -              * Avoid wasting memory on PTEs by choosing a virtual base that
>> -              * is compatible with section mappings if this region has the
>> -              * appropriate size and physical alignment. (Sections are 2 MB
>> -              * on 4k granule kernels)
>> -              */
>> -             if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
>> -                     efi_virt_base = round_up(efi_virt_base, SZ_2M);
>> +             if (!regions_are_adjacent(prev, in) ||
>> +                 !regions_have_compatible_memory_type_attrs(prev, in)) {
>> +
>> +                     paddr = round_down(in->phys_addr, SZ_64K);
>> +                     size += in->phys_addr - paddr;
>> +
>> +                     /*
>> +                      * Avoid wasting memory on PTEs by choosing a virtual
>> +                      * base that is compatible with section mappings if this
>> +                      * region has the appropriate size and physical
>> +                      * alignment. (Sections are 2 MB on 4k granule kernels)
>> +                      */
>> +                     if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
>> +                             efi_virt_base = round_up(efi_virt_base, SZ_2M);
>> +                     else
>> +                             efi_virt_base = round_up(efi_virt_base, SZ_64K);
>> +             }
>>
>>               in->virt_addr = efi_virt_base + in->phys_addr - paddr;
>>               efi_virt_base += size;
>> --
>> 1.9.1
>>

Re: [PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Matt Fleming]

On Thu, 10 Sep, at 06:10:57PM, Ard Biesheuvel wrote:
> 
> Thanks Mark.
> 
> @Matt: could you please drop the version you have queued, and take
> this one instead?
> Also, this patch will not apply to anything before v4.0 so could you
> please include that in the cc:stable tag?

Sorry for the delay. I've queued up this version with all tags and
I'll send an urgent pull request this week. Thanks everyone.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v3] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions

[Matt Fleming]

On Thu, 10 Sep, at 06:10:57PM, Ard Biesheuvel wrote:
> 
> Thanks Mark.
> 
> @Matt: could you please drop the version you have queued, and take
> this one instead?
> Also, this patch will not apply to anything before v4.0 so could you
> please include that in the cc:stable tag?

Sorry for the delay. I've queued up this version with all tags and
I'll send an urgent pull request this week. Thanks everyone.

-- 
Matt Fleming, Intel Open Source Technology Center