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


Groups > linux.kernel > #1221079 > unrolled thread

Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime

Started byMatt Fleming <matt@codeblueprint.co.uk>
First post2015-09-08 22:50 +0200
Last post2015-09-16 16:10 +0200
Articles 11 — 5 participants

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

  Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-08 22:50 +0200
    Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime joeyli <jlee@suse.com> - 2015-09-09 02:40 +0200
      Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-09 13:30 +0200
        Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime joeyli <jlee@suse.com> - 2015-09-10 05:40 +0200
        Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Borislav Petkov <bp@suse.de> - 2015-09-16 12:10 +0200
          Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-16 13:30 +0200
            Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Borislav Petkov <bp@suse.de> - 2015-09-16 15:30 +0200
              Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-16 15:40 +0200
                Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Borislav Petkov <bp@suse.de> - 2015-09-17 10:10 +0200
            Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime James Bottomley <jbottomley@odin.com> - 2015-09-16 15:40 +0200
              Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-16 16:10 +0200

#1221079 — Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2015-09-08 22:50 +0200
SubjectRe: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime
Message-ID<q6y01-6V3-1@gated-at.bofh.it>
On Mon, 07 Sep, at 12:07:52PM, joeyli wrote:
> 
> This patch works to me on Intel S1200V3RPS to fix issue:
> DMI: Intel Corporation (uefidk.com) Intel Server Board S1200V3RPS UEFI Development Kit/ROMLEY, BIOS 2.0
> 
> Tested-by: Lee, Chun-Yi <jlee@suse.com>

When you say "fix issue", do you mean that your machine has the
EFI_PROPERTIES_TABLE feature enabled, and that it doesn't boot without
this patch?

-- 
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] | [next] | [standalone]


#1221156

Fromjoeyli <jlee@suse.com>
Date2015-09-09 02:40 +0200
Message-ID<q6BAB-3Go-3@gated-at.bofh.it>
In reply to#1221079
Hi Matt,

On Tue, Sep 08, 2015 at 09:41:47PM +0100, Matt Fleming wrote:
> On Mon, 07 Sep, at 12:07:52PM, joeyli wrote:
> > 
> > This patch works to me on Intel S1200V3RPS to fix issue:
> > DMI: Intel Corporation (uefidk.com) Intel Server Board S1200V3RPS UEFI Development Kit/ROMLEY, BIOS 2.0
> > 
> > Tested-by: Lee, Chun-Yi <jlee@suse.com>
> 
> When you say "fix issue", do you mean that your machine has the
> EFI_PROPERTIES_TABLE feature enabled, and that it doesn't boot without
> this patch?
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Yes, the machine on my hand has EFI_PROPERTIES_TABLE enabled, and it doesn't
boot without your patch.

I captured similar kernel oops through serial port:

[    0.037745] ACPI: All ACPI Tables successfully acquired
[    0.044666] BUG: unable to handle kernel paging request at fffffffef0e5d450
[    0.052451] IP: [<fffffffef0e5d450>] 0xfffffffef0e5d450
[    0.058291] PGD 1c0d067 PUD 17fcfd063 PMD 17fd6f063 PTE 0
[    0.064355] Oops: 0010 [#1] SMP 
[    0.067972] Modules linked in:
[    0.071388] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.2.0-rc8-2.gc02428d-default #1
[    0.080121] Hardware name: Intel Corporation (uefidk.com) Intel Server Board S1200V3RPS UEFI Development Kit/ROMLEY, BIOS 2.0 
[    0.092827] task: ffffffff81c114c0 ti: ffffffff81c00000 task.ti: ffffffff81c00000
[    0.101172] RIP: 0010:[<fffffffef0e5d450>]  [<fffffffef0e5d450>] 0xfffffffef0e5d450
[    0.109724] RSP: 0000:ffffffff81c03d38  EFLAGS: 00010082
[    0.115647] RAX: fffffffef0c5d100 RBX: fffffffef1d66040 RCX: fffffffef0c5d100
[    0.123604] RDX: 00000000be366018 RSI: 0000000000000000 RDI: ffffffff81c38b00
[    0.131560] RBP: 000000000000000c R08: ffffffff81c03d70 R09: ffffffff81c38b0b
[    0.139517] R10: 0000000000000078 R11: 0000000000000002 R12: 0000000000000296
[    0.147475] R13: ffffffff81c03eb8 R14: 0000000000000030 R15: 0000000000000007
[    0.155432] FS:  0000000000000000(0000) GS:ffff88042e600000(0000) knlGS:0000000000000000
[    0.164457] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.170864] CR2: fffffffef0e5d450 CR3: 000000000009b000 CR4: 00000000000406b0
[    0.178819] Stack:
[    0.181059]  fffffffef10574c9 0000000000000002 0000000000000000 0000000000000000
[    0.189351]  ffffffff81c38b00 0000000000000000 fffffffef1057a4b 0000000000000058
[    0.197645]  000000078000203c 000000000009b000 ffff8801ffd70000 000000000009b000
[    0.205939] Call Trace:
[    0.208669]  [<ffffffff8105dd9e>] ? efi_call+0x7e/0x100
[    0.214497]  [<ffffffff81523006>] ? virt_efi_set_variable+0x66/0x90
[    0.221487]  [<ffffffff8105cd67>] ? efi_delete_dummy_variable+0x77/0x90
[    0.228866]  [<ffffffff81d41ff5>] ? efi_enter_virtual_mode+0x3ac/0x3bb
[    0.236147]  [<ffffffff81d26f24>] ? start_kernel+0x3f4/0x484
[    0.242459]  [<ffffffff81d26120>] ? early_idt_handler_array+0x120/0x120
[    0.249835]  [<ffffffff81d26315>] ? x86_64_start_reservations+0x2a/0x2c
[    0.257212]  [<ffffffff81d26452>] ? x86_64_start_kernel+0x13b/0x14a
[    0.264200] Code:  Bad RIP value.
[    0.267916] RIP  [<fffffffef0e5d450>] 0xfffffffef0e5d450
[    0.273851]  RSP <ffffffff81c03d38>
[    0.277739] CR2: fffffffef0e5d450
[    0.281436] ---[ end trace 19be7a419bfa9401 ]---
[    0.286575] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.294049] Rebooting in 90 seconds..
[    0.299184] ACPI MEMORY or I/O RESET_REG.


