public inbox for linux-efi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] x86/efi: Drop support for the EFI_PROPERTIES_TABLE
@ 2024-11-12 18:52 Nicolas Saenz Julienne
  2024-11-12 18:52 ` [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec Nicolas Saenz Julienne
  0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Saenz Julienne @ 2024-11-12 18:52 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H . Peter Anvin, Matt Fleming, linux-efi, linux-kernel, stanspas,
	nh-open-source, Nicolas Saenz Julienne

Drop support for the EFI_PROPERTIES_TABLE. It was a failed, short-lived
experiment that broke the boot both on Linux and Windows, and was
replaced by the EFI_MEMORY_ATTRIBUTES_TABLE shortly after.

Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>
---
 arch/x86/platform/efi/efi.c    | 19 ---------------
 arch/x86/platform/efi/efi_64.c | 42 ----------------------------------
 include/linux/efi.h            | 17 +++-----------
 3 files changed, 3 insertions(+), 75 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 88a96816de9a..375ebd78296a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -54,14 +54,12 @@
 #include <asm/uv/uv.h>
 
 static unsigned long efi_systab_phys __initdata;
-static unsigned long prop_phys = EFI_INVALID_TABLE_ADDR;
 static unsigned long uga_phys = EFI_INVALID_TABLE_ADDR;
 static unsigned long efi_runtime, efi_nr_tables;
 
 unsigned long efi_fw_vendor, efi_config_table;
 
 static const efi_config_table_type_t arch_tables[] __initconst = {
-	{EFI_PROPERTIES_TABLE_GUID,	&prop_phys,		"PROP"		},
 	{UGA_IO_PROTOCOL_GUID,		&uga_phys,		"UGA"		},
 #ifdef CONFIG_X86_UV
 	{UV_SYSTEM_TABLE_GUID,		&uv_systab_phys,	"UVsystab"	},
@@ -82,7 +80,6 @@ static const unsigned long * const efi_tables[] = {
 	&efi_runtime,
 	&efi_config_table,
 	&efi.esrt,
-	&prop_phys,
 	&efi_mem_attr_table,
 #ifdef CONFIG_EFI_RCI2_TABLE
 	&rci2_table_phys,
@@ -502,22 +499,6 @@ void __init efi_init(void)
 		return;
 	}
 
-	/* Parse the EFI Properties table if it exists */
-	if (prop_phys != EFI_INVALID_TABLE_ADDR) {
-		efi_properties_table_t *tbl;
-
-		tbl = early_memremap_ro(prop_phys, sizeof(*tbl));
-		if (tbl == NULL) {
-			pr_err("Could not map Properties table!\n");
-		} else {
-			if (tbl->memory_protection_attribute &
-			    EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA)
-				set_bit(EFI_NX_PE_DATA, &efi.flags);
-
-			early_memunmap(tbl, sizeof(*tbl));
-		}
-	}
-
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	efi_clean_memmap();
 
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 91d31ac422d6..ac57259a432b 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -412,51 +412,9 @@ static int __init efi_update_mem_attr(struct mm_struct *mm, efi_memory_desc_t *m
 
 void __init efi_runtime_update_mappings(void)
 {
-	efi_memory_desc_t *md;
-
-	/*
-	 * Use the EFI Memory Attribute Table for mapping permissions if it
-	 * exists, since it is intended to supersede EFI_PROPERTIES_TABLE.
-	 */
 	if (efi_enabled(EFI_MEM_ATTR)) {
 		efi_disable_ibt_for_runtime = false;
 		efi_memattr_apply_permissions(NULL, efi_update_mem_attr);
-		return;
-	}
-
-	/*
-	 * EFI_MEMORY_ATTRIBUTES_TABLE is intended to replace
-	 * EFI_PROPERTIES_TABLE. So, use EFI_PROPERTIES_TABLE to update
-	 * permissions only if EFI_MEMORY_ATTRIBUTES_TABLE is not
-	 * published by the firmware. Even if we find a buggy implementation of
-	 * EFI_MEMORY_ATTRIBUTES_TABLE, don't fall back to
-	 * EFI_PROPERTIES_TABLE, because of the same reason.
-	 */
-
-	if (!efi_enabled(EFI_NX_PE_DATA))
-		return;
-
-	for_each_efi_memory_desc(md) {
-		unsigned long pf = 0;
-
-		if (!(md->attribute & EFI_MEMORY_RUNTIME))
-			continue;
-
-		if (!(md->attribute & EFI_MEMORY_WB))
-			pf |= _PAGE_PCD;
-
-		if ((md->attribute & EFI_MEMORY_XP) ||
-			(md->type == EFI_RUNTIME_SERVICES_DATA))
-			pf |= _PAGE_NX;
-
-		if (!(md->attribute & EFI_MEMORY_RO) &&
-			(md->type != EFI_RUNTIME_SERVICES_CODE))
-			pf |= _PAGE_RW;
-
-		if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
-			pf |= _PAGE_ENC;
-
-		efi_update_mappings(md, pf);
 	}
 }
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e28d88066033..e5815867aba9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -379,7 +379,6 @@ void efi_native_runtime_setup(void);
 #define EFI_SYSTEM_RESOURCE_TABLE_GUID		EFI_GUID(0xb122a263, 0x3661, 0x4f68,  0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80)
 #define EFI_FILE_SYSTEM_GUID			EFI_GUID(0x964e5b22, 0x6459, 0x11d2,  0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 #define DEVICE_TREE_GUID			EFI_GUID(0xb1b621d5, 0xf19c, 0x41a5,  0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0)
-#define EFI_PROPERTIES_TABLE_GUID		EFI_GUID(0x880aaca3, 0x4adc, 0x4a04,  0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5)
 #define EFI_RNG_PROTOCOL_GUID			EFI_GUID(0x3152bca5, 0xeade, 0x433d,  0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
 #define EFI_RNG_ALGORITHM_RAW			EFI_GUID(0xe43176d7, 0xb6e8, 0x4827,  0xb7, 0x84, 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)
 #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
@@ -580,15 +579,6 @@ struct efi_mem_range {
 	u64 attribute;
 };
 
-typedef struct {
-	u32 version;
-	u32 length;
-	u64 memory_protection_attribute;
-} efi_properties_table_t;
-
-#define EFI_PROPERTIES_TABLE_VERSION	0x00010000
-#define EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA	0x1
-
 typedef struct {
 	u16 version;
 	u16 length;
@@ -871,10 +861,9 @@ static inline int efi_range_is_wc(unsigned long start, unsigned long len)
 #define EFI_PARAVIRT		6	/* Access is via a paravirt interface */
 #define EFI_ARCH_1		7	/* First arch-specific bit */
 #define EFI_DBG			8	/* Print additional debug info at runtime */
-#define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
-#define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
-#define EFI_MEM_NO_SOFT_RESERVE	11	/* Is the kernel configured to ignore soft reservations? */
-#define EFI_PRESERVE_BS_REGIONS	12	/* Are EFI boot-services memory segments available? */
+#define EFI_MEM_ATTR		9	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
+#define EFI_MEM_NO_SOFT_RESERVE	10	/* Is the kernel configured to ignore soft reservations? */
+#define EFI_PRESERVE_BS_REGIONS	11	/* Are EFI boot-services memory segments available? */
 
 #ifdef CONFIG_EFI
 /*
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-12 18:52 [PATCH v2 1/2] x86/efi: Drop support for the EFI_PROPERTIES_TABLE Nicolas Saenz Julienne
@ 2024-11-12 18:52 ` Nicolas Saenz Julienne
  2024-11-15 16:39   ` Ard Biesheuvel
  2024-11-22 13:03   ` Dave Young
  0 siblings, 2 replies; 9+ messages in thread
