linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot.
@ 2018-11-04 20:10 Sai Praneeth Prakhya
  2018-11-04 20:10 ` [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services Sai Praneeth Prakhya
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Sai Praneeth Prakhya @ 2018-11-04 20:10 UTC (permalink / raw)
  To: linux-efi, linux-kernel, x86
  Cc: Sai Praneeth Prakhya, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Thomas Gleixner,
	Peter Zijlstra, Ard Biesheuvel

CC'ing x86 folks because this patch set touches x86/mm which I am no expert of.

Ideally, after kernel assumes control of the platform, firmware shouldn't access
EFI boot services code/data regions. But, it's noticed that this is not so true
in many x86 platforms. Hence, during boot, kernel reserves EFI boot services
code/data regions [1] and maps [2] them to efi_pgd so that call to
set_virtual_address_map() doesn't fail. After returning from
set_virtual_address_map(), kernel frees the reserved regions [3] but they still
remain mapped.

This means that any code that's running in efi_pgd address space (e.g: any EFI
runtime service) would still be able to access EFI boot services code/data
regions but the contents of these regions would have long been over written by
someone else as they are freed by efi_free_boot_services(). So, it's important
to unmap these regions. After unmapping EFI boot services code/data regions, any
illegal access by buggy firmware to these regions would result in page fault
which will be handled by efi specific fault handler.

Unmapping EFI boot services code/data regions will result in clearing
PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already handled
by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.

[1] Please see efi_reserve_boot_services()
[2] Please see efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd()
[3] Please see efi_free_boot_services()

Testing the patch set:
----------------------
1. Download buggy firmware (which accesses boot regions even after kernel has
booted) from here [1].
2. Without the patch set, you shouldn't see any kernel warning/error messages
(i.e. kernel allows accesses to EFI boot services code/data regions even after
call to set_virtual_address_map()).
3. With the patch set, you should see a kernel warning about buggy firmware,
efi_rts_wq beeing freezed and disabling runtime services forever.

Please note that this patch will change kernel's existing behavior for some EFI
runtime services but I think it's OK because kernel should have never allowed
those accesses in the first place.

Also please note that this patch set needs lot of real time trashing as I just
tested it out with OVMF.

Note:
-----
Patch set based on "next" branch in efi tree.

Changes from V2 -> V3:
----------------------
1. Expliclty set pfn to 0 in kernel_unmap_pages_in_pgd().
2. Add __init modifier to kernel_<map/unmap>_pages_in_pgd().
3. Warn if kernel_<map/unmap>_pages_in_pgd() are called after smp_init().
4. Split efi_unmap_pages() into a separate patch.

Changes from V1 -> V2:
----------------------
1. Rewrite the cpa initializer in a more readable fashion.
2. Don't use cpa->pfn while unmapping, as it's not useful.
3. Unmap regions before freeing them up.
4. Fix spelling nits.

Sai Praneeth (3):
  x86/mm/pageattr: Introduce helper function to unmap EFI boot services
  x86/efi: Unmap EFI boot services code/data regions from efi_pgd
  x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86

 arch/x86/include/asm/efi.h           |  2 ++
 arch/x86/include/asm/pgtable_types.h |  8 ++++++--
 arch/x86/mm/pageattr.c               | 40 ++++++++++++++++++++++++++++++++++--
 arch/x86/platform/efi/efi.c          |  2 ++
 arch/x86/platform/efi/quirks.c       | 25 ++++++++++++++++++++++
 include/linux/efi.h                  |  3 ---
 init/main.c                          |  4 ----
 7 files changed, 73 insertions(+), 11 deletions(-)

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

-- 
2.7.4


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

* [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services
  2018-11-04 20:10 [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Sai Praneeth Prakhya
@ 2018-11-04 20:10 ` Sai Praneeth Prakhya
  2018-11-04 20:17   ` Prakhya, Sai Praneeth
  2018-11-05 13:18   ` Thomas Gleixner
  2018-11-04 20:10 ` [PATCH V3 2/3] x86/efi: Unmap EFI boot services code/data regions from efi_pgd Sai Praneeth Prakhya
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 9+ messages in thread
From: Sai Praneeth Prakhya @ 2018-11-04 20:10 UTC (permalink / raw)
  To: linux-efi, linux-kernel, x86
  Cc: Sai Praneeth Prakhya, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Thomas Gleixner,
	Peter Zijlstra, Ard Biesheuvel

Ideally, after kernel assumes control of the platform, firmware
shouldn't access EFI boot services code/data regions. But, it's noticed
that this is not so true in many x86 platforms. Hence, during boot,
kernel reserves EFI boot services code/data regions [1] and maps [2]
them to efi_pgd so that call to set_virtual_address_map() doesn't fail.
After returning from set_virtual_address_map(), kernel frees the
reserved regions [3] but they still remain mapped. Hence, introduce
kernel_unmap_pages_in_pgd() which will later be used to unmap EFI boot
services code/data regions.

While at it modify kernel_map_pages_in_pgd() by
1. Adding __init modifier because it's always used *only* during boot.
2. Add a warning if it's used after SMP is initialized because it uses
   __flush_tlb_all() which flushes mappings only on current CPU.

Unmapping EFI boot services code/data regions will result in clearing
PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already
handled by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.

[1] efi_reserve_boot_services()
[2] efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd()
[3] efi_free_boot_services()

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/include/asm/pgtable_types.h |  8 ++++++--
 arch/x86/mm/pageattr.c               | 40 ++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index b64acb08a62b..79aa79bb2cfa 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -564,8 +564,12 @@ extern pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long address,
 				    unsigned int *level);
 extern pmd_t *lookup_pmd_address(unsigned long address);
 extern phys_addr_t slow_virt_to_phys(void *__address);
-extern int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
-				   unsigned numpages, unsigned long page_flags);
+extern int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn,
+					  unsigned long address,
+					  unsigned numpages,
+					  unsigned long page_flags);
+extern int __init kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
+					    unsigned long numpages);
 #endif	/* !__ASSEMBLY__ */
 
 #endif /* _ASM_X86_PGTABLE_DEFS_H */
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 51a5a69ecac9..1b1d5a68c4b2 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -2111,8 +2111,8 @@ bool kernel_page_present(struct page *page)
 
 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
-int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
-			    unsigned numpages, unsigned long page_flags)
+int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
+				   unsigned numpages, unsigned long page_flags)
 {
 	int retval = -EINVAL;
 
@@ -2126,6 +2126,8 @@ int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
 		.flags = 0,
 	};
 
+	WARN_ONCE(num_online_cpus() > 1, "Don't call after initializing SMP");
+
 	if (!(__supported_pte_mask & _PAGE_NX))
 		goto out;
 
@@ -2148,6 +2150,40 @@ int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
 }
 
 /*
+ * __flush_tlb_all() flushes mappings only on current CPU and hence this
+ * function shouldn't be used in an SMP environment. Presently, it's used only
+ * during boot (way before smp_init()) by EFI subsystem and hence is ok.
+ */
+int __init kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
+				     unsigned long numpages)
+{
+	int retval;
+
+	/*
+	 * The typical sequence for unmapping is to find a pte through
+	 * lookup_address_in_pgd() (ideally, it should never return NULL because
+	 * the address is already mapped) and change it's protections. As pfn is
+	 * the *target* of a mapping, it's not useful while unmapping.
+	 */
+	struct cpa_data cpa = {
+		.vaddr		= &address,
+		.pfn		= 0,
+		.pgd		= pgd,
+		.numpages	= numpages,
+		.mask_set	= __pgprot(0),
+		.mask_clr	= __pgprot(_PAGE_PRESENT | _PAGE_RW),
+		.flags		= 0,
+	};
+
+	WARN_ONCE(num_online_cpus() > 1, "Don't call after initializing SMP");
+
+	retval = __change_page_attr_set_clr(&cpa, 0);
+	__flush_tlb_all();
+
+	return retval;
+}
+
+/*
  * The testcases use internal knowledge of the implementation that shouldn't
  * be exposed to the rest of the kernel. Include these directly here.
  */
-- 
2.7.4


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

* [PATCH V3 2/3] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
  2018-11-04 20:10 [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Sai Praneeth Prakhya
  2018-11-04 20:10 ` [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services Sai Praneeth Prakhya
@ 2018-11-04 20:10 ` Sai Praneeth Prakhya
  2018-11-04 20:10 ` [PATCH V3 3/3] x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86 Sai Praneeth Prakhya
  2018-11-05 12:47 ` [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Ard Biesheuvel
  3 siblings, 0 replies; 9+ messages in thread
From: Sai Praneeth Prakhya @ 2018-11-04 20:10 UTC (permalink / raw)
  To: linux-efi, linux-kernel, x86
  Cc: Sai Praneeth Prakhya, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Thomas Gleixner,
	Peter Zijlstra, Ard Biesheuvel

efi_free_boot_services(), as the name suggests, frees EFI boot services
code/data regions but forgets to unmap these regions from efi_pgd. This
means that any code that's running in efi_pgd address space (e.g:
any EFI runtime service) would still be able to access these regions but
the contents of these regions would have long been over written by
someone else. So, it's important to unmap these regions. Hence,
introduce efi_unmap_pages() to unmap these regions from efi_pgd.

After unmapping EFI boot services code/data regions, any illegal access
by buggy firmware to these regions would result in page fault which will
be handled by EFI specific fault handler.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/platform/efi/quirks.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 669babcaf245..fb1c44b11235 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -370,6 +370,24 @@ void __init efi_reserve_boot_services(void)
 	}
 }
 
+/*
+ * Apart from having VA mappings for EFI boot services code/data regions,
+ * (duplicate) 1:1 mappings were also created as a quirk for buggy firmware. So,
+ * unmap both 1:1 and VA mappings.
+ */
+static void __init efi_unmap_pages(efi_memory_desc_t *md)
+{
+	pgd_t *pgd = efi_mm.pgd;
+	u64 pa = md->phys_addr;
+	u64 va = md->virt_addr;
+
+	if (kernel_unmap_pages_in_pgd(pgd, pa, md->num_pages))
+		pr_err("Failed to unmap 1:1 mapping for 0x%llx\n", pa);
+
+	if (kernel_unmap_pages_in_pgd(pgd, va, md->num_pages))
+		pr_err("Failed to unmap VA mapping for 0x%llx\n", va);
+}
+
 void __init efi_free_boot_services(void)
 {
 	phys_addr_t new_phys, new_size;
@@ -395,6 +413,13 @@ void __init efi_free_boot_services(void)
 		}
 
 		/*
+		 * Before calling set_virtual_address_map(), EFI boot services
+		 * code/data regions were mapped as a quirk for buggy firmware.
+		 * Unmap them from efi_pgd before freeing them up.
+		 */
+		efi_unmap_pages(md);
+
+		/*
 		 * Nasty quirk: if all sub-1MB memory is used for boot
 		 * services, we can get here without having allocated the
 		 * real mode trampoline.  It's too late to hand boot services
-- 
2.7.4


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

* [PATCH V3 3/3] x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86
  2018-11-04 20:10 [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Sai Praneeth Prakhya
  2018-11-04 20:10 ` [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services Sai Praneeth Prakhya
  2018-11-04 20:10 ` [PATCH V3 2/3] x86/efi: Unmap EFI boot services code/data regions from efi_pgd Sai Praneeth Prakhya
@ 2018-11-04 20:10 ` Sai Praneeth Prakhya
  2018-11-05 12:47 ` [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Ard Biesheuvel
  3 siblings, 0 replies; 9+ messages in thread
From: Sai Praneeth Prakhya @ 2018-11-04 20:10 UTC (permalink / raw)
  To: linux-efi, linux-kernel, x86
  Cc: Sai Praneeth Prakhya, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Thomas Gleixner,
	Peter Zijlstra, Ard Biesheuvel

efi_<reserve/free>_boot_services() are x86 specific quirks and as such
should be in asm/efi.h, so move them from linux/efi.h. Also, call
efi_free_boot_services() from __efi_enter_virtual_mode() as it is x86
specific call and ideally shouldn't be part of init/main.c

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/include/asm/efi.h  | 2 ++
 arch/x86/platform/efi/efi.c | 2 ++
 include/linux/efi.h         | 3 ---
 init/main.c                 | 4 ----
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index eea40d52ca78..d1e64ac80b9c 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -141,6 +141,8 @@ extern int __init efi_reuse_config(u64 tables, int nr_tables);
 extern void efi_delete_dummy_variable(void);
 extern void efi_switch_mm(struct mm_struct *mm);
 extern void efi_recover_from_page_fault(unsigned long phys_addr);
+extern void efi_free_boot_services(void);
+extern void efi_reserve_boot_services(void);
 
 struct efi_setup_data {
 	u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 9061babfbc83..93924a353e3b 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -994,6 +994,8 @@ static void __init __efi_enter_virtual_mode(void)
 		panic("EFI call to SetVirtualAddressMap() failed!");
 	}
 
+	efi_free_boot_services();
+
 	/*
 	 * Now that EFI is in virtual mode, update the function
 	 * pointers in the runtime service table to the new virtual addresses.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 845174e113ce..ed2058073385 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1000,13 +1000,11 @@ extern void efi_memmap_walk (efi_freemem_callback_t callback, void *arg);
 extern void efi_gettimeofday (struct timespec64 *ts);
 extern void efi_enter_virtual_mode (void);	/* switch EFI to virtual mode, if possible */
 #ifdef CONFIG_X86
-extern void efi_free_boot_services(void);
 extern efi_status_t efi_query_variable_store(u32 attributes,
 					     unsigned long size,
 					     bool nonblocking);
 extern void efi_find_mirror(void);
 #else
-static inline void efi_free_boot_services(void) {}
 
 static inline efi_status_t efi_query_variable_store(u32 attributes,
 						    unsigned long size,
@@ -1046,7 +1044,6 @@ extern void efi_mem_reserve(phys_addr_t addr, u64 size);
 extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
-extern void efi_reserve_boot_services(void);
 extern int efi_get_fdt_params(struct efi_fdt_params *params);
 extern struct kobject *efi_kobj;
 
diff --git a/init/main.c b/init/main.c
index 18f8f0140fa0..174fb14196cc 100644
--- a/init/main.c
+++ b/init/main.c
@@ -731,10 +731,6 @@ asmlinkage __visible void __init start_kernel(void)
 	arch_post_acpi_subsys_init();
 	sfi_init_late();
 
-	if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-		efi_free_boot_services();
-	}
-
 	/* Do the rest non-__init'ed, we're now alive */
 	rest_init();
 }
-- 
2.7.4


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

* RE: [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services
  2018-11-04 20:10 ` [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services Sai Praneeth Prakhya
@ 2018-11-04 20:17   ` Prakhya, Sai Praneeth
  2018-11-05 13:18   ` Thomas Gleixner
  1 sibling, 0 replies; 9+ messages in thread
From: Prakhya, Sai Praneeth @ 2018-11-04 20:17 UTC (permalink / raw)
  To: linux-efi, linux-kernel, x86
  Cc: Borislav Petkov, Ingo Molnar, Andy Lutomirski, Hansen, Dave,
	Bhupesh Sharma, Thomas Gleixner, Peter Zijlstra, Ard Biesheuvel

> Ideally, after kernel assumes control of the platform, firmware shouldn't access
> EFI boot services code/data regions. But, it's noticed that this is not so true in
> many x86 platforms. Hence, during boot, kernel reserves EFI boot services
> code/data regions [1] and maps [2] them to efi_pgd so that call to
> set_virtual_address_map() doesn't fail.
> After returning from set_virtual_address_map(), kernel frees the reserved
> regions [3] but they still remain mapped. Hence, introduce
> kernel_unmap_pages_in_pgd() which will later be used to unmap EFI boot
> services code/data regions.
> 
> While at it modify kernel_map_pages_in_pgd() by 1. Adding __init modifier
> because it's always used *only* during boot.
> 2. Add a warning if it's used after SMP is initialized because it uses
>    __flush_tlb_all() which flushes mappings only on current CPU.
> 
> Unmapping EFI boot services code/data regions will result in clearing
> PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already
> handled by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.
> 
> [1] efi_reserve_boot_services()
> [2] efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd() [3]
> efi_free_boot_services()
> 
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Bhupesh Sharma <bhsharma@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Wasn't sure (and hence didn't) if I should add Acked-by Ingo because this patch changed from V2.

Regards,
Sai

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

* Re: [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot.
  2018-11-04 20:10 [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Sai Praneeth Prakhya
                   ` (2 preceding siblings ...)
  2018-11-04 20:10 ` [PATCH V3 3/3] x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86 Sai Praneeth Prakhya
@ 2018-11-05 12:47 ` Ard Biesheuvel
  2018-11-05 13:20   ` Thomas Gleixner
  3 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2018-11-05 12:47 UTC (permalink / raw)
  To: Sai Praneeth Prakhya
  Cc: linux-efi, Linux Kernel Mailing List, the arch/x86 maintainers,
	Borislav Petkov, Ingo Molnar, Andy Lutomirski, Dave Hansen,
	Bhupesh Sharma, Thomas Gleixner, Peter Zijlstra

On 4 November 2018 at 21:10, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.com> wrote:
> CC'ing x86 folks because this patch set touches x86/mm which I am no expert of.
>
> Ideally, after kernel assumes control of the platform, firmware shouldn't access
> EFI boot services code/data regions. But, it's noticed that this is not so true
> in many x86 platforms. Hence, during boot, kernel reserves EFI boot services
> code/data regions [1] and maps [2] them to efi_pgd so that call to
> set_virtual_address_map() doesn't fail. After returning from
> set_virtual_address_map(), kernel frees the reserved regions [3] but they still
> remain mapped.
>
> This means that any code that's running in efi_pgd address space (e.g: any EFI
> runtime service) would still be able to access EFI boot services code/data
> regions but the contents of these regions would have long been over written by
> someone else as they are freed by efi_free_boot_services(). So, it's important
> to unmap these regions. After unmapping EFI boot services code/data regions, any
> illegal access by buggy firmware to these regions would result in page fault
> which will be handled by efi specific fault handler.
>
> Unmapping EFI boot services code/data regions will result in clearing
> PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already handled
> by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.
>
> [1] Please see efi_reserve_boot_services()
> [2] Please see efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd()
> [3] Please see efi_free_boot_services()
>
> Testing the patch set:
> ----------------------
> 1. Download buggy firmware (which accesses boot regions even after kernel has
> booted) from here [1].
> 2. Without the patch set, you shouldn't see any kernel warning/error messages
> (i.e. kernel allows accesses to EFI boot services code/data regions even after
> call to set_virtual_address_map()).
> 3. With the patch set, you should see a kernel warning about buggy firmware,
> efi_rts_wq beeing freezed and disabling runtime services forever.
>
> Please note that this patch will change kernel's existing behavior for some EFI
> runtime services but I think it's OK because kernel should have never allowed
> those accesses in the first place.
>
> Also please note that this patch set needs lot of real time trashing as I just
> tested it out with OVMF.
>
> Note:
> -----
> Patch set based on "next" branch in efi tree.
>
> Changes from V2 -> V3:
> ----------------------
> 1. Expliclty set pfn to 0 in kernel_unmap_pages_in_pgd().
> 2. Add __init modifier to kernel_<map/unmap>_pages_in_pgd().
> 3. Warn if kernel_<map/unmap>_pages_in_pgd() are called after smp_init().
> 4. Split efi_unmap_pages() into a separate patch.
>
> Changes from V1 -> V2:
> ----------------------
> 1. Rewrite the cpa initializer in a more readable fashion.
> 2. Don't use cpa->pfn while unmapping, as it's not useful.
> 3. Unmap regions before freeing them up.
> 4. Fix spelling nits.
>
> Sai Praneeth (3):
>   x86/mm/pageattr: Introduce helper function to unmap EFI boot services
>   x86/efi: Unmap EFI boot services code/data regions from efi_pgd
>   x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86
>
>  arch/x86/include/asm/efi.h           |  2 ++
>  arch/x86/include/asm/pgtable_types.h |  8 ++++++--
>  arch/x86/mm/pageattr.c               | 40 ++++++++++++++++++++++++++++++++++--
>  arch/x86/platform/efi/efi.c          |  2 ++
>  arch/x86/platform/efi/quirks.c       | 25 ++++++++++++++++++++++
>  include/linux/efi.h                  |  3 ---
>  init/main.c                          |  4 ----
>  7 files changed, 73 insertions(+), 11 deletions(-)
>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Bhupesh Sharma <bhsharma@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>

Thanks Sai.

I have queued these up in efi/next for now, but I'd still like a fresh
ack from the x86 maintainers.

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

* Re: [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services
  2018-11-04 20:10 ` [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services Sai Praneeth Prakhya
  2018-11-04 20:17   ` Prakhya, Sai Praneeth
@ 2018-11-05 13:18   ` Thomas Gleixner
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Gleixner @ 2018-11-05 13:18 UTC (permalink / raw)
  To: Sai Praneeth Prakhya
  Cc: linux-efi, linux-kernel, x86, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Peter Zijlstra,
	Ard Biesheuvel

On Sun, 4 Nov 2018, Sai Praneeth Prakhya wrote:

> Ideally, after kernel assumes control of the platform, firmware
> shouldn't access EFI boot services code/data regions. But, it's noticed
> that this is not so true in many x86 platforms. Hence, during boot,
> kernel reserves EFI boot services code/data regions [1] and maps [2]
> them to efi_pgd so that call to set_virtual_address_map() doesn't fail.
> After returning from set_virtual_address_map(), kernel frees the
> reserved regions [3] but they still remain mapped. Hence, introduce
> kernel_unmap_pages_in_pgd() which will later be used to unmap EFI boot
> services code/data regions.
> 
> While at it modify kernel_map_pages_in_pgd() by
> 1. Adding __init modifier because it's always used *only* during boot.
> 2. Add a warning if it's used after SMP is initialized because it uses
>    __flush_tlb_all() which flushes mappings only on current CPU.
> 
> Unmapping EFI boot services code/data regions will result in clearing
> PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already
> handled by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.
> 
> [1] efi_reserve_boot_services()
> [2] efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd()
> [3] efi_free_boot_services()
> 
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Bhupesh Sharma <bhsharma@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

* Re: [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot.
  2018-11-05 12:47 ` [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Ard Biesheuvel
@ 2018-11-05 13:20   ` Thomas Gleixner
  2018-11-05 13:20     ` Ard Biesheuvel
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2018-11-05 13:20 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Sai Praneeth Prakhya, linux-efi, Linux Kernel Mailing List,
	the arch/x86 maintainers, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Peter Zijlstra

On Mon, 5 Nov 2018, Ard Biesheuvel wrote:
> On 4 November 2018 at 21:10, Sai Praneeth Prakhya
> <sai.praneeth.prakhya@intel.com> wrote:
> Thanks Sai.
> 
> I have queued these up in efi/next for now, but I'd still like a fresh
> ack from the x86 maintainers.

I've reviewed the CPA part and for the rest feel free to add

Acked-by: Thomas Gleixner <tglx@linutronix.de>


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

* Re: [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot.
  2018-11-05 13:20   ` Thomas Gleixner
@ 2018-11-05 13:20     ` Ard Biesheuvel
  0 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2018-11-05 13:20 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Sai Praneeth Prakhya, linux-efi, Linux Kernel Mailing List,
	the arch/x86 maintainers, Borislav Petkov, Ingo Molnar,
	Andy Lutomirski, Dave Hansen, Bhupesh Sharma, Peter Zijlstra

On 5 November 2018 at 14:20, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 5 Nov 2018, Ard Biesheuvel wrote:
>> On 4 November 2018 at 21:10, Sai Praneeth Prakhya
>> <sai.praneeth.prakhya@intel.com> wrote:
>> Thanks Sai.
>>
>> I have queued these up in efi/next for now, but I'd still like a fresh
>> ack from the x86 maintainers.
>
> I've reviewed the CPA part and for the rest feel free to add
>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
>

Thanks Thomas

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

end of thread, other threads:[~2018-11-05 13:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-04 20:10 [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Sai Praneeth Prakhya
2018-11-04 20:10 ` [PATCH V3 1/3] x86/mm/pageattr: Introduce helper function to unmap EFI boot services Sai Praneeth Prakhya
2018-11-04 20:17   ` Prakhya, Sai Praneeth
2018-11-05 13:18   ` Thomas Gleixner
2018-11-04 20:10 ` [PATCH V3 2/3] x86/efi: Unmap EFI boot services code/data regions from efi_pgd Sai Praneeth Prakhya
2018-11-04 20:10 ` [PATCH V3 3/3] x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86 Sai Praneeth Prakhya
2018-11-05 12:47 ` [PATCH V3 0/3] Unmap EFI boot services code/data regions after boot Ard Biesheuvel
2018-11-05 13:20   ` Thomas Gleixner
2018-11-05 13:20     ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).