Thanks a lot!
Joey Lee
--
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]


#1221375

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2015-09-09 13:30 +0200
Message-ID<q6LJF-1Dc-21@gated-at.bofh.it>
In reply to#1221156
On Wed, 09 Sep, at 08:33:07AM, joeyli wrote:
> 
> Yes, the machine on my hand has EFI_PROPERTIES_TABLE enabled, and it doesn't
> boot without your patch.

Awesome. Could you test the following patch instead?

---

From 24d324b781a3b688dcc265995949a9cf4e8af687 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@intel.com>
Date: Thu, 3 Sep 2015 15:56:25 +0100
Subject: [PATCH v2] x86/efi: Map EFI memmap entries in-order at runtime

Beginning with UEFI v2.5 EFI_PROPERTIES_TABLE was introduced that
signals that the firmware PE/COFF loader supports splitting code and
data sections of PE/COFF images into separate EFI memory map entries.
This allows the kernel to map those regions with strict memory
protections, e.g. EFI_MEMORY_RO for code, EFI_MEMORY_XP for data, etc.

Unfortunately, an unwritten requirement of this new feature is that
the regions need to be mapped with the same offsets relative to each
other as observed in the EFI memory map. If this is not done crashes
like this may occur,

 [    0.006391] BUG: unable to handle kernel paging request at fffffffefe6086dd
 [    0.006923] IP: [<fffffffefe6086dd>] 0xfffffffefe6086dd
 [    0.007000] Call Trace:
 [    0.007000]  [<ffffffff8104c90e>] efi_call+0x7e/0x100
 [    0.007000]  [<ffffffff81602091>] ? virt_efi_set_variable+0x61/0x90
 [    0.007000]  [<ffffffff8104c583>] efi_delete_dummy_variable+0x63/0x70
 [    0.007000]  [<ffffffff81f4e4aa>] efi_enter_virtual_mode+0x383/0x392
 [    0.007000]  [<ffffffff81f37e1b>] start_kernel+0x38a/0x417
 [    0.007000]  [<ffffffff81f37495>] x86_64_start_reservations+0x2a/0x2c
 [    0.007000]  [<ffffffff81f37582>] x86_64_start_kernel+0xeb/0xef

Here 0xfffffffefe6086dd refers to an address the firmware expects to
be mapped but which the OS never claimed was mapped. The issue is that
included in these regions are relative addresses to other regions
which were emitted by the firmware toolchain before the "splitting" of
sections occurred at runtime.

Needless to say, we don't satisfy this unwritten requirement on x86_64
and instead map the EFI memory map entries in reverse order. The above
crash is almost certainly triggerable with any kernel newer than v3.13
because that's when we rewrote the EFI runtime region mapping code, in
commit d2f7cbe7b26a ("x86/efi: Runtime services virtual mapping"). For
kernel versions before v3.13 things may work by pure luck depending on
the fragmentation of the kernel virtual address space at the time we
map the EFI regions.

Instead of mapping the EFI memory map entries in reverse order, where
entry N has a higher virtual address than entry N+1, map them in the
same order as they appear in the EFI memory map to preserve this
relative offset between regions.

This patch has been kept as small as possible with the intention that
it should be applied aggressively to stable and distribution kernels.
It is very much a bugfix rather than support for a new feature, since
when EFI_PROPERTIES_TABLE is enabled we must map things as outlined
above to even boot - we have no way of asking the firmware not to
split the code/data regions.

In fact, this patch doesn't even make use of the more strict memory
protections available in UEFI v2.5. That will come later.

Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Peter Jones <pjones@redhat.com>
Cc: James Bottomley <JBottomley@Odin.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---

v2: Use Ard's reverse iteration scheme so that we can reuse the
existing efi_map_region() implementation that maps things top-down.

 arch/x86/platform/efi/efi.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e4308fe6afe8..c6835bfad3a1 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -705,6 +705,70 @@ out:
 }
 
 /*
+ * Iterate the EFI memory map in reverse order because the regions
+ * will be mapped top-down. The end result is the same as if we had
+ * mapped things forward, but doesn't require us to change the
+ * existing implementation of efi_map_region().
+ */
+static inline void *efi_map_next_entry_reverse(void *entry)
+{
+	/* Initial call */
+	if (!entry)
+		return memmap.map_end - memmap.desc_size;
+
+	entry -= memmap.desc_size;
+	if (entry < memmap.map)
+		return NULL;
+
+	return entry;
+}
+
+/*
+ * efi_map_next_entry - Return the next EFI memory map descriptor
+ * @entry: Previous EFI memory map descriptor
+ *
+ * This is a helper function to iterate over the EFI memory map, which
+ * we do in different orders depending on the current configuration.
+ *
+ * To begin traversing the memory map @entry must be %NULL.
+ *
+ * Returns %NULL when we reach the end of the memory map.
+ */
+static void *efi_map_next_entry(void *entry)
+{
+	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
+		/*
+		 * Starting in UEFI v2.5 the EFI_PROPERTIES_TABLE
+		 * config table feature requires us to map all entries
+		 * in the same order as they appear in the EFI memory
+		 * map. That is to say, entry N must have a lower
+		 * virtual address than entry N+1. This is because the
+		 * firmware toolchain leaves relative references in
+		 * the code/data sections, which are split and become
+		 * separate EFI memory regions. Mapping things
+		 * out-of-order leads to the firmware accessing
+		 * unmapped addresses.
+		 *
+		 * Since we need to map things this way whether or not
+		 * the kernel actually makes use of
+		 * EFI_PROPERTIES_TABLE, let's just switch to this
+		 * scheme by default for 64-bit.
+		 */
+		return efi_map_next_entry_reverse(entry);
+	}
+
+	/* Initial call */
+	if (!entry)
+		return memmap.map;
+
+	entry += memmap.desc_size;
+	if (entry >= memmap.map_end)
+		return NULL;
+
+	return entry;
+}
+
+/*
  * Map the efi memory ranges of the runtime services and update new_mmap with
  * virtual addresses.
  */