From: Nicolas Saenz Julienne @ 2024-11-12 18:52 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H . Peter Anvin, Matt Fleming, linux-efi, linux-kernel, stanspas,
	nh-open-source, Nicolas Saenz Julienne, stable

Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
routine, kexec_enter_virtual_mode(), which replays the mappings made by
the original kernel. Unfortunately, that function fails to reinstate
EFI's memory attributes, which would've otherwise been set after
entering virtual mode. Remediate this by calling
efi_runtime_update_mappings() within kexec's routine.

Cc: stable@vger.kernel.org
Fixes: 18141e89a76c ("x86/efi: Add support for EFI_MEMORY_ATTRIBUTES_TABLE")
Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>

---

Notes:
- Tested with QEMU/OVMF.

 arch/x86/platform/efi/efi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 375ebd78296a..a7ff189421c3 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -765,6 +765,7 @@ static void __init kexec_enter_virtual_mode(void)
 
 	efi_sync_low_kernel_mappings();
 	efi_native_runtime_setup();
+	efi_runtime_update_mappings();
 #endif
 }
 
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-12 18:52 ` [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec Nicolas Saenz Julienne
@ 2024-11-15 16:39   ` Ard Biesheuvel
  2024-11-18 10:52     ` Nicolas Saenz Julienne
  2024-11-22 13:03   ` Dave Young
  1 sibling, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2024-11-15 16:39 UTC (permalink / raw)
  To: Nicolas Saenz Julienne
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H . Peter Anvin, Matt Fleming, linux-efi, linux-kernel, stanspas,
	nh-open-source, stable

