Skip to contentSkip to conversation

efi/x86: righten memory protections at runtime

PATCH New Ard Biesheuvel 1 participant 4 messages Dec 27, 2019

Subsystems (3)
lore
Share & tools
lore

Ard Biesheuvel

author maintainer
#0 lore raw

cover efi/x86: righten memory protections at runtime

For historical reasons, the EFI startup code uses R/W/X mappings for most memory regions that it maps, and in the mixed mode case, it even maps all of DRAM R/W/X in its 1:1 mapping.

Let's tighten this a bit, and use the NX bit where possible, and ensure that at least the kernel text+rodata are not mapped RWX in the mixed mode case.

Cc Andy Lutomirski <[email protected]>
Cc Ingo Molnar <[email protected]>

Ard Biesheuvel (3):
x86/mm: fix NX bit clearing issue in kernel_map_pages_in_pgd efi/x86: don't map the entire kernel text RW for mixed mode efi/x86: avoid RWX mappings for all of DRAM

 arch/x86/mm/pageattr.c         |  8 +-------
 arch/x86/platform/efi/efi_64.c | 21 ++++++++++++++------
 2 files changed, 16 insertions(+), 13 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH 1/3 x86/mm: fix NX bit clearing issue in kernel_map_pages_in_pgd

Commit 15f003d20782 ("x86/mm/pat: Don't implicitly allow _PAGE_RW in kernel_map_pages_in_pgd()") modified kernel_map_pages_in_pgd() to manage writable permissions of memory mappings in the EFI page table in a different way, but in the process, it removed the ability to clear NX attributes from read-only mappings, by clobbering the clear mask if _PAGE_RW is not being requested.

Failure to remove the NX attribute from read-only mappings is unlikely to be a security issue, but it does prevent us from tightening the permissions in the EFI page tables going forward, so let's fix it now.

Fixes 15f003d20782 ("x86/mm/pat: Don't implicitly allow _PAGE_RW in kernel_map_pages_in_pgd()
---
 arch/x86/mm/pageattr.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
arch/x86/mm/pageattr.c +1 -7
index 1b99ad05b117..f42780ba0893 100644
@@ -2215,7 +2215,7 @@ int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
 		.pgd = pgd,
 		.numpages = numpages,
 		.mask_set = __pgprot(0),
-		.mask_clr = __pgprot(0),
+		.mask_clr = __pgprot(~page_flags & (_PAGE_NX|_PAGE_RW)),
 		.flags = 0,
 	};
 
@@ -2224,12 +2224,6 @@ int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
 	if (!(__supported_pte_mask & _PAGE_NX))
 		goto out;
 
-	if (!(page_flags & _PAGE_NX))
-		cpa.mask_clr = __pgprot(_PAGE_NX);
-
-	if (!(page_flags & _PAGE_RW))
-		cpa.mask_clr = __pgprot(_PAGE_RW);
-
 	if (!(page_flags & _PAGE_ENC))
 		cpa.mask_clr = pgprot_encrypted(cpa.mask_clr);
 
Signature · 1 line
2.17.1
author maintainer
#2 lore raw

PATCH 2/3 efi/x86: don't map the entire kernel text RW for mixed mode

In reply to Ard Biesheuvel

The mixed mode thunking routine requires a part of it to be mapped 1:1, and for this reason, we currently map the entire kernel .text read/write in the EFI page tables, which is bad.

In fact, the kernel_map_pages_in_pgd() invocation that installs this mapping is entirely redundant, since all of DRAM is already 1:1 mapped read/write in the EFI page tables when we reach this point, which means that .rodata is mapped read-write as well.

So let's remap both .text and .rodata read-only in the EFI page tables.

---
 arch/x86/platform/efi/efi_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/platform/efi/efi_64.c +2 -2
index 5cb081107d10..9e9a4b31f74b 100644
@@ -402,11 +402,11 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	efi_scratch.phys_stack = virt_to_phys(page_address(page));
 	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
 
-	npages = (_etext - _text) >> PAGE_SHIFT;
+	npages = (__end_rodata_aligned - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
 	pfn = text >> PAGE_SHIFT;
 
-	pf = _PAGE_RW | _PAGE_ENC;
+	pf = _PAGE_ENC;
 	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, pf)) {
 		pr_err("Failed to map kernel text 1:1\n");
 		return 1;
Signature · 1 line
2.17.1
author maintainer
#3 lore raw

PATCH 3/3 efi/x86: avoid RWX mappings for all of DRAM

In reply to Ard Biesheuvel

The EFI code creates RWX mappings for all memory regions that are occupied after the stub completes, and in the mixed mode case, it even creates RWX mappings for all of the remaining DRAM as well.

Let's try to avoid this, by setting the NX bit for all memory regions except the ones that are marked as EFI runtime services code [which means text+rodata+data in practice, so we cannot mark them read-only right away]

---
 arch/x86/platform/efi/efi_64.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
arch/x86/platform/efi/efi_64.c +13 -4
index 9e9a4b31f74b..efdad1052457 100644
@@ -377,10 +377,6 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	 * as trim_bios_range() will reserve the first page and isolate it away
 	 * from memory allocators anyway.
 	 */
-	pf = _PAGE_RW;
-	if (sev_active())
-		pf |= _PAGE_ENC;
-
 	if (kernel_map_pages_in_pgd(pgd, 0x0, 0x0, 1, pf)) {
 		pr_err("Failed to create 1:1 mapping for the first page!\n");
 		return 1;
@@ -421,6 +417,19 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va)
 	unsigned long pfn;
 	pgd_t *pgd = efi_mm.pgd;
 
+	/*
+	 * EFI_RUNTIME_SERVICES_CODE regions typically cover PE/COFF
+	 * executable images in memory that consist of both R-X and
+	 * RW- sections, so we cannot apply read-only or non-exec
+	 * permissions just yet. However, modern EFI systems provide
+	 * a memory attributes table that describes those sections
+	 * with the appropriate restricted permissions, which are
+	 * applied in efi_runtime_update_mappings() below. All other
+	 * regions can be mapped non-executable at this point.
+	 */
+	if (md->type != EFI_RUNTIME_SERVICES_CODE)
+		flags |= _PAGE_NX;
+
 	if (!(md->attribute & EFI_MEMORY_WB))
 		flags |= _PAGE_PCD;
 
Signature · 1 line
2.17.1