@@ -714,7 +778,8 @@ static void * __init efi_map_regions(int *count, int *pg_shift)
 	unsigned long left = 0;
 	efi_memory_desc_t *md;
 
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+	p = NULL;
+	while ((p = efi_map_next_entry(p))) {
 		md = p;
 		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
 #ifdef CONFIG_X86_64
-- 
2.1.0

-- 
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]


#1221871

Fromjoeyli <jlee@suse.com>
Date2015-09-10 05:40 +0200
Message-ID<q70Sm-6tp-9@gated-at.bofh.it>
In reply to#1221375
Hi,

On Wed, Sep 09, 2015 at 12:21:23PM +0100, Matt Fleming wrote:
> On Wed, 09 Sep, at 08:33:07AM, joeyli wrote:
> > 
> > Yes, the machine on my hand has EFI_PROPERTIES_TABLE enabled, and it doesn't
> > boot without your patch.
> 
> Awesome. Could you test the following patch instead?
> 
> ---

Yes, as the first edition, this patch works on my S1200V3RPS machine.

Tested-by: Lee, Chun-Yi <jlee@suse.com>

Regards
Joey Lee

> 
> >From 24d324b781a3b688dcc265995949a9cf4e8af687 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming@intel.com>
> Date: Thu, 3 Sep 2015 15:56:25 +0100
> Subject: [PATCH v2] x86/efi: Map EFI memmap entries in-order at runtime
> 
> Beginning with UEFI v2.5 EFI_PROPERTIES_TABLE was introduced that
> signals that the firmware PE/COFF loader supports splitting code and
> data sections of PE/COFF images into separate EFI memory map entries.
> This allows the kernel to map those regions with strict memory
> protections, e.g. EFI_MEMORY_RO for code, EFI_MEMORY_XP for data, etc.
> 
> Unfortunately, an unwritten requirement of this new feature is that
> the regions need to be mapped with the same offsets relative to each
> other as observed in the EFI memory map. If this is not done crashes
> like this may occur,
> 
>  [    0.006391] BUG: unable to handle kernel paging request at fffffffefe6086dd
>  [    0.006923] IP: [<fffffffefe6086dd>] 0xfffffffefe6086dd
>  [    0.007000] Call Trace:
>  [    0.007000]  [<ffffffff8104c90e>] efi_call+0x7e/0x100
>  [    0.007000]  [<ffffffff81602091>] ? virt_efi_set_variable+0x61/0x90
>  [    0.007000]  [<ffffffff8104c583>] efi_delete_dummy_variable+0x63/0x70
>  [    0.007000]  [<ffffffff81f4e4aa>] efi_enter_virtual_mode+0x383/0x392
>  [    0.007000]  [<ffffffff81f37e1b>] start_kernel+0x38a/0x417
>  [    0.007000]  [<ffffffff81f37495>] x86_64_start_reservations+0x2a/0x2c
>  [    0.007000]  [<ffffffff81f37582>] x86_64_start_kernel+0xeb/0xef
> 
> Here 0xfffffffefe6086dd refers to an address the firmware expects to
> be mapped but which the OS never claimed was mapped. The issue is that
> included in these regions are relative addresses to other regions
> which were emitted by the firmware toolchain before the "splitting" of
> sections occurred at runtime.
> 
> Needless to say, we don't satisfy this unwritten requirement on x86_64
> and instead map the EFI memory map entries in reverse order. The above
> crash is almost certainly triggerable with any kernel newer than v3.13
> because that's when we rewrote the EFI runtime region mapping code, in
> commit d2f7cbe7b26a ("x86/efi: Runtime services virtual mapping"). For
> kernel versions before v3.13 things may work by pure luck depending on
> the fragmentation of the kernel virtual address space at the time we
> map the EFI regions.
> 
> Instead of mapping the EFI memory map entries in reverse order, where
> entry N has a higher virtual address than entry N+1, map them in the
> same order as they appear in the EFI memory map to preserve this
> relative offset between regions.
> 
> This patch has been kept as small as possible with the intention that
> it should be applied aggressively to stable and distribution kernels.
> It is very much a bugfix rather than support for a new feature, since
> when EFI_PROPERTIES_TABLE is enabled we must map things as outlined
> above to even boot - we have no way of asking the firmware not to
> split the code/data regions.
> 
> In fact, this patch doesn't even make use of the more strict memory
> protections available in UEFI v2.5. That will come later.
> 
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Lee, Chun-Yi <jlee@suse.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Peter Jones <pjones@redhat.com>
> Cc: James Bottomley <JBottomley@Odin.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
> 
> v2: Use Ard's reverse iteration scheme so that we can reuse the
> existing efi_map_region() implementation that maps things top-down.
> 
>  arch/x86/platform/efi/efi.c | 67 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index e4308fe6afe8..c6835bfad3a1 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -705,6 +705,70 @@ out:
>  }
>  
>  /*
> + * Iterate the EFI memory map in reverse order because the regions
> + * will be mapped top-down. The end result is the same as if we had
> + * mapped things forward, but doesn't require us to change the
> + * existing implementation of efi_map_region().
> + */
> +static inline void *efi_map_next_entry_reverse(void *entry)
> +{
> +	/* Initial call */
> +	if (!entry)
> +		return memmap.map_end - memmap.desc_size;
> +
> +	entry -= memmap.desc_size;
> +	if (entry < memmap.map)
> +		return NULL;
> +
> +	return entry;
> +}
> +
> +/*
> + * efi_map_next_entry - Return the next EFI memory map descriptor
> + * @entry: Previous EFI memory map descriptor
> + *
> + * This is a helper function to iterate over the EFI memory map, which
> + * we do in different orders depending on the current configuration.
> + *
> + * To begin traversing the memory map @entry must be %NULL.
> + *
> + * Returns %NULL when we reach the end of the memory map.
> + */
> +static void *efi_map_next_entry(void *entry)
> +{
> +	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
> +		/*
> +		 * Starting in UEFI v2.5 the EFI_PROPERTIES_TABLE
> +		 * config table feature requires us to map all entries
> +		 * in the same order as they appear in the EFI memory
> +		 * map. That is to say, entry N must have a lower
> +		 * virtual address than entry N+1. This is because the
> +		 * firmware toolchain leaves relative references in
> +		 * the code/data sections, which are split and become
> +		 * separate EFI memory regions. Mapping things
> +		 * out-of-order leads to the firmware accessing
> +		 * unmapped addresses.
> +		 *
> +		 * Since we need to map things this way whether or not
> +		 * the kernel actually makes use of
> +		 * EFI_PROPERTIES_TABLE, let's just switch to this
> +		 * scheme by default for 64-bit.
> +		 */
> +		return efi_map_next_entry_reverse(entry);
> +	}
> +
> +	/* Initial call */
> +	if (!entry)
> +		return memmap.map;
> +
> +	entry += memmap.desc_size;
> +	if (entry >= memmap.map_end)
> +		return NULL;
> +
> +	return entry;
> +}
> +
> +/*
>   * Map the efi memory ranges of the runtime services and update new_mmap with
>   * virtual addresses.
>   */
> @@ -714,7 +778,8 @@ static void * __init efi_map_regions(int *count, int *pg_shift)
>  	unsigned long left = 0;
>  	efi_memory_desc_t *md;
>  
> -	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> +	p = NULL;
> +	while ((p = efi_map_next_entry(p))) {
>  		md = p;
>  		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
>  #ifdef CONFIG_X86_64
> -- 
> 2.1.0
> 
> -- 
> 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]


