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]

Message-ID: <20160602212515.GE242721@stormcage.americas.sgi.com>
Date:	Thu, 2 Jun 2016 16:25:15 -0500
From:	Alex Thorlton <athorlton@....com>
To:	Matt Fleming <matt@...eblueprint.co.uk>
Cc:	Alex Thorlton <athorlton@....com>, 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 Thu, Jun 02, 2016 at 09:19:49PM +0100, Matt Fleming wrote:
> On Wed, 18 May, at 02:11:41PM, Alex Thorlton wrote:
> > +#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.

Ahh, yep, you're absolutely correct.  That's not what I intended to do,
but that's definitely the effect that this change has.

> 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;								\

This looks good to me.  We're at least making use of the
arch_efi_call_virt_* stuff where possible, and only using the special
thunk code where necessary.  I think it's a good middle ground between
the two approaches (especially considering the fact that mine won't
work :) 

I will re-work that last patch to include this change instead of my
original, broken one.

Thanks, Matt!

- Alex