[v4.9,STABLE] x86/fpu: Disable bottom halves while loading FPU registers

Commit Message

Sebastian Andrzej Siewior Dec. 21, 2018, 4:23 p.m. UTC

The sequence

  fpu->initialized = 1;		/* step A */
  preempt_disable();		/* step B */
  fpu__restore(fpu);
  preempt_enable();

in __fpu__restore_sig() is racy in regard to a context switch.

For 32bit frames, __fpu__restore_sig() prepares the FPU state within
fpu->state. To ensure that a context switch (switch_fpu_prepare() in
particular) does not modify fpu->state it uses fpu__drop() which sets
fpu->initialized to 0.

After fpu->initialized is cleared, the CPU's FPU state is not saved
to fpu->state during a context switch. The new state is loaded via
fpu__restore(). It gets loaded into fpu->state from userland and
ensured it is sane. fpu->initialized is then set to 1 in order to avoid
fpu__initialize() doing anything (overwrite the new state) which is part
of fpu__restore().

A context switch between step A and B above would save CPU's current FPU
registers to fpu->state and overwrite the newly prepared state. This
looks like a tiny race window but the Kernel Test Robot reported this
back in 2016 while we had lazy FPU support. Borislav Petkov made the
link between that report and another patch that has been posted. Since
the removal of the lazy FPU support, this race goes unnoticed because
the warning has been removed.

Disable bottom halves around the restore sequence to avoid the race. BH
need to be disabled because BH is allowed to run (even with preemption
disabled) and might invoke kernel_fpu_begin() by doing IPsec.

 [ bp: massage commit message a bit. ]

Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
Signed-off-by: Borislav Petkov <[e-mail]>
Acked-by: Ingo Molnar <[e-mail]>
Acked-by: Thomas Gleixner <[e-mail]>
Cc: Andy Lutomirski <[e-mail]>
Cc: Dave Hansen <[e-mail]>
Cc: "H. Peter Anvin" <[e-mail]>
Cc: "Jason A. Donenfeld" <[e-mail]>
Cc: kvm ML <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Radim Krčmář <[e-mail]>
Cc: Rik van Riel <[e-mail]>
Cc: [e-mail]
Cc: x86-ml <[e-mail]>
Link: http://lkml.kernel.org/r/[e-mail]
Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
---
 arch/x86/kernel/fpu/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Greg Kroah-Hartman Dec. 21, 2018, 4:29 p.m. UTC | #1

On Fri, Dec 21, 2018 at 05:23:38PM +0100, Sebastian Andrzej Siewior wrote:
> The sequence
> 
>   fpu->initialized = 1;		/* step A */
>   preempt_disable();		/* step B */
>   fpu__restore(fpu);
>   preempt_enable();
> 
> in __fpu__restore_sig() is racy in regard to a context switch.
> 
> For 32bit frames, __fpu__restore_sig() prepares the FPU state within
> fpu->state. To ensure that a context switch (switch_fpu_prepare() in
> particular) does not modify fpu->state it uses fpu__drop() which sets
> fpu->initialized to 0.
> 
> After fpu->initialized is cleared, the CPU's FPU state is not saved
> to fpu->state during a context switch. The new state is loaded via
> fpu__restore(). It gets loaded into fpu->state from userland and
> ensured it is sane. fpu->initialized is then set to 1 in order to avoid
> fpu__initialize() doing anything (overwrite the new state) which is part
> of fpu__restore().
> 
> A context switch between step A and B above would save CPU's current FPU
> registers to fpu->state and overwrite the newly prepared state. This
> looks like a tiny race window but the Kernel Test Robot reported this
> back in 2016 while we had lazy FPU support. Borislav Petkov made the
> link between that report and another patch that has been posted. Since
> the removal of the lazy FPU support, this race goes unnoticed because
> the warning has been removed.
> 
> Disable bottom halves around the restore sequence to avoid the race. BH
> need to be disabled because BH is allowed to run (even with preemption
> disabled) and might invoke kernel_fpu_begin() by doing IPsec.
> 
>  [ bp: massage commit message a bit. ]
> 
> Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
> Signed-off-by: Borislav Petkov <[e-mail]>
> Acked-by: Ingo Molnar <[e-mail]>
> Acked-by: Thomas Gleixner <[e-mail]>
> Cc: Andy Lutomirski <[e-mail]>
> Cc: Dave Hansen <[e-mail]>
> Cc: "H. Peter Anvin" <[e-mail]>
> Cc: "Jason A. Donenfeld" <[e-mail]>
> Cc: kvm ML <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Radim Krčmář <[e-mail]>
> Cc: Rik van Riel <[e-mail]>
> Cc: [e-mail]
> Cc: x86-ml <[e-mail]>
> Link: http://lkml.kernel.org/r/[e-mail]
> Link: https://lkml.kernel.org/r/[e-mail]
> Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
> ---
>  arch/x86/kernel/fpu/signal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

What is the git commit id of this patch upstream?

thanks,

greg k-h

Sebastian Andrzej Siewior Dec. 21, 2018, 4:38 p.m. UTC | #2

On 2018-12-21 17:29:05 [+0100], Greg Kroah-Hartman wrote:
> What is the git commit id of this patch upstream?

commit 68239654acafe6aad5a3c1dc7237e60accfebc03 upstream.

I'm sorry. I cherry picked the original commit, resolved the conflict
and forgot about the original commit id…

> thanks,
> 
> greg k-h

Sebastian

Patch

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index ae52ef05d0981..769831d9fd114 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -342,10 +342,10 @@  static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			sanitize_restored_xstate(tsk, &env, xfeatures, fx_only);
 		}
 
+		local_bh_disable();
 		fpu->fpstate_active = 1;
-		preempt_disable();
 		fpu__restore(fpu);
-		preempt_enable();
+		local_bh_enable();
 
 		return err;
 	} else {