#1225907

FromBorislav Petkov <bp@suse.de>
Date2015-09-16 12:10 +0200
Message-ID<q9hP3-1id-1@gated-at.bofh.it>
In reply to#1221375
On Wed, Sep 09, 2015 at 12:21:23PM +0100, Matt Fleming wrote:
> On Wed, 09 Sep, at 08:33:07AM, joeyli wrote:
> > 
> > Yes, the machine on my hand has EFI_PROPERTIES_TABLE enabled, and it doesn't
> > boot without your patch.
> 
> Awesome. Could you test the following patch instead?
> 
> ---
> 
> From 24d324b781a3b688dcc265995949a9cf4e8af687 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming@intel.com>
> Date: Thu, 3 Sep 2015 15:56:25 +0100
> Subject: [PATCH v2] x86/efi: Map EFI memmap entries in-order at runtime
> 
> Beginning with UEFI v2.5 EFI_PROPERTIES_TABLE was introduced that
> signals that the firmware PE/COFF loader supports splitting code and
> data sections of PE/COFF images into separate EFI memory map entries.
> This allows the kernel to map those regions with strict memory
> protections, e.g. EFI_MEMORY_RO for code, EFI_MEMORY_XP for data, etc.
> 
> Unfortunately, an unwritten requirement of this new feature is that
> the regions need to be mapped with the same offsets relative to each
> other as observed in the EFI memory map. If this is not done crashes

Let me get this straight: this looks like the next EFI screwup which
practically requires specific mapping placement in VA space just
because it uses relative addresses? And since you say "unwritten" this
practically a requirement is not even in the spec?

Can we state explicitly in the spec NOT to rely on mapping VA placement?
I mean, this "unwritten" requirement is seriously screwed on soo many
levels...

What else are we to expect? Spelled out virtual addresses which are
going to be the EFI-allowed ones only??!

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
--
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]


