[PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Borislav Petkov]

Ok,

here's the fix first so that it goes in. I'll hammer on the test case later.

---
From: Petteri Aimonen <jpa@git.mail.kapsi.fi>

Previously, kernel floating point code would run with the MXCSR control
register value last set by userland code by the thread that was active
on the CPU core just before kernel call. This could affect calculation
results if rounding mode was changed, or a crash if a FPU/SIMD exception
was unmasked.

Restore MXCSR to the kernel's default value.

 [ bp: Carve out from a bigger patch by Petteri, add feature check. ]

Signed-off-by: Petteri Aimonen <jpa@git.mail.kapsi.fi>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=207979
---
 arch/x86/include/asm/fpu/internal.h | 5 +++++
 arch/x86/kernel/fpu/core.c          | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 42159f45bf9c..845e7481ab77 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -623,6 +623,11 @@ static inline void switch_fpu_finish(struct fpu *new_fpu)
  * MXCSR and XCR definitions:
  */
 
+static inline void ldmxcsr(u32 mxcsr)
+{
+	asm volatile("ldmxcsr %0" :: "m" (mxcsr));
+}
+
 extern unsigned int mxcsr_feature_mask;
 
 #define XCR_XFEATURE_ENABLED_MASK	0x00000000
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 06c818967bb6..f398fedc590a 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -101,6 +101,9 @@ void kernel_fpu_begin(void)
 		copy_fpregs_to_fpstate(&current->thread.fpu);
 	}
 	__cpu_invalidate_fpregs_state();
+
+	if (boot_cpu_has(X86_FEATURE_XMM))
+		ldmxcsr(MXCSR_DEFAULT);
 }
 EXPORT_SYMBOL_GPL(kernel_fpu_begin);
 
-- 
2.21.0


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Andy Lutomirski]

On Tue, Jun 16, 2020 at 2:53 AM Borislav Petkov <bp@alien8.de> wrote:
>
> Ok,
>
> here's the fix first so that it goes in. I'll hammer on the test case later.

Does the 32-bit case need FNINIT?

>
> ---
> From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
>
> Previously, kernel floating point code would run with the MXCSR control
> register value last set by userland code by the thread that was active
> on the CPU core just before kernel call. This could affect calculation
> results if rounding mode was changed, or a crash if a FPU/SIMD exception
> was unmasked.
>
> Restore MXCSR to the kernel's default value.
>
>  [ bp: Carve out from a bigger patch by Petteri, add feature check. ]
>
> Signed-off-by: Petteri Aimonen <jpa@git.mail.kapsi.fi>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=207979
> ---
>  arch/x86/include/asm/fpu/internal.h | 5 +++++
>  arch/x86/kernel/fpu/core.c          | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 42159f45bf9c..845e7481ab77 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -623,6 +623,11 @@ static inline void switch_fpu_finish(struct fpu *new_fpu)
>   * MXCSR and XCR definitions:
>   */
>
> +static inline void ldmxcsr(u32 mxcsr)
> +{
> +       asm volatile("ldmxcsr %0" :: "m" (mxcsr));
> +}
> +
>  extern unsigned int mxcsr_feature_mask;
>
>  #define XCR_XFEATURE_ENABLED_MASK      0x00000000
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 06c818967bb6..f398fedc590a 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -101,6 +101,9 @@ void kernel_fpu_begin(void)
>                 copy_fpregs_to_fpstate(&current->thread.fpu);
>         }
>         __cpu_invalidate_fpregs_state();
> +
> +       if (boot_cpu_has(X86_FEATURE_XMM))
> +               ldmxcsr(MXCSR_DEFAULT);
>  }
>  EXPORT_SYMBOL_GPL(kernel_fpu_begin);
>
> --
> 2.21.0
>
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Borislav Petkov]

On Tue, Jun 16, 2020 at 09:53:39AM -0700, Andy Lutomirski wrote:
> On Tue, Jun 16, 2020 at 2:53 AM Borislav Petkov <bp@alien8.de> wrote:
> >
> > Ok,
> >
> > here's the fix first so that it goes in. I'll hammer on the test case later.
> 
> Does the 32-bit case need FNINIT?

Pasting from IRC:

I'm thinking if you'd need to reinit the FPU, then you need to do it for
both, not only 32-bit or do you mean something else? Also, if you end up
doing FNSAVE (old CPU) that one reinits state.

Whatever we decide doing, this should be a separate patch anyway.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Andy Lutomirski]

> On Jun 16, 2020, at 11:01 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Tue, Jun 16, 2020 at 09:53:39AM -0700, Andy Lutomirski wrote:
>>> On Tue, Jun 16, 2020 at 2:53 AM Borislav Petkov <bp@alien8.de> wrote:
>>> 
>>> Ok,
>>> 
>>> here's the fix first so that it goes in. I'll hammer on the test case later.
>> 
>> Does the 32-bit case need FNINIT?
> 
> Pasting from IRC:
> 
> I'm thinking if you'd need to reinit the FPU, then you need to do it for
> both, not only 32-bit or do you mean something else? Also, if you end up
> doing FNSAVE (old CPU) that one reinits state.

We definitely need to sanitize MXCSR for kernel fpu if kernel fpu means SSE2.  If kernel fpu means x87, we need to fix the fpu control word.

On x86_64, I suspect the UEFI ABI technically requires a clean x87 control word too. If we’re willing to declare that the kernel proper won’t use x87, then we could shove that into the UEFI code.

> 
> Whatever we decide doing, this should be a separate patch anyway.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Borislav Petkov]

On Tue, Jun 16, 2020 at 02:17:16PM -0700, Andy Lutomirski wrote:
> We definitely need to sanitize MXCSR for kernel fpu if kernel fpu
> means SSE2. If kernel fpu means x87, we need to fix the fpu control
> word.

Bah, there's no need to beat around the bush - let's just do:

        if (boot_cpu_has(X86_FEATURE_XMM))
                ldmxcsr(MXCSR_DEFAULT);

        if (boot_cpu_has(X86_FEATURE_FPU))
                asm volatile ("fninit");

and be sure that kernel users get a squeaky-clean FPU.

> On x86_64, I suspect the UEFI ABI technically requires a clean x87
> control word too. If we’re willing to declare that the kernel proper
> won’t use x87, then we could shove that into the UEFI code.

Nah, we don't trust the firmware.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Andy Lutomirski]

On Wed, Jun 17, 2020 at 1:33 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Jun 16, 2020 at 02:17:16PM -0700, Andy Lutomirski wrote:
> > We definitely need to sanitize MXCSR for kernel fpu if kernel fpu
> > means SSE2. If kernel fpu means x87, we need to fix the fpu control
> > word.
>
> Bah, there's no need to beat around the bush - let's just do:
>
>         if (boot_cpu_has(X86_FEATURE_XMM))
>                 ldmxcsr(MXCSR_DEFAULT);
>
>         if (boot_cpu_has(X86_FEATURE_FPU))
>                 asm volatile ("fninit");
>
> and be sure that kernel users get a squeaky-clean FPU.
>
> > On x86_64, I suspect the UEFI ABI technically requires a clean x87
> > control word too. If we’re willing to declare that the kernel proper
> > won’t use x87, then we could shove that into the UEFI code.
>
> Nah, we don't trust the firmware.

What I mean is: if we trust ourselves to have no x87 instructions in
the kernel, we could put the FNINIT in the UEFI stubs to save some
cycles.  I don't know how slow FNINIT is.

>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
Andy Lutomirski
AMA Capital Management, LLC