On Tue, 12 Nov 2024 at 19:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>
> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
> routine, kexec_enter_virtual_mode(), which replays the mappings made by
> the original kernel. Unfortunately, that function fails to reinstate
> EFI's memory attributes, which would've otherwise been set after
> entering virtual mode. Remediate this by calling
> efi_runtime_update_mappings() within kexec's routine.
>
> Cc: stable@vger.kernel.org
> Fixes: 18141e89a76c ("x86/efi: Add support for EFI_MEMORY_ATTRIBUTES_TABLE")
> Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>
>
> ---
>
> Notes:
> - Tested with QEMU/OVMF.
>


I'll queue these up, but I am going drop the cc stable: the memory
attributes table is an overlay of the EFI memory map with restricted
permissions for EFI runtime services regions, which are only mapped
while a EFI runtime call is in progress.

So if the table is not taken into account after kexec, the runtime
code and data mappings will all be RWX but I think this is a situation
we can live with. If nothing breaks, we can always revisit this later
if there is an actual need.

Thanks,


>  arch/x86/platform/efi/efi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 375ebd78296a..a7ff189421c3 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -765,6 +765,7 @@ static void __init kexec_enter_virtual_mode(void)
>
>         efi_sync_low_kernel_mappings();
>         efi_native_runtime_setup();
> +       efi_runtime_update_mappings();
>  #endif
>  }
>
> --
> 2.40.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-15 16:39   ` Ard Biesheuvel
@ 2024-11-18 10:52     ` Nicolas Saenz Julienne
  0 siblings, 0 replies; 9+ messages in thread
From: Nicolas Saenz Julienne @ 2024-11-18 10:52 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H . Peter Anvin, Matt Fleming, linux-efi, linux-kernel, stanspas,
	nh-open-source, stable

On Fri Nov 15, 2024 at 4:39 PM UTC, Ard Biesheuvel wrote:
> On Tue, 12 Nov 2024 at 19:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>>
>> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
>> routine, kexec_enter_virtual_mode(), which replays the mappings made by
>> the original kernel. Unfortunately, that function fails to reinstate
>> EFI's memory attributes, which would've otherwise been set after
>> entering virtual mode. Remediate this by calling
>> efi_runtime_update_mappings() within kexec's routine.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 18141e89a76c ("x86/efi: Add support for EFI_MEMORY_ATTRIBUTES_TABLE")
>> Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>
>>
>> ---
>>
>> Notes:
>> - Tested with QEMU/OVMF.
>>
>
>
> I'll queue these up,

Thanks!

> but I am going drop the cc stable: the memory attributes table is an
> overlay of the EFI memory map with restricted permissions for EFI
> runtime services regions, which are only mapped while a EFI runtime
> call is in progress.
>
> So if the table is not taken into account after kexec, the runtime
> code and data mappings will all be RWX but I think this is a situation
> we can live with. If nothing breaks, we can always revisit this later
> if there is an actual need.