#1226016

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2015-09-16 13:30 +0200
Message-ID<q9j4u-31b-21@gated-at.bofh.it>
In reply to#1225907
On 16 September 2015 at 12:08, Borislav Petkov <bp@suse.de> wrote:
> On Wed, Sep 09, 2015 at 12:21:23PM +0100, Matt Fleming wrote:
>> On Wed, 09 Sep, at 08:33:07AM, joeyli wrote:
>> >
>> > Yes, the machine on my hand has EFI_PROPERTIES_TABLE enabled, and it doesn't
>> > boot without your patch.
>>
>> Awesome. Could you test the following patch instead?
>>
>> ---
>>
>> From 24d324b781a3b688dcc265995949a9cf4e8af687 Mon Sep 17 00:00:00 2001
>> From: Matt Fleming <matt.fleming@intel.com>
>> Date: Thu, 3 Sep 2015 15:56:25 +0100
>> Subject: [PATCH v2] x86/efi: Map EFI memmap entries in-order at runtime
>>
>> Beginning with UEFI v2.5 EFI_PROPERTIES_TABLE was introduced that
>> signals that the firmware PE/COFF loader supports splitting code and
>> data sections of PE/COFF images into separate EFI memory map entries.
>> This allows the kernel to map those regions with strict memory
>> protections, e.g. EFI_MEMORY_RO for code, EFI_MEMORY_XP for data, etc.
>>
>> Unfortunately, an unwritten requirement of this new feature is that
>> the regions need to be mapped with the same offsets relative to each
>> other as observed in the EFI memory map. If this is not done crashes
>
> Let me get this straight: this looks like the next EFI screwup which
> practically requires specific mapping placement in VA space just
> because it uses relative addresses?

Both relative and absolute references, currently. The latter are also
affected since the relocation offset that is applied to all PE/COFF
relocation entries is based on the displacement of ImageBase, and
absolute references to symbols in .data need to be treated specially
(since it may be shifted relative to the .text section containing
ImageBase). This could be worked around by converting each absolute
reference individually using ConvertPointer () [and I have a proof of
concept that actually makes the problem go away on x86] but it would
still be only a partial solution, since relative references are not
tracked in the PE/COFF metadata, so even if we wanted to, it would be
intractible to find each cross-section relative reference and do the
fixup.

> And since you say "unwritten" this
> practically a requirement is not even in the spec?
>

No, it seems nobody thought of this when designing the feature.

> Can we state explicitly in the spec NOT to rely on mapping VA placement?
> I mean, this "unwritten" requirement is seriously screwed on soo many
> levels...
>

Several solutions and/or work arounds are currently under discussion

-- 
Ard.

> What else are we to expect? Spelled out virtual addresses which are
> going to be the EFI-allowed ones only??!
>
> --
> Regards/Gruss,
>     Boris.
>
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --
--
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]


#1226082

FromBorislav Petkov <bp@suse.de>
Date2015-09-16 15:30 +0200
Message-ID<q9kWE-5Hr-51@gated-at.bofh.it>
In reply to#1226016
On Wed, Sep 16, 2015 at 01:25:06PM +0200, Ard Biesheuvel wrote:
> ... so even if we wanted to, it would be intractible to find each
> cross-section relative reference and do the fixup.

Hmm, maybe we should go and patch EFI code segments and fixup all
relative references after mapping. I mean, if you want something done
right, you better do it yourself. :-\

> No, it seems nobody thought of this when designing the feature.

Not surprised at all, to be honest.

> Several solutions and/or work arounds are currently under discussion.

And requiring for code segments not to refer to each other with relative
offsets and holding that down in the spec post-factum is not possible
anymore...?

 [ I can already imagine what the answer to that question would be though... ]

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
--
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]


#1226091

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2015-09-16 15:40 +0200
Message-ID<q9l6l-5SL-63@gated-at.bofh.it>
In reply to#1226082
On 16 September 2015 at 15:28, Borislav Petkov <bp@suse.de> wrote:
> On Wed, Sep 16, 2015 at 01:25:06PM +0200, Ard Biesheuvel wrote:
>> ... so even if we wanted to, it would be intractible to find each
>> cross-section relative reference and do the fixup.
>
> Hmm, maybe we should go and patch EFI code segments and fixup all
> relative references after mapping. I mean, if you want something done
> right, you better do it yourself. :-\
>

That is a can of worms I'd rather keep closed, if you don't mind ...

>> No, it seems nobody thought of this when designing the feature.
>
> Not surprised at all, to be honest.
>
>> Several solutions and/or work arounds are currently under discussion.
>
> And requiring for code segments not to refer to each other with relative
> offsets and holding that down in the spec post-factum is not possible
> anymore...?
>
>  [ I can already imagine what the answer to that question would be though... ]
>

Fixing the spec is easy. Modifying all the toolchains out there to add
an option that inhibits cross-section relative references is the
problem. Note that, at the object level, it is not necessarily obvious
to the compiler whether a symbol reference will end up referring to
another section than its own. So this basically means 'no relative
references at all', and most compilers don't have that option yet.
[Note that GCC's large code model, which makes no assumption about the
proximity of external symbols, may still emit PC relative literals
(e.g., '.quad sym - .') on ARM. Not sure about X64)

But in general, since we are already violating the PE/COFF spec by
relocating each runtime image once, then invoke its entry point, then
fire an event which it should catch to manually update its pointers,
and then relocate it again into the OS VA space.
--
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]


#1226734

FromBorislav Petkov <bp@suse.de>
Date2015-09-17 10:10 +0200
Message-ID<q9Cqu-5Ub-11@gated-at.bofh.it>
In reply to#1226091
On Wed, Sep 16, 2015 at 03:38:45PM +0200, Ard Biesheuvel wrote:
> That is a can of worms I'd rather keep closed, if you don't mind ...

Same here.

> But in general, since we are already violating the PE/COFF spec by
> relocating each runtime image once, then invoke its entry point, then
> fire an event which it should catch to manually update its pointers,
> and then relocate it again into the OS VA space.

