Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1233091 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-26 00:10 +0200
    Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Ingo Molnar <mingo@kernel.org> - 2015-09-26 08:10 +0200
      Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-26 09:10 +0200
        Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Ingo Molnar <mingo@kernel.org> - 2015-09-27 09:10 +0200
          Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Borislav Petkov <bp@alien8.de> - 2015-09-27 12:20 +0200
            Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Ingo Molnar <mingo@kernel.org> - 2015-09-28 08:30 +0200
            Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Dave Young <dyoung@redhat.com> - 2015-09-29 11:40 +0200
              Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Borislav Petkov <bp@alien8.de> - 2015-09-29 12:30 +0200
            Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-29 16:40 +0200
              Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions "H. Peter Anvin" <hpa@zytor.com> - 2015-09-30 03:00 +0200
                Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Borislav Petkov <bp@alien8.de> - 2015-09-30 10:40 +0200
          Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions "H. Peter Anvin" <hpa@zytor.com> - 2015-09-30 03:10 +0200
            Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions Andy Lutomirski <luto@amacapital.net> - 2015-09-30 03:20 +0200
              Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions "H. Peter Anvin" <hpa@zytor.com> - 2015-09-30 03:20 +0200
              Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-30 06:30 +0200
            Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME  regions Ingo Molnar <mingo@kernel.org> - 2015-10-01 12:50 +0200
    [tip:core/urgent] arm64/efi:   Fix boot crash by not padding between EFI_MEMORY_RUNTIME regions tip-bot for Ard Biesheuvel <tipbot@zytor.com> - 2015-10-01 15:00 +0200

#1233091 — [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Tested-by: Mark Salter <msalter@redhat.com>
Reviewed-by: Mark Salter <msalter@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com> [UEFI 2.4 only]
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: <stable@vger.kernel.org> # v4.0+
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 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;
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1233151 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> 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.

So I think this is fundamentally wrong as well, similarly to the related x86 fix.

I think for compatibility reasons the whole 'EFI runtime image' should be mapped 
in a single go, as closely matching the EFI layouts and offsets as possible. We 
are not talking about gigabytes here, right?

Even if technically they are 'separate sections', the x86 bug shows that they 
aren't. So we should not pretend so on the Linux side either and we should not 
tear them apart (and then work hard to preserve the interdependencies, some 
declared, some hidden!).

If we allocate the EFI runtime as a single virtual memory block then issues like 
rounding between sections does not even come up as a problem: we map the original 
offsets and sizes byte by byte.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233160

On 25 September 2015 at 23:01, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
>
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> 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.
>
> So I think this is fundamentally wrong as well, similarly to the related x86 fix.
>
> I think for compatibility reasons the whole 'EFI runtime image' should be mapped
> in a single go, as closely matching the EFI layouts and offsets as possible. We
> are not talking about gigabytes here, right?
>

As I explained in the other thread, this is really not necessary, and
never has been until the firmware started splitting up PE/COFF images
into several sections each. As long as we keep those PE/COFF images
together, everything will work as before, and the only complication is
that the memory map does not contain any clues about which regions
belong to a single PE/COFF image, so we need to keep all adjacent
EFI_MEMORY_RUNTIME regions adjacent in the VA mapping.

> Even if technically they are 'separate sections', the x86 bug shows that they
> aren't. So we should not pretend so on the Linux side either and we should not
> tear them apart (and then work hard to preserve the interdependencies, some
> declared, some hidden!).
>

This is about relocations and interdependencies at the symbol level,
and such interdependencies only exist internally inside PE/COFF
images.

> If we allocate the EFI runtime as a single virtual memory block then issues like
> rounding between sections does not even come up as a problem: we map the original
> offsets and sizes byte by byte.
>

Well, by that reasoning, we should not call SetVirtualAddressMap() in
the first place, and just use the 1:1 mapping UEFI uses natively. This
is more than feasible on arm64, and I actually fought hard against
using SetVirtualAddressMap() at all, but I was overruled by others. I
think this is also trivially possible on X64, since the 1:1 mapping is
already active alongside the VA mapping.

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233629 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > If we allocate the EFI runtime as a single virtual memory block then issues 
> > like rounding between sections does not even come up as a problem: we map the 
> > original offsets and sizes byte by byte.
> 
> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first 
> place, and just use the 1:1 mapping UEFI uses natively. This is more than 
> feasible on arm64, and I actually fought hard against using 
> SetVirtualAddressMap() at all, but I was overruled by others. I think this is 
> also trivially possible on X64, since the 1:1 mapping is already active 
> alongside the VA mapping.