My intention was backporting the fix all the way to
'stable/linux-5.10.y'. But I'm happy to wait, or even to maintain an
internal backport. It's simple enough.

Nicolas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-12 18:52 ` [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec Nicolas Saenz Julienne
  2024-11-15 16:39   ` Ard Biesheuvel
@ 2024-11-22 13:03   ` Dave Young
  2024-11-28 15:58     ` Nicolas Saenz Julienne
  1 sibling, 1 reply; 9+ messages in thread
From: Dave Young @ 2024-11-22 13:03 UTC (permalink / raw)
  To: Nicolas Saenz Julienne
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Matt Fleming, linux-efi,
	linux-kernel, stanspas, nh-open-source, stable, kexec

Hi,

On Wed, 13 Nov 2024 at 02:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>
> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
> routine, kexec_enter_virtual_mode(), which replays the mappings made by
> the original kernel. Unfortunately, that function fails to reinstate
> EFI's memory attributes, which would've otherwise been set after
> entering virtual mode. Remediate this by calling
> efi_runtime_update_mappings() within kexec's routine.

In the function __map_region(), there are playing with the flags
similar to the efi_runtime_update_mappings though it looks a little
different.  Is this extra callback really necessary?

Have you seen a real bug happened?

>
> Cc: stable@vger.kernel.org
> Fixes: 18141e89a76c ("x86/efi: Add support for EFI_MEMORY_ATTRIBUTES_TABLE")
> Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>
>
> ---
>
> Notes:
> - Tested with QEMU/OVMF.
>
>  arch/x86/platform/efi/efi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 375ebd78296a..a7ff189421c3 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -765,6 +765,7 @@ static void __init kexec_enter_virtual_mode(void)
>
>         efi_sync_low_kernel_mappings();
>         efi_native_runtime_setup();
> +       efi_runtime_update_mappings();
>  #endif
>  }
>
> --
> 2.40.1
>
>
Thanks
Dave


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-22 13:03   ` Dave Young
@ 2024-11-28 15:58     ` Nicolas Saenz Julienne
  2024-11-29  7:11       ` Dave Young
  0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Saenz Julienne @ 2024-11-28 15:58 UTC (permalink / raw)
  To: Dave Young
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Matt Fleming, linux-efi,
	linux-kernel, stanspas, nh-open-source, stable, kexec

Hi Dave,

On Fri Nov 22, 2024 at 1:03 PM UTC, Dave Young wrote:
> On Wed, 13 Nov 2024 at 02:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>>
>> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
>> routine, kexec_enter_virtual_mode(), which replays the mappings made by
>> the original kernel. Unfortunately, that function fails to reinstate
>> EFI's memory attributes, which would've otherwise been set after
>> entering virtual mode. Remediate this by calling
>> efi_runtime_update_mappings() within kexec's routine.
>
> In the function __map_region(), there are playing with the flags
> similar to the efi_runtime_update_mappings though it looks a little
> different.  Is this extra callback really necessary?

EFI Memory attributes aren't tracked through
`/sys/firmware/efi/runtime-map`, and as such, whatever happens in
`__map_region()` after kexec will not honor them.

> Have you seen a real bug happened?

If lowered security posture after kexec counts as a bug, yes. The system
remains stable otherwise.

Nicolas

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-28 15:58     ` Nicolas Saenz Julienne
@ 2024-11-29  7:11       ` Dave Young
  2024-11-29  7:31         ` Dave Young
  0 siblings, 1 reply; 9+ messages in thread
From: Dave Young @ 2024-11-29  7:11 UTC (permalink / raw)
  To: Nicolas Saenz Julienne
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Matt Fleming, linux-efi,
	linux-kernel, stanspas, nh-open-source, stable, kexec

Hi Nicolas,