Yeah, I vaguely remember at the time hpa proposing an EFI-specific
page fault handler or so. I guess we should consider such or similar
technique as it should be most flexible to deal with such screwups. And
with whatever funky fw stuff comes our way in the future...

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
--
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]


#1226084

FromJames Bottomley <jbottomley@odin.com>
Date2015-09-16 15:40 +0200
Message-ID<q9l6i-5SL-19@gated-at.bofh.it>
In reply to#1226016
T24gV2VkLCAyMDE1LTA5LTE2IGF0IDEzOjI1ICswMjAwLCBBcmQgQmllc2hldXZlbCB3cm90ZToN
Cj4gT24gMTYgU2VwdGVtYmVyIDIwMTUgYXQgMTI6MDgsIEJvcmlzbGF2IFBldGtvdiA8YnBAc3Vz
ZS5kZT4gd3JvdGU6DQo+ID4gT24gV2VkLCBTZXAgMDksIDIwMTUgYXQgMTI6MjE6MjNQTSArMDEw
MCwgTWF0dCBGbGVtaW5nIHdyb3RlOg0KPiA+PiBPbiBXZWQsIDA5IFNlcCwgYXQgMDg6MzM6MDdB
TSwgam9leWxpIHdyb3RlOg0KPiA+PiA+DQo+ID4+ID4gWWVzLCB0aGUgbWFjaGluZSBvbiBteSBo
YW5kIGhhcyBFRklfUFJPUEVSVElFU19UQUJMRSBlbmFibGVkLCBhbmQgaXQgZG9lc24ndA0KPiA+
PiA+IGJvb3Qgd2l0aG91dCB5b3VyIHBhdGNoLg0KPiA+Pg0KPiA+PiBBd2Vzb21lLiBDb3VsZCB5
b3UgdGVzdCB0aGUgZm9sbG93aW5nIHBhdGNoIGluc3RlYWQ/DQo+ID4+DQo+ID4+IC0tLQ0KPiA+
Pg0KPiA+PiBGcm9tIDI0ZDMyNGI3ODFhM2I2ODhkY2MyNjU5OTU5NDlhOWNmNGU4YWY2ODcgTW9u
IFNlcCAxNyAwMDowMDowMCAyMDAxDQo+ID4+IEZyb206IE1hdHQgRmxlbWluZyA8bWF0dC5mbGVt
aW5nQGludGVsLmNvbT4NCj4gPj4gRGF0ZTogVGh1LCAzIFNlcCAyMDE1IDE1OjU2OjI1ICswMTAw
DQo+ID4+IFN1YmplY3Q6IFtQQVRDSCB2Ml0geDg2L2VmaTogTWFwIEVGSSBtZW1tYXAgZW50cmll
cyBpbi1vcmRlciBhdCBydW50aW1lDQo+ID4+DQo+ID4+IEJlZ2lubmluZyB3aXRoIFVFRkkgdjIu
NSBFRklfUFJPUEVSVElFU19UQUJMRSB3YXMgaW50cm9kdWNlZCB0aGF0DQo+ID4+IHNpZ25hbHMg
dGhhdCB0aGUgZmlybXdhcmUgUEUvQ09GRiBsb2FkZXIgc3VwcG9ydHMgc3BsaXR0aW5nIGNvZGUg
YW5kDQo+ID4+IGRhdGEgc2VjdGlvbnMgb2YgUEUvQ09GRiBpbWFnZXMgaW50byBzZXBhcmF0ZSBF
RkkgbWVtb3J5IG1hcCBlbnRyaWVzLg0KPiA+PiBUaGlzIGFsbG93cyB0aGUga2VybmVsIHRvIG1h
cCB0aG9zZSByZWdpb25zIHdpdGggc3RyaWN0IG1lbW9yeQ0KPiA+PiBwcm90ZWN0aW9ucywgZS5n
LiBFRklfTUVNT1JZX1JPIGZvciBjb2RlLCBFRklfTUVNT1JZX1hQIGZvciBkYXRhLCBldGMuDQo+
ID4+DQo+ID4+IFVuZm9ydHVuYXRlbHksIGFuIHVud3JpdHRlbiByZXF1aXJlbWVudCBvZiB0aGlz
IG5ldyBmZWF0dXJlIGlzIHRoYXQNCj4gPj4gdGhlIHJlZ2lvbnMgbmVlZCB0byBiZSBtYXBwZWQg
d2l0aCB0aGUgc2FtZSBvZmZzZXRzIHJlbGF0aXZlIHRvIGVhY2gNCj4gPj4gb3RoZXIgYXMgb2Jz
ZXJ2ZWQgaW4gdGhlIEVGSSBtZW1vcnkgbWFwLiBJZiB0aGlzIGlzIG5vdCBkb25lIGNyYXNoZXMN
Cj4gPg0KPiA+IExldCBtZSBnZXQgdGhpcyBzdHJhaWdodDogdGhpcyBsb29rcyBsaWtlIHRoZSBu
ZXh0IEVGSSBzY3Jld3VwIHdoaWNoDQo+ID4gcHJhY3RpY2FsbHkgcmVxdWlyZXMgc3BlY2lmaWMg
bWFwcGluZyBwbGFjZW1lbnQgaW4gVkEgc3BhY2UganVzdA0KPiA+IGJlY2F1c2UgaXQgdXNlcyBy
ZWxhdGl2ZSBhZGRyZXNzZXM/DQo+IA0KPiBCb3RoIHJlbGF0aXZlIGFuZCBhYnNvbHV0ZSByZWZl
cmVuY2VzLCBjdXJyZW50bHkuIFRoZSBsYXR0ZXIgYXJlIGFsc28NCj4gYWZmZWN0ZWQgc2luY2Ug
dGhlIHJlbG9jYXRpb24gb2Zmc2V0IHRoYXQgaXMgYXBwbGllZCB0byBhbGwgUEUvQ09GRg0KPiBy
ZWxvY2F0aW9uIGVudHJpZXMgaXMgYmFzZWQgb24gdGhlIGRpc3BsYWNlbWVudCBvZiBJbWFnZUJh
c2UsIGFuZA0KPiBhYnNvbHV0ZSByZWZlcmVuY2VzIHRvIHN5bWJvbHMgaW4gLmRhdGEgbmVlZCB0
byBiZSB0cmVhdGVkIHNwZWNpYWxseQ0KPiAoc2luY2UgaXQgbWF5IGJlIHNoaWZ0ZWQgcmVsYXRp
dmUgdG8gdGhlIC50ZXh0IHNlY3Rpb24gY29udGFpbmluZw0KPiBJbWFnZUJhc2UpLiBUaGlzIGNv
dWxkIGJlIHdvcmtlZCBhcm91bmQgYnkgY29udmVydGluZyBlYWNoIGFic29sdXRlDQo+IHJlZmVy
ZW5jZSBpbmRpdmlkdWFsbHkgdXNpbmcgQ29udmVydFBvaW50ZXIgKCkgW2FuZCBJIGhhdmUgYSBw
cm9vZiBvZg0KPiBjb25jZXB0IHRoYXQgYWN0dWFsbHkgbWFrZXMgdGhlIHByb2JsZW0gZ28gYXdh
eSBvbiB4ODZdIGJ1dCBpdCB3b3VsZA0KPiBzdGlsbCBiZSBvbmx5IGEgcGFydGlhbCBzb2x1dGlv
biwgc2luY2UgcmVsYXRpdmUgcmVmZXJlbmNlcyBhcmUgbm90DQo+IHRyYWNrZWQgaW4gdGhlIFBF
L0NPRkYgbWV0YWRhdGEsIHNvIGV2ZW4gaWYgd2Ugd2FudGVkIHRvLCBpdCB3b3VsZCBiZQ0KPiBp
bnRyYWN0aWJsZSB0byBmaW5kIGVhY2ggY3Jvc3Mtc2VjdGlvbiByZWxhdGl2ZSByZWZlcmVuY2Ug
YW5kIGRvIHRoZQ0KPiBmaXh1cC4NCj4gDQo+ID4gQW5kIHNpbmNlIHlvdSBzYXkgInVud3JpdHRl
biIgdGhpcw0KPiA+IHByYWN0aWNhbGx5IGEgcmVxdWlyZW1lbnQgaXMgbm90IGV2ZW4gaW4gdGhl
IHNwZWM/DQo+ID4NCj4gDQo+IE5vLCBpdCBzZWVtcyBub2JvZHkgdGhvdWdodCBvZiB0aGlzIHdo
ZW4gZGVzaWduaW5nIHRoZSBmZWF0dXJlLg0KDQpUbyBhZGQgY29sb3VyOiBvdXIgcHJvYmxlbSBp
cyBzZWN0aW9uIHJlbGF0aXZlIHJlZmVyZW5jZXMgKGVpdGhlciBsb2Fkcw0Kb3IganVtcHMpLiAg
VGhlIFBFL0NPRkYgbGlua2VyIGlzIGFsbG93ZWQgbm90IHRvIGVtaXQgcmVsb2NhdGlvbnMgZm9y
DQpzZWN0aW9uIHJlbGF0aXZlIHJlZmVyZW5jZXMgYmVjYXVzZSBpdCBleHBlY3RzIHRoYXQgdGhl
IHNlY3Rpb25zIHdpbGwNCmFsd2F5cyBiZSBsb2FkZWQgYXQgdGhlIHNhbWUgcmVsYXRpdmUgb2Zm
c2V0LiAgSXQgbG9va3MgbGlrZSBpdCBpcw0KcG9zc2libGUgdG8gZm9yY2UgdGhlbSB0byBoYXZl
IHJlbG9jYXRpb24gZW50cmllcywgc28gaXQgd291bGQgYmUNCnBvc3NpYmxlIHRvIGFkZCB0byB0
aGUgc3RhbmRhcmQgbGFuZ3VhZ2UgcmVxdWlyaW5nIHRoaXMgZm9yIFVFRkkNCmNvbXBhdGlibGUg
YmluYXJpZXMsIGJ1dCB0aGF0IHdvbid0IGhlbHAgd2l0aCBhbnkgb2YgdGhlIGV4aXN0aW5nDQpQ
RS9DT0ZGIHN0dWZmIGluIHRoZSBmaWVsZC4NCg0KVGhlIHByb2JsZW0gaXMgdGhhdCB0byBhcHBs
eSB0aGUgdmFyaW91cyBwcm90ZWN0aW9ucyBVRUZJIGlzDQppbnRyb2R1Y2luZywgd2UncmUgdHJ5
aW5nIHRvIHJlbG9jYXRlIHRoZSBzZWN0aW9ucyBhbmQgdGhhdCdzIHdoYXQncw0KY2F1c2luZyB0
aGUgaXNzdWUuICBCZWZvcmUgdGhpcywgbm8tb25lIHJlYWxseSB0aG91Z2h0IG9mIG1hcHBpbmcg
dGhlDQpzZWN0aW9ucyBhdCBkaWZmZXJlbnQgcmVsYXRpdmUgYWRkcmVzc2VzLiAgSXQncyByZWFs
bHkgYW4gdW5leHBlY3RlZA0Kd2Vha25lc3MgaW4gdGhlIFBFL0NPRkYgc3BlYyB0aGF0IHdlIGNh
bid0IGZpeCwgc28gd2UgaGF2ZSB0byB3b3JrDQphcm91bmQgaXQuDQoNCkphbWVzDQoNCj4gPiBD
YW4gd2Ugc3RhdGUgZXhwbGljaXRseSBpbiB0aGUgc3BlYyBOT1QgdG8gcmVseSBvbiBtYXBwaW5n
IFZBIHBsYWNlbWVudD8NCj4gPiBJIG1lYW4sIHRoaXMgInVud3JpdHRlbiIgcmVxdWlyZW1lbnQg
aXMgc2VyaW91c2x5IHNjcmV3ZWQgb24gc29vIG1hbnkNCj4gPiBsZXZlbHMuLi4NCj4gPg0KPiAN
Cj4gU2V2ZXJhbCBzb2x1dGlvbnMgYW5kL29yIHdvcmsgYXJvdW5kcyBhcmUgY3VycmVudGx5IHVu
ZGVyIGRpc2N1c3Npb24NCj4gDQoNCg0K
--
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]