Could we please re-list all the arguments pro and contra of 1:1 physical mappings, 
in a post that also explains the background so that more people can chime in, not 
just people versed in EFI internals? It's very much possible that a bad decision 
was made.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233638 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote:
> Could we please re-list all the arguments pro and contra of 1:1 physical mappings, 
> in a post that also explains the background so that more people can chime in, not 
> just people versed in EFI internals? It's very much possible that a bad decision 
> was made.

The main reason why we did the additional, top-down mapping was kexec
kernel wanting to use UEFI runtime facilities too and the braindead
design of SetVirtualAddressMap() being callable only once per system
boot. So we had to have stable mappings which are valid in the kexec-ed
kernel too.

But this was long time ago and I most certainly have forgotten all the
details.

And now I'm wondering why didn't we do the 1:1 thing and rebuild the
exact same EFI pagetable in the kexec-ed kernel? Because when we do
an EFI call, we switch to the special pagetable so why didn't we make
the kexec-ed kernel rebuild the 1:1 pagetable which it can use for EFI
calls...

Hmm, again, I've forgotten a lot of details so I'm sure Matt will come
in and say "No, you can't do that because..."

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233875 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

* Borislav Petkov <bp@alien8.de> wrote:

> On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote:
>
> > Could we please re-list all the arguments pro and contra of 1:1 physical 
> > mappings, in a post that also explains the background so that more people can 
> > chime in, not just people versed in EFI internals? It's very much possible 
> > that a bad decision was made.
> 
> The main reason why we did the additional, top-down mapping was kexec kernel 
> wanting to use UEFI runtime facilities too and the braindead design of 
> SetVirtualAddressMap() being callable only once per system boot. So we had to 
> have stable mappings which are valid in the kexec-ed kernel too.
> 
> But this was long time ago and I most certainly have forgotten all the details.
> 
> And now I'm wondering why didn't we do the 1:1 thing and rebuild the exact same 
> EFI pagetable in the kexec-ed kernel? Because when we do an EFI call, we switch 
> to the special pagetable so why didn't we make the kexec-ed kernel rebuild the 
> 1:1 pagetable which it can use for EFI calls...

Yeah.

> Hmm, again, I've forgotten a lot of details so I'm sure Matt will come in and 
> say "No, you can't do that because..."

Would be nice to re-examine all this.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234912 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On 09/27/15 at 12:40pm, Borislav Petkov wrote:
> On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote:
> > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, 
> > in a post that also explains the background so that more people can chime in, not 
> > just people versed in EFI internals? It's very much possible that a bad decision 
> > was made.
> 
> The main reason why we did the additional, top-down mapping was kexec
> kernel wanting to use UEFI runtime facilities too and the braindead
> design of SetVirtualAddressMap() being callable only once per system
> boot. So we had to have stable mappings which are valid in the kexec-ed
> kernel too.
> 
> But this was long time ago and I most certainly have forgotten all the
> details.
> 
> And now I'm wondering why didn't we do the 1:1 thing and rebuild the
> exact same EFI pagetable in the kexec-ed kernel? Because when we do
> an EFI call, we switch to the special pagetable so why didn't we make
> the kexec-ed kernel rebuild the 1:1 pagetable which it can use for EFI
> calls...
> 
> Hmm, again, I've forgotten a lot of details so I'm sure Matt will come
> in and say "No, you can't do that because..."
> 

http://permalink.gmane.org/gmane.linux.kernel.efi/822

And more replies here:
http://comments.gmane.org/gmane.linux.kernel.efi/820

Thanks
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234941 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On Tue, Sep 29, 2015 at 05:31:00PM +0800, Dave Young wrote:
> http://permalink.gmane.org/gmane.linux.kernel.efi/822
> 
> And more replies here:
> http://comments.gmane.org/gmane.linux.kernel.efi/820

Whatever we did think back then, it all is moot as long as some UEFI
vendors do funky crap or the spec has documentation holes.

So we need a reality check and just do exactly what the OS does with
which UEFI writers are testing with. Be it windoze, be it some open
firmware testing tool or whatever. Provided there's even a tool with
which UEFI validation is done and it is used by everybody...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235161 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On Sun, 27 Sep, at 12:40:14PM, Borislav Petkov wrote:
> On Sun, Sep 27, 2015 at 09:06:44AM +0200, Ingo Molnar wrote:
> > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, 
> > in a post that also explains the background so that more people can chime in, not 
> > just people versed in EFI internals? It's very much possible that a bad decision 
> > was made.
> 
> The main reason why we did the additional, top-down mapping was kexec
> kernel wanting to use UEFI runtime facilities too and the braindead
> design of SetVirtualAddressMap() being callable only once per system
> boot. So we had to have stable mappings which are valid in the kexec-ed
> kernel too.
> 
> But this was long time ago and I most certainly have forgotten all the
> details.
 