On Thu, 28 Nov 2024 at 23:58, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>
> Hi Dave,
>
> On Fri Nov 22, 2024 at 1:03 PM UTC, Dave Young wrote:
> > On Wed, 13 Nov 2024 at 02:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
> >>
> >> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
> >> routine, kexec_enter_virtual_mode(), which replays the mappings made by
> >> the original kernel. Unfortunately, that function fails to reinstate
> >> EFI's memory attributes, which would've otherwise been set after
> >> entering virtual mode. Remediate this by calling
> >> efi_runtime_update_mappings() within kexec's routine.
> >
> > In the function __map_region(), there are playing with the flags
> > similar to the efi_runtime_update_mappings though it looks a little
> > different.  Is this extra callback really necessary?
>
> EFI Memory attributes aren't tracked through
> `/sys/firmware/efi/runtime-map`, and as such, whatever happens in
> `__map_region()` after kexec will not honor them.

From the comment below the reason to do the mappings update is that
firmware could perform some fixups.  But for kexec case I think doing
the mapping correctly in the mapping code would be good enough.

        /*
         * Apply more restrictive page table mapping attributes now that
         * SVAM() has been called and the firmware has performed all
         * necessary relocation fixups for the new virtual addresses.
         */
        efi_runtime_update_mappings();

Otherwise /sys/firmware/efi/runtime-map is a copy for kexec-tools to
create the virtual efi memmap,  but I think the __map_region is called
after kexecing into the 2nd kernel, so I feel that at that time the
mem attr table should be usable.

Anyway thanks for explaining about this.  It is indeed something to
improve.  I have no strong opinion as your code will also work.


>
> > Have you seen a real bug happened?
>
> If lowered security posture after kexec counts as a bug, yes. The system
> remains stable otherwise.
>
> Nicolas
>

Thanks
Dave


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-29  7:11       ` Dave Young
@ 2024-11-29  7:31         ` Dave Young
  2024-11-29 17:03           ` Nicolas Saenz Julienne
  0 siblings, 1 reply; 9+ messages in thread
From: Dave Young @ 2024-11-29  7:31 UTC (permalink / raw)
  To: Nicolas Saenz Julienne
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Matt Fleming, linux-efi,
	linux-kernel, stanspas, nh-open-source, stable, kexec

On Fri, 29 Nov 2024 at 15:11, Dave Young <dyoung@redhat.com> wrote:
>
> Hi Nicolas,
>
> On Thu, 28 Nov 2024 at 23:58, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
> >
> > Hi Dave,
> >
> > On Fri Nov 22, 2024 at 1:03 PM UTC, Dave Young wrote:
> > > On Wed, 13 Nov 2024 at 02:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
> > >>
> > >> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
> > >> routine, kexec_enter_virtual_mode(), which replays the mappings made by
> > >> the original kernel. Unfortunately, that function fails to reinstate
> > >> EFI's memory attributes, which would've otherwise been set after
> > >> entering virtual mode. Remediate this by calling
> > >> efi_runtime_update_mappings() within kexec's routine.
> > >
> > > In the function __map_region(), there are playing with the flags
> > > similar to the efi_runtime_update_mappings though it looks a little
> > > different.  Is this extra callback really necessary?
> >
> > EFI Memory attributes aren't tracked through
> > `/sys/firmware/efi/runtime-map`, and as such, whatever happens in
> > `__map_region()` after kexec will not honor them.
>
> From the comment below the reason to do the mappings update is that
> firmware could perform some fixups.  But for kexec case I think doing
> the mapping correctly in the mapping code would be good enough.
>
>         /*
>          * Apply more restrictive page table mapping attributes now that
>          * SVAM() has been called and the firmware has performed all
>          * necessary relocation fixups for the new virtual addresses.
>          */
>         efi_runtime_update_mappings();
>
> Otherwise /sys/firmware/efi/runtime-map is a copy for kexec-tools to
> create the virtual efi memmap,  but I think the __map_region is called
> after kexecing into the 2nd kernel, so I feel that at that time the
> mem attr table should be usable.

Another thing I'm not sure why the updated mem attr is not stored in
the memmap md descriptor "attribute" field, if that is possible then
the runtime-map will carry them,  anyway, the __map_region still needs
tweaking to use the attribute.

>
> Anyway thanks for explaining about this.  It is indeed something to
> improve.  I have no strong opinion as your code will also work.
>
>
> >
> > > Have you seen a real bug happened?
> >
> > If lowered security posture after kexec counts as a bug, yes. The system
> > remains stable otherwise.
> >
> > Nicolas
> >
>
> Thanks
> Dave


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec
  2024-11-29  7:31         ` Dave Young
@ 2024-11-29 17:03           ` Nicolas Saenz Julienne
  0 siblings, 0 replies; 9+ messages in thread
From: Nicolas Saenz Julienne @ 2024-11-29 17:03 UTC (permalink / raw)
  To: Dave Young
  Cc: Ard Biesheuvel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Matt Fleming, linux-efi,
	linux-kernel, stanspas, nh-open-source, stable, kexec

On Fri Nov 29, 2024 at 7:31 AM UTC, Dave Young wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Fri, 29 Nov 2024 at 15:11, Dave Young <dyoung@redhat.com> wrote:
>>
>> Hi Nicolas,
>>
>> On Thu, 28 Nov 2024 at 23:58, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>> >
>> > Hi Dave,
>> >
>> > On Fri Nov 22, 2024 at 1:03 PM UTC, Dave Young wrote:
>> > > On Wed, 13 Nov 2024 at 02:53, Nicolas Saenz Julienne <nsaenz@amazon.com> wrote:
>> > >>
>> > >> Kexec bypasses EFI's switch to virtual mode. In exchange, it has its own
>> > >> routine, kexec_enter_virtual_mode(), which replays the mappings made by
>> > >> the original kernel. Unfortunately, that function fails to reinstate
>> > >> EFI's memory attributes, which would've otherwise been set after
>> > >> entering virtual mode. Remediate this by calling
>> > >> efi_runtime_update_mappings() within kexec's routine.
>> > >
>> > > In the function __map_region(), there are playing with the flags
>> > > similar to the efi_runtime_update_mappings though it looks a little
>> > > different.  Is this extra callback really necessary?
>> >
>> > EFI Memory attributes aren't tracked through
>> > `/sys/firmware/efi/runtime-map`, and as such, whatever happens in
>> > `__map_region()` after kexec will not honor them.
>>
>> From the comment below the reason to do the mappings update is that
>> firmware could perform some fixups.  But for kexec case I think doing
>> the mapping correctly in the mapping code would be good enough.
>>
>>         /*
>>          * Apply more restrictive page table mapping attributes now that
>>          * SVAM() has been called and the firmware has performed all
>>          * necessary relocation fixups for the new virtual addresses.
>>          */
>>         efi_runtime_update_mappings();
>>
>> Otherwise /sys/firmware/efi/runtime-map is a copy for kexec-tools to
>> create the virtual efi memmap,  but I think the __map_region is called
>> after kexecing into the 2nd kernel, so I feel that at that time the
>> mem attr table should be usable.
>
> Another thing I'm not sure why the updated mem attr is not stored in
> the memmap md descriptor "attribute" field, if that is possible then
> the runtime-map will carry them,  anyway, the __map_region still needs
> tweaking to use the attribute.

AFAIK there isn't a technical reason we why couldn't do it through the
runtime-map, but it's annoying to do so because EFI Memory Attributes
are allowed to segment EFI memory regions into smaller sections with
distinct attributes. We'd have to carefully update the kernel's
representation of the EFI runtime memory map as we apply the attributes
(the one that's ultimately used to populate
`/sys/firmware/efi/runtime-map`).

On the other hand, the config table and memory region that holds the
attributes is already being persisted through kexec, so using it is
straightforward.

Nicolas

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-11-29 17:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-12 18:52 [PATCH v2 1/2] x86/efi: Drop support for the EFI_PROPERTIES_TABLE Nicolas Saenz Julienne
2024-11-12 18:52 ` [PATCH v2 2/2] x86/efi: Apply EFI Memory Attributes after kexec Nicolas Saenz Julienne
2024-11-15 16:39   ` Ard Biesheuvel
2024-11-18 10:52     ` Nicolas Saenz Julienne
2024-11-22 13:03   ` Dave Young
2024-11-28 15:58     ` Nicolas Saenz Julienne
2024-11-29  7:11       ` Dave Young
2024-11-29  7:31         ` Dave Young
2024-11-29 17:03           ` Nicolas Saenz Julienne

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox