linux-kernel - Re: [PATCH 2/3] Update uv_bios_call 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160602194547.GK2658@codeblueprint.co.uk>
Date:	Thu, 2 Jun 2016 20:45:47 +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 2/3] Update uv_bios_call to use efi_call_virt_generic

On Wed, 18 May, at 02:11:40PM, Alex Thorlton wrote:
> Now that the efi_call_virt macro has been generalized to be able to
> use EFI system tables besides efi.systab, we are able to convert our
> uv_bios_call wrapper to use this standard EFI callback mechanism.
> 
> This simple change is part of a much larger effort to recover from some
> issues with the way we were mapping in some of our MMRs, and the way
> that we were doing our BIOS callbacks, which were uncovered by commit
> 67a9108ed431 ("x86/efi: Build our own page table structures").
> 
> The first issue that this uncovered was that we were relying on the EFI
> memory mapping mechanism to map in our MMR space for us, which, while
> reliable, was technically a bug, as it relied on "undefined" behavior in
> the mapping code.
> 
> The reason we were able to piggyback on the EFI memory mapping code to
> map in our MMRs was because, previously, EFI code used the
> trampoline_pgd, which shares a few entries with the main kernel pgd.  It
> just so happened, that the memory range containing our MMRs was inside
> one of those shared regions, which kept our code working without issue
> for quite a while.
> 
> Anyways, once we discovered this problem, we brought back our original
> code to map in the MMRs with commit 08914f436bdd ("x86/platform/UV:
> Bring back the call to map_low_mmrs in uv_system_init").  This got our
> systems a little further along, but we were still running into trouble
> with our EFI callbacks, which prevented us from booting all the way up.
> 
> Our first step towards fixing the BIOS callbacks was to get our
> uv_bios_call wrapper updated to use efi_call_virt instead of the plain
> efi_call.  The previous patch took care of the effort needed to make
> that possible.  Along the way, we hit a major issue with some confusion
> about how to properly pull arguments higher than number 6 off the stack
> in the efi_call code, which resulted in Linus's commit 683ad8092cd2
> ("x86/efi: Fix 7-parameter efi_call()s").
> 
> Now that all of those issues are out of the way, we're able to make this
> simple change to use the new efi_call_virt_generic in uv_bios_call which
> gets our machines booting, running properly, and able to execute our
> callbacks with 6+ arguments.
> 
> Signed-off-by: Alex Thorlton <athorlton@....com>
> Cc: Matt Fleming <matt@...eblueprint.co.uk>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Mike Travis <travis@....com>
> Cc: Russ Anderson <rja@....com>
> Cc: Dimitri Sivanich <sivanich@....com>
> Cc: x86@...nel.org
> Cc: linux-efi@...r.kernel.org
> ---
>  arch/x86/platform/uv/bios_uv.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 815fec6..0ae0826 100644
> --- a/arch/x86/platform/uv/bios_uv.c
> +++ b/arch/x86/platform/uv/bios_uv.c
> @@ -40,8 +40,7 @@ s64 uv_bios_call(enum uv_bios_cmd which, u64 a1, u64 a2, u64 a3, u64 a4, u64 a5)
>  		 */
>  		return BIOS_STATUS_UNIMPLEMENTED;
>  
> -	ret = efi_call((void *)__va(tab->function), (u64)which,
> -			a1, a2, a3, a4, a5);
> +	ret = efi_call_virt_generic(tab, function, (u64)which, a1, a2, a3, a4, a5);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(uv_bios_call);

Unless I've missed it, I didn't see an explanation in the changelog of
why it's OK to switch from using __va(tab->function) to tab->function
directly, which presumably is a physical address.

Was that intended?