That's a pretty good summary for x86. I think specifically the reason
we map the EFI memmap entries "backwards" (entry N has higher VA than
entry N+1) is because the code was easier to write that way, but
you'll know better than me ;-)

> And now I'm wondering why didn't we do the 1:1 thing and rebuild the
> exact same EFI pagetable in the kexec-ed kernel? Because when we do
> an EFI call, we switch to the special pagetable so why didn't we make
> the kexec-ed kernel rebuild the 1:1 pagetable which it can use for EFI
> calls...
> 
> Hmm, again, I've forgotten a lot of details so I'm sure Matt will come
> in and say "No, you can't do that because..."

I *think* the only reason was the Apple firmware problem where it
explodes if you pass the 1:1 mappings to SetVirtualAddressMap(). And
obviously people do want to use kexec with Apple machines.

It's probably worth revisiting this whole thing from the x86 side.

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235681 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On 09/29/2015 07:36 AM, Matt Fleming wrote:
>  
> That's a pretty good summary for x86. I think specifically the reason
> we map the EFI memmap entries "backwards" (entry N has higher VA than
> entry N+1) is because the code was easier to write that way, but
> you'll know better than me ;-)
> 

There were two reasons:

1. The code was easier to write.
2. Windows did it that way.

Windows apparently broke and was changed due to this feature, too.

	-hpa


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235899 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On Tue, Sep 29, 2015 at 05:56:07PM -0700, H. Peter Anvin wrote:
> On 09/29/2015 07:36 AM, Matt Fleming wrote:
> >  
> > That's a pretty good summary for x86. I think specifically the reason
> > we map the EFI memmap entries "backwards" (entry N has higher VA than
> > entry N+1) is because the code was easier to write that way, but
> > you'll know better than me ;-)
> > 
> 
> There were two reasons:
> 
> 1. The code was easier to write.
> 2. Windows did it that way.
> 
> Windows apparently broke and was changed due to this feature, too.

So can we do the 1:1 thing again?

I mean, we do create a special pagetable for EFI anyway, we can put in
there whatever we want.

I know, some apple boxes reportedly fail when 1:1 mapping is in use but
we can do the VA mapping as a workaround for them. I.e., have the 1:1
mapping be the default.

Apparently, there's not a single OS or tool which is used by fw writers
to test their brain dumplings. Windoze breakage case-in-point.

Because if there were, we'd simply do what that OS/tool does and be done
with it.

What really makes me climb the walls is when half-cooked, untested fw
hits the wild and we have to support it indefinitely.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235687 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On 09/27/2015 12:06 AM, Ingo Molnar wrote:
> 
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
>>> If we allocate the EFI runtime as a single virtual memory block then issues 
>>> like rounding between sections does not even come up as a problem: we map the 
>>> original offsets and sizes byte by byte.
>>
>> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first 
>> place, and just use the 1:1 mapping UEFI uses natively. This is more than 
>> feasible on arm64, and I actually fought hard against using 
>> SetVirtualAddressMap() at all, but I was overruled by others. I think this is 
>> also trivially possible on X64, since the 1:1 mapping is already active 
>> alongside the VA mapping.
> 
> Could we please re-list all the arguments pro and contra of 1:1 physical mappings, 
> in a post that also explains the background so that more people can chime in, not 
> just people versed in EFI internals? It's very much possible that a bad decision 
> was made.
> 

Pro: by far the sanest way to map the UEFI tables.
Con: doesn't actually work (breaks on several known platforms.)

	-hpa


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235690

On Tue, Sep 29, 2015 at 6:03 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 09/27/2015 12:06 AM, Ingo Molnar wrote:
>>
>> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>>> If we allocate the EFI runtime as a single virtual memory block then issues
>>>> like rounding between sections does not even come up as a problem: we map the
>>>> original offsets and sizes byte by byte.
>>>
>>> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first
>>> place, and just use the 1:1 mapping UEFI uses natively. This is more than
>>> feasible on arm64, and I actually fought hard against using
>>> SetVirtualAddressMap() at all, but I was overruled by others. I think this is
>>> also trivially possible on X64, since the 1:1 mapping is already active
>>> alongside the VA mapping.
>>
>> Could we please re-list all the arguments pro and contra of 1:1 physical mappings,
>> in a post that also explains the background so that more people can chime in, not
>> just people versed in EFI internals? It's very much possible that a bad decision
>> was made.
>>
>
> Pro: by far the sanest way to map the UEFI tables.
> Con: doesn't actually work (breaks on several known platforms.)

Can we at least do 1:1 without an offset on arm64?  Given that Linux
seems to be more of a reference on arm64 than Windows is, maybe that
would give everyone something vaguely sane to work with.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235692 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

