Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64)

Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64)

[Borislav Petkov]

On Sat, Apr 19, 2014 at 03:50:44PM -0700, tip-bot for Ricardo Neri wrote:
> Commit-ID:  de05764e0b2a3d9559e099a2e134f8cef4500fdd
> Gitweb:     http://git.kernel.org/tip/de05764e0b2a3d9559e099a2e134f8cef4500fdd
> Author:     Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> AuthorDate: Thu, 27 Mar 2014 15:10:42 -0700
> Committer:  Matt Fleming <matt.fleming@intel.com>
> CommitDate: Thu, 17 Apr 2014 13:26:32 +0100
> 
> x86/efi: Save and restore FPU context around efi_calls (x86_64)
> 
> Do a complete FPU context save/restore around the EFI calls. This required
> as runtime EFI firmware may potentially use the FPU.
> 
> This change covers only the x86_64 configuration.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  arch/x86/include/asm/efi.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 19292e7..0b19187 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -1,6 +1,7 @@
>  #ifndef _ASM_X86_EFI_H
>  #define _ASM_X86_EFI_H
>  
> +#include <asm/i387.h>
>  /*
>   * We map the EFI regions needed for runtime services non-contiguously,
>   * with preserved alignment on virtual addresses starting from -4G down
> @@ -54,7 +55,9 @@ extern u64 asmlinkage efi_call(void *fp, ...);
>  									\
>  	efi_sync_low_kernel_mappings();					\
>  	preempt_disable();						\
> +	__kernel_fpu_begin();						\
>  	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
> +	__kernel_fpu_end();						\
>  	preempt_enable();						\

I guess you can use the kernel_fpu_begin/end() variants here (i.e.,
without the "__") which disable and enable preemption and thus drop the
preempt_* calls:

	efi_sync_low_kernel_mappings();
	kernel_fpu_begin();
	 __s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);
	 kernel_fpu_end();
	 __s;

I'm not sure about the

	WARN_ON_ONCE(!irq_fpu_usable());

thing in kernel_fpu_begin() though, I guess it wouldn't hurt...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64)

[Matt Fleming]

On Sun, 20 Apr, at 02:28:11AM, Borislav Petkov wrote:
> 
> I guess you can use the kernel_fpu_begin/end() variants here (i.e.,
> without the "__") which disable and enable preemption and thus drop the
> preempt_* calls:
> 
> 	efi_sync_low_kernel_mappings();
> 	kernel_fpu_begin();
> 	 __s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);
> 	 kernel_fpu_end();
> 	 __s;
> 
> I'm not sure about the
> 
> 	WARN_ON_ONCE(!irq_fpu_usable());
> 
> thing in kernel_fpu_begin() though, I guess it wouldn't hurt...

Hmm... note that we may call EFI runtime services from interrupt context
in efi_pstore_write(), so it seems like it would be possible to trigger
that WARN_ON_ONCE() there.

Seiji (Cc'd) might have some opinions on this.

Either way, if someone sends me a patch ontop of this one that swaps the
__kernel_fpu_begin() for kernel_fpu_begin() I can try them out in my
lab.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64)

[Borislav Petkov]

On Fri, Apr 25, 2014 at 12:09:36PM +0100, Matt Fleming wrote:
> Hmm... note that we may call EFI runtime services from interrupt context
> in efi_pstore_write(), so it seems like it would be possible to trigger
> that WARN_ON_ONCE() there.
> 
> Seiji (Cc'd) might have some opinions on this.
> 
> Either way, if someone sends me a patch ontop of this one that swaps the
> __kernel_fpu_begin() for kernel_fpu_begin() I can try them out in my
> lab.

Well, the more I think about it, the more I'm persuaded that
you actually do *really* need that WARN_ON_ONCE check there to
make sure you're not fiddling with the FPU while in an interrupt
context and in an unsafe way (see interrupted_kernel_fpu_idle() and
interrupted_user_mode()). And so you do need the variants without the
"__" which include the check.

Anyway, here it is, do give it a good run:

---
From: Borislav Petkov <bp@suse.de>
Date: Fri, 25 Apr 2014 13:30:21 +0200
Subject: [PATCH] efi: Check for unsafe dealing with FPU state in irq ctxt

efi_call can happen in an irq context (pstore) and there we really need
to make sure we're not scribbling over FPU state while we've interrupted
a thread or kernel mode with a live FPU state. Therefore, use the
kernel_fpu_begin/end() variants which do that check.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/efi.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 1eb5f6433ad8..f969ce8bea24 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -67,11 +67,9 @@ extern u64 asmlinkage efi_call(void *fp, ...);
 	efi_status_t __s;						\
 									\
 	efi_sync_low_kernel_mappings();					\
-	preempt_disable();						\
-	__kernel_fpu_begin();						\
+	kernel_fpu_begin();						\
 	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
-	__kernel_fpu_end();						\
-	preempt_enable();						\
+	kernel_fpu_end();						\
 	__s;								\
 })
 
-- 
1.9.0.258.g00eda23

Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64)

[Matt Fleming]

On Fri, 25 Apr, at 01:40:10PM, Borislav Petkov wrote:
> 
> Well, the more I think about it, the more I'm persuaded that
> you actually do *really* need that WARN_ON_ONCE check there to
> make sure you're not fiddling with the FPU while in an interrupt
> context and in an unsafe way (see interrupted_kernel_fpu_idle() and
> interrupted_user_mode()). And so you do need the variants without the
> "__" which include the check.
> 
> Anyway, here it is, do give it a good run:

Thanks Borislav, I gave this a run on my machines and everything appears
to be fine. I'll probably send it to tip by the end of the week, I'm
just gonna let it stew on my 'next' branch for a while.

-- 
Matt Fleming, Intel Open Source Technology Center