#1226111

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2015-09-16 16:10 +0200
Message-ID<q9lzk-6Gb-15@gated-at.bofh.it>
In reply to#1226084
On 16 September 2015 at 15:37, James Bottomley <jbottomley@odin.com> wrote:
> On Wed, 2015-09-16 at 13:25 +0200, Ard Biesheuvel wrote:
>> On 16 September 2015 at 12:08, Borislav Petkov <bp@suse.de> wrote:
>> > On Wed, Sep 09, 2015 at 12:21:23PM +0100, Matt Fleming wrote:
>> >> On Wed, 09 Sep, at 08:33:07AM, joeyli wrote:
>> >> >
>> >> > Yes, the machine on my hand has EFI_PROPERTIES_TABLE enabled, and it doesn't
>> >> > boot without your patch.
>> >>
>> >> Awesome. Could you test the following patch instead?
>> >>
>> >> ---
>> >>
>> >> From 24d324b781a3b688dcc265995949a9cf4e8af687 Mon Sep 17 00:00:00 2001
>> >> From: Matt Fleming <matt.fleming@intel.com>
>> >> Date: Thu, 3 Sep 2015 15:56:25 +0100
>> >> Subject: [PATCH v2] x86/efi: Map EFI memmap entries in-order at runtime
>> >>
>> >> Beginning with UEFI v2.5 EFI_PROPERTIES_TABLE was introduced that
>> >> signals that the firmware PE/COFF loader supports splitting code and
>> >> data sections of PE/COFF images into separate EFI memory map entries.
>> >> This allows the kernel to map those regions with strict memory
>> >> protections, e.g. EFI_MEMORY_RO for code, EFI_MEMORY_XP for data, etc.
>> >>
>> >> Unfortunately, an unwritten requirement of this new feature is that
>> >> the regions need to be mapped with the same offsets relative to each
>> >> other as observed in the EFI memory map. If this is not done crashes
>> >
>> > Let me get this straight: this looks like the next EFI screwup which
>> > practically requires specific mapping placement in VA space just
>> > because it uses relative addresses?
>>
>> Both relative and absolute references, currently. The latter are also
>> affected since the relocation offset that is applied to all PE/COFF
>> relocation entries is based on the displacement of ImageBase, and
>> absolute references to symbols in .data need to be treated specially
>> (since it may be shifted relative to the .text section containing
>> ImageBase). This could be worked around by converting each absolute
>> reference individually using ConvertPointer () [and I have a proof of
>> concept that actually makes the problem go away on x86] but it would
>> still be only a partial solution, since relative references are not
>> tracked in the PE/COFF metadata, so even if we wanted to, it would be
>> intractible to find each cross-section relative reference and do the
>> fixup.
>>
>> > And since you say "unwritten" this
>> > practically a requirement is not even in the spec?
>> >
>>
>> No, it seems nobody thought of this when designing the feature.
>
> To add colour: our problem is section relative references (either loads
> or jumps).  The PE/COFF linker is allowed not to emit relocations for
> section relative references because it expects that the sections will
> always be loaded at the same relative offset.

