linux-kernel - Re: [PATCH 3/3] Update efi_thunk to use efi_call_virt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 2 Jun 2016 21:19:49 +0100
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Alex Thorlton <athorlton@....com>
Cc:	linux-kernel@...r.kernel.org, Borislav Petkov <bp@...e.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Mike Travis <travis@....com>,
	Russ Anderson <rja@....com>,
	Dimitri Sivanich <sivanich@....com>, x86@...nel.org,
	linux-efi@...r.kernel.org
Subject: Re: [PATCH 3/3] Update efi_thunk to use efi_call_virt_generic

On Wed, 18 May, at 02:11:41PM, Alex Thorlton wrote:
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index f310f0b..6643f9b 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -68,6 +68,52 @@ struct efi_scratch {
>  	u64	phys_stack;
>  } __packed;
>  
> +#ifdef CONFIG_EFI_MIXED
> +extern efi_status_t efi64_thunk(u32, ...);
> +
> +#define runtime_service32(func)						 \
> +({									 \
> +	u32 table = (u32)(unsigned long)efi.systab;			 \
> +	u32 *rt, *___f;							 \
> +									 \
> +	rt = (u32 *)(table + offsetof(efi_system_table_32_t, runtime));	 \
> +	___f = (u32 *)(*rt + offsetof(efi_runtime_services_32_t, func)); \
> +	*___f;								 \
> +})
> +
> +/*
> + * Switch to the EFI page tables early so that we can access the 1:1
> + * runtime services mappings which are not mapped in any other page
> + * tables. This function must be called before runtime_service32().
> + *
> + * Also, disable interrupts because the IDT points to 64-bit handlers,
> + * which aren't going to function correctly when we switch to 32-bit.
> + */
> +#define arch_efi_call_virt_setup()					\
> +({									\
> +	efi_sync_low_kernel_mappings();					\
> +	local_irq_save(flags);						\
> +									\
> +	efi_scratch.prev_cr3 = read_cr3();				\
> +	write_cr3((unsigned long)efi_scratch.efi_pgt);			\
> +	__flush_tlb_all();						\
> +})
> +
> +#define arch_efi_call_virt(p, f, ...)					\
> +({									\
> +	u32 func = runtime_service32(f);				\
> +	efi64_thunk(func, __VA_ARGS__);					\
> +})
> +

This isn't correct because you're turning the runtime decision of
whether we're executing the thunking code into a build time one.

Users can enable CONFIG_EFI_MIXED in their builds but never actually
run that kernel on a mixed mode machine. One of the original design
intentions behind CONFIG_EFI_MIXED was that you can (and should!) turn
it on because it has no effect unless you run it on a machine with
32-bit EFI.

The switch to the thunk layer is done in efi_thunk_runtime_setup().

As a real world example of this, the openSUSE x86_64 kernel config has
CONFIG_EFI_MIXED enabled out of the box.

The thunk code should be able to reuse the regular x86_64
arch_efi_call_virt_setup() and arch_efi_call_virt_teardown(), since,

  a. We can also disable preemption without issue
  b. We can disable/reenable interrupts around those existing wrappers
  c. The "if (efi_scratch.use_pgd)" check is missing because we
     *always* use the EFI pgtables for mixed mode, it's a requirement

Would something like this work instead? It's not as neat as your
suggestion but it's a damn sight better than what we have today.

---

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 6e7242be1c87..b976084e56ef 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -469,18 +469,13 @@ extern efi_status_t efi64_thunk(u32, ...);
 	unsigned long flags;						\
 	u32 func;							\
 									\
-	efi_sync_low_kernel_mappings();					\
 	local_irq_save(flags);						\
-									\
-	efi_scratch.prev_cr3 = read_cr3();				\
-	write_cr3((unsigned long)efi_scratch.efi_pgt);			\
-	__flush_tlb_all();						\
+	arch_efi_call_virt_setup();					\
 									\
 	func = runtime_service32(f);					\
 	__s = efi64_thunk(func, __VA_ARGS__);			\
 									\
-	write_cr3(efi_scratch.prev_cr3);				\
-	__flush_tlb_all();						\
+	arch_efi_call_virt_teardown();					\
 	local_irq_restore(flags);					\
 									\
 	__s;								\