On 09/29/2015 06:16 PM, Andy Lutomirski wrote:
> 
> Can we at least do 1:1 without an offset on arm64?  Given that Linux
> seems to be more of a reference on arm64 than Windows is, maybe that
> would give everyone something vaguely sane to work with.
> 

I have no idea.  That's a question for the ARM folks.

	-hpa


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235741

On 30 September 2015 at 03:16, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Sep 29, 2015 at 6:03 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 09/27/2015 12:06 AM, Ingo Molnar wrote:
>>>
>>> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>>>> If we allocate the EFI runtime as a single virtual memory block then issues
>>>>> like rounding between sections does not even come up as a problem: we map the
>>>>> original offsets and sizes byte by byte.
>>>>
>>>> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first
>>>> place, and just use the 1:1 mapping UEFI uses natively. This is more than
>>>> feasible on arm64, and I actually fought hard against using
>>>> SetVirtualAddressMap() at all, but I was overruled by others. I think this is
>>>> also trivially possible on X64, since the 1:1 mapping is already active
>>>> alongside the VA mapping.
>>>
>>> Could we please re-list all the arguments pro and contra of 1:1 physical mappings,
>>> in a post that also explains the background so that more people can chime in, not
>>> just people versed in EFI internals? It's very much possible that a bad decision
>>> was made.
>>>
>>
>> Pro: by far the sanest way to map the UEFI tables.
>> Con: doesn't actually work (breaks on several known platforms.)
>
> Can we at least do 1:1 without an offset on arm64?  Given that Linux
> seems to be more of a reference on arm64 than Windows is, maybe that
> would give everyone something vaguely sane to work with.
>

Yes, as I mentioned before in this thread, on arm64 this is very much
feasible, and it was my strong preference all along. However, the
arguments made by others that outweighed my preference were
(a) x86 uses it
(b) if we don't use it now, we will never be able to start using it
later since it will undoubtedly be broken in /some/ implementation in
the field.

As I also mentioned, the only minor complication is that arm64's VA
space may be configured to be smaller than the physical base of DRAM,
but I already had to address that for the boot ID map and KVM as well.

I will cook up a patch later today.

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237262 — Re: [PATCH 2/2] arm64/efi: Don't pad between EFI_MEMORY_RUNTIME regions

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 09/27/2015 12:06 AM, Ingo Molnar wrote:
> > 
> > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > 
> >>> If we allocate the EFI runtime as a single virtual memory block then issues 
> >>> like rounding between sections does not even come up as a problem: we map the 
> >>> original offsets and sizes byte by byte.
> >>
> >> Well, by that reasoning, we should not call SetVirtualAddressMap() in the first 
> >> place, and just use the 1:1 mapping UEFI uses natively. This is more than 
> >> feasible on arm64, and I actually fought hard against using 
> >> SetVirtualAddressMap() at all, but I was overruled by others. I think this is 
> >> also trivially possible on X64, since the 1:1 mapping is already active 
> >> alongside the VA mapping.
> > 
> > Could we please re-list all the arguments pro and contra of 1:1 physical mappings, 
> > in a post that also explains the background so that more people can chime in, not 
> > just people versed in EFI internals? It's very much possible that a bad decision 
> > was made.
> > 
> 
> Pro: by far the sanest way to map the UEFI tables.
> Con: doesn't actually work (breaks on several known platforms.)

You knew this next question was coming: in what way does it break on known 
platforms?

I.e. do those platforms require a SetVirtualAddressMap() call and break if one 
does not come?

Note that there's 3 models possible:

 - pure 1:1
 - 1:1 plus offset, with SetVirtualAddressMap(offset)
 - bottom up allocator

I don't think we want 'pure' 1:1 physical/virtual (for security reasons, etc.).

So the question is, in what way does our current proposed bottom-up allocator 
differ from 1:1 plus offset? My impression is that they are mostly identical.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237396 — [tip:core/urgent] arm64/efi: Fix boot crash by not padding between EFI_MEMORY_RUNTIME regions

Commit-ID:  0ce3cc008ec04258b6a6314b09f1a6012810881a
Gitweb:     http://git.kernel.org/tip/0ce3cc008ec04258b6a6314b09f1a6012810881a
Author:     Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Fri, 25 Sep 2015 23:02:19 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 1 Oct 2015 12:51:28 +0200

arm64/efi: Fix boot crash by not padding between EFI_MEMORY_RUNTIME regions

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>
Tested-by: Mark Rutland <mark.rutland@arm.com> [UEFI 2.4 only]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
Reviewed-by: Mark Salter <msalter@redhat.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Cc: <stable@vger.kernel.org> # v4.0+
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/1443218539-7610-3-git-send-email-matt@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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 e8ca6ea..13671a9 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 e29560e..950c87f 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web