The PE/COFF spec does not define any relative relocation types to be
used in the .reloc section (which is what is used for runtime
relocations). It does define such relocation types as COFF
relocations, which are more like static relocations in ELF, i.e., what
the linker uses at build time to combine object files into an
executable, but those cannot appear in an executable, only in object
files.

> It looks like it is
> possible to force them to have relocation entries, so it would be
> possible to add to the standard language requiring this for UEFI
> compatible binaries, but that won't help with any of the existing
> PE/COFF stuff in the field.
>

Sadly, no. The PE/COFF spec is clear about COFF relocations appearing
only in object files, and the .reloc section only tracks absolute
references.

> The problem is that to apply the various protections UEFI is
> introducing, we're trying to relocate the sections and that's what's
> causing the issue.  Before this, no-one really thought of mapping the
> sections at different relative addresses.  It's really an unexpected
> weakness in the PE/COFF spec that we can't fix, so we have to work
> around it.
>

To be honest, while I am not a big fan of PE/COFF, I don't think it is
reasonable to expect that an image can be simply split up and moved
apart. I think we (UEFI forum) have dropped the ball here.

I won't go into too much detail about how I think it should be
implemented instead, let's save that for the conf call. But splitting
memory regions that belong together without /any/ annotations
whatsoever in the memory map is just sloppy design.

-- 
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] | [standalone]


Back to top | Article view | linux.kernel


csiph-web