linux-kernel - [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 19 Nov 2018 17:04:10 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>, x86@...nel.org
Cc:     Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in
 __fpu__restore_sig()

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->initializes to 0. With this change the CPU's FPU state is not saved
to fpu->state during a context switch. It then loads the state to
fpu->state from userland and ensures it 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.

Use local_bh_disable() 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().

Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
Cc: stable@...r.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
---
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