x86: fix some bugs about EFI runtime code mapping

This patch fixes some bugs of making EFI runtime code executable.

- Use change_page_attr in i386 too. Because the runtime code may be mapped not through ioremap.

- If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr is not called.

- Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime code may be mapped through efi_ioremap.

Signed-off-by Huang Ying <[email protected]>

---
 arch/x86/kernel/efi.c    |   35 ++++++++++++++++++++++++++++++-----
 arch/x86/kernel/efi_64.c |   26 ++++++--------------------
 include/asm-x86/efi.h    |    1 -
 3 files changed, 36 insertions(+), 26 deletions(-)

arch/x86/kernel/efi.c +2 -0

@@ -40,6 +40,8 @@
 #include <asm/setup.h>
 #include <asm/efi.h>
 #include <asm/time.h>
+#include <asm/cacheflush.h>
+#include <asm/tlbflush.h>
 
 #define EFI_DEBUG	1
 #define PFX 		"EFI: "

@@ -379,6 +381,32 @@ void __init efi_init(void)
 #endif
 }
 

Quoted 6 lines

+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)

arch/x86/kernel/efi.c +5 -0

+static void __init runtime_code_page_mkexec(void)
+{
+	efi_memory_desc_t *md;
+	unsigned long end;
+	void *p;

arch/x86/kernel/efi.c void __init efi_init(void) · line 392

Quoted 4 lines

arch/x86/kernel/efi.c +3 -0

+
+	if (!(__supported_pte_mask & _PAGE_NX))
+		return;

On 32-bit non-PAE, _PAGE_NX == 0, so this if() statement should be sufficient to disable the whole function at compile time without needing the outer #if defined wrapper.

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 6 lines

+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)

arch/x86/kernel/efi.c +5 -0

+static void __init runtime_code_page_mkexec(void)
+{
+	efi_memory_desc_t *md;
+	unsigned long end;
+	void *p;

Quoted 8 lines

arch/x86/kernel/efi.c void __init efi_init(void) · line 392

Quoted 4 lines

arch/x86/kernel/efi.c +3 -0

+
+	if (!(__supported_pte_mask & _PAGE_NX))
+		return;

On 32-bit non-PAE, _PAGE_NX == 0, so this if() statement should be sufficient to disable the whole function at compile time without needing the outer #if defined wrapper.

good point. The patch fixes bugs and the consolidation it does is very nice so i've applied it already, but we could indeed further consolidate it and make it a nice #ifdef-less function. Could one of you send an add-on patch for this?

This patch fixes some bugs of making EFI runtime code executable.

- Use change_page_attr in i386 too. Because the runtime code may be mapped not through ioremap.

- If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr is not called.

- Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime code may be mapped through efi_ioremap.

Quoted 10 lines · Huang, Ying

This patch fixes some bugs of making EFI runtime code executable.

- Use change_page_attr in i386 too. Because the runtime code may be mapped not through ioremap.

- If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr is not called.

- Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime code may be mapped through efi_ioremap.

thanks, applied.

note that here:

Review

Quoted 4 lines

(quoted diff) +2 -2

-		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
-				   offset);
+		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
+			     offset, PAGE_KERNEL_EXEC);

you've changed it from nocache-noexec to cached-exec. I suspect that's what we want - except if an early EFI area can be non-prefetchable device memory. Can that ever happen? Would you like to have PAGE_KERNEL_NOCACHE_EXEC perhaps? I implemented that yesterday but did not commit it yet. (see the patch below)

Quoted 17 lines · Ingo Molnar

Quoted 10 lines · Huang, Ying

This patch fixes some bugs of making EFI runtime code executable.

- Use change_page_attr in i386 too. Because the runtime code may be mapped not through ioremap.

- If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr is not called.

- Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime code may be mapped through efi_ioremap.

thanks, applied.

note that here:

Quoted 10 lines

Review

Quoted 4 lines

(quoted diff) +2 -2

-		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
-				   offset);
+		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
+			     offset, PAGE_KERNEL_EXEC);

you've changed it from nocache-noexec to cached-exec. I suspect that's what we want - except if an early EFI area can be non-prefetchable device memory. Can that ever happen? Would you like to have PAGE_KERNEL_NOCACHE_EXEC perhaps? I implemented that yesterday but did not commit it yet. (see the patch below)

Yes. EFI area can be non-prefetchable device memory. I should use PAGE_KERNEL_NOCACHE_EXEC.

A question about this:

The MTRR on x86 should have set the memory area as un-cachable. Why do we bother to set it in page table?