On Mon, Nov 19, 2018 at 05:04:10PM +0100, Sebastian Andrzej Siewior wrote: > 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. "... ->initialized to 0." Also, a new line here pls. > With this change the CPU's FPU state is not saved ^ comma: , Also, instead of "with this change" I think you mean: "After ->initialized is cleared, the CPU's FPU state..." > to fpu->state during a context switch. > It then loads the state to fpu->state from userland and ensures it > sane. "... 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 fpu->initialized > anything (overwrite the new state) which is part of fpu__restore(). <---- newline here. > 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 Let's write it out once: "Bottom halves need to be... " > needs to be disabled because BH is allowed to run (even with preemption > disabled) and might invoke kernel_fpu_begin(). ... and let's put the potential example here with IPsec and softirq. > Link: https://lkml.kernel.org/r/20160226074940.GA28911@xxxxxxx > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > --- > v1…v2: A more verbose commit as message. Very much needed, thanks! > 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 { > -- -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
