linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org, "Ingo Molnar" <mingo@kernel.org>,
	linux-kernel@vger.kernel.org, "Andy Lutomirski" <luto@kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	kvm@vger.kernel.org, "Jason A. Donenfeld" <Jason@zx2c4.com>,
	"Rik van Riel" <riel@surriel.com>,
	"Dave Hansen" <dave.hansen@linux.intel.com>
Subject: [PATCH v3] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()
Date: Tue, 20 Nov 2018 11:26:35 +0100	[thread overview]
Message-ID: <20181120102635.ddv3fvavxajjlfqk@linutronix.de> (raw)
In-Reply-To: <20181119181000.GH14688@zn.tnic>

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

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. It then loads the new state to
fpu->state from userland and ensures it is sane. The new state is loaded
via fpu__restore(). The code sets then fpu->initializes 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 would save CPU's current FPU
registers to fpu->state and overwrite the newly prepared state. This
looks like 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 halfes around the restore sequence to avoid the race. BH
needs to be disabled because BH is allowed to run (even with preemption
disabled) and might invoke kernel_fpu_begin() by doing IPsec.

Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
Cc: stable@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: Rewording parts of the commit message as per Borislav Petkov.
v1…v2: A more verbose commit as message.

 arch/x86/kernel/fpu/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 61a949d84dfa5..d99a8ee9e185e 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -344,10 +344,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->initialized = 1;
-		preempt_disable();
 		fpu__restore(fpu);
-		preempt_enable();
+		local_bh_enable();
 
 		return err;
 	} else {
-- 
2.19.1


  reply	other threads:[~2018-11-20 10:27 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-19 16:04 [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() Sebastian Andrzej Siewior
2018-11-19 17:02 ` Dave Hansen
2018-11-19 17:11   ` Sebastian Andrzej Siewior
2018-11-19 17:27     ` Borislav Petkov
2018-11-19 17:31       ` Sebastian Andrzej Siewior
2018-11-19 17:41         ` Borislav Petkov
2018-11-19 17:32       ` Dave Hansen
2018-11-19 18:10 ` Borislav Petkov
2018-11-20 10:26   ` Sebastian Andrzej Siewior [this message]
     [not found] ` <20181120132531.6E80C206BB@mail.kernel.org>
2018-11-20 18:34   ` Borislav Petkov
     [not found]     ` <CAMU5Q=72mNG4ikXKpLK3=SiDK3dRXtCw7wpHZ2i==awrtvRwdg@mail.gmail.com>
2018-11-21 10:56       ` Borislav Petkov
     [not found]         ` <CAMU5Q=5LkypghWA=23YwscaXSx5yBv7LyW1WHbY_J_hQwryNpA@mail.gmail.com>
2018-11-21 12:54           ` Boris Petkov
     [not found] <87d1rk9str.fsf@yhuang-dev.intel.com>
2016-02-26  7:49 ` [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() Borislav Petkov
2016-02-27 12:02   ` Ingo Molnar
2016-02-27 13:13     ` Borislav Petkov
2018-11-20 12:05   ` [tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers tip-bot for Sebastian Andrzej Siewior
2018-11-20 16:29   ` tip-bot for Sebastian Andrzej Siewior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181120102635.ddv3fvavxajjlfqk@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=Jason@zx2c4.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=riel@surriel.com \
    --cc=rkrcmar@redhat.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).