x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()

This is a preparation for the removal of the ->initialized member in the
fpu struct.

__fpu__restore_sig() is deactivating the FPU via fpu__drop() and then
setting manually ->initialized followed by fpu__restore(). The result is
that it is possible to manipulate fpu->state and the state of registers
won't be saved/restored on a context switch which would overwrite
fpu->state:

fpu__drop(fpu):
  ...
  fpu->initialized = 0;
  preempt_enable();

  <--- context switch

Don't access the fpu->state while the content is read from user space
and examined/sanitized. Use a temporary kmalloc() buffer for the
preparation of the FPU registers and once the state is considered okay,
load it. Should something go wrong, return with an error and without
altering the original FPU registers.

The removal of fpu__initialize() is a nop because fpu->initialized is
already set for the user task.

 [ bp: Massage a bit. ]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Borislav Petkov <bp@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: kvm ML <kvm@vger.kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Rik van Riel <riel@surriel.com>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190403164156.19645-2-bigeasy@linutronix.de

diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 44bbc39a57b30be9bf6b71103d5a162e0c61c0e3..7fb516b6893a8fea46895275c8496b484446e333 100644 (file)
--- a/arch/x86/include/asm/fpu/signal.h
+++ b/arch/x86/include/asm/fpu/signal.h
@@ -22,7 +22,7 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
 
 extern void convert_from_fxsr(struct user_i387_ia32_struct *env,
                              struct task_struct *tsk);
-extern void convert_to_fxsr(struct task_struct *tsk,
+extern void convert_to_fxsr(struct fxregs_state *fxsave,
                            const struct user_i387_ia32_struct *env);
 
 unsigned long

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index bc02f5144b958bd8182f284e2cfa6dd5a65384c8..5dbc099178a88b14043c432e345ca367ca421830 100644 (file)
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -269,11 +269,10 @@ convert_from_fxsr(struct user_i387_ia32_struct *env, struct task_struct *tsk)
                memcpy(&to[i], &from[i], sizeof(to[0]));
 }
 
-void convert_to_fxsr(struct task_struct *tsk,
+void convert_to_fxsr(struct fxregs_state *fxsave,
                     const struct user_i387_ia32_struct *env)
 
 {
-       struct fxregs_state *fxsave = &tsk->thread.fpu.state.fxsave;
        struct _fpreg *from = (struct _fpreg *) &env->st_space[0];
        struct _fpxreg *to = (struct _fpxreg *) &fxsave->st_space[0];
        int i;
@@ -350,7 +349,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 
        ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &env, 0, -1);
        if (!ret)
-               convert_to_fxsr(target, &env);
+               convert_to_fxsr(&target->thread.fpu.state.fxsave, &env);
 
        /*
         * update the header bit in the xsave header, indicating the

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 55b80de13ea5a2c0d9e8accb3c999079a2ba1bcd..7296a9bb78e7824213947dac39b330b7b1f813a7 100644 (file)
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -207,11 +207,11 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 }
 
 static inline void
-sanitize_restored_xstate(struct task_struct *tsk,
+sanitize_restored_xstate(union fpregs_state *state,
                         struct user_i387_ia32_struct *ia32_env,
                         u64 xfeatures, int fx_only)
 {
-       struct xregs_state *xsave = &tsk->thread.fpu.state.xsave;
+       struct xregs_state *xsave = &state->xsave;
        struct xstate_header *header = &xsave->header;
 
        if (use_xsave()) {
@@ -238,7 +238,7 @@ sanitize_restored_xstate(struct task_struct *tsk,
                 */
                xsave->i387.mxcsr &= mxcsr_feature_mask;
 
-               convert_to_fxsr(tsk, ia32_env);
+               convert_to_fxsr(&state->fxsave, ia32_env);
        }
 }
 
@@ -284,8 +284,6 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
        if (!access_ok(buf, size))
                return -EACCES;
 
-       fpu__initialize(fpu);
-
        if (!static_cpu_has(X86_FEATURE_FPU))
                return fpregs_soft_set(current, NULL,
                                       0, sizeof(struct user_i387_ia32_struct),
@@ -315,40 +313,32 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
                 * header. Validate and sanitize the copied state.
                 */
                struct user_i387_ia32_struct env;
+               union fpregs_state *state;
                int err = 0;
+               void *tmp;
 
-               /*
-                * Drop the current fpu which clears fpu->initialized. This ensures
-                * that any context-switch during the copy of the new state,
-                * avoids the intermediate state from getting restored/saved.
-                * Thus avoiding the new restored state from getting corrupted.
-                * We will be ready to restore/save the state only after
-                * fpu->initialized is again set.
-                */
-               fpu__drop(fpu);
+               tmp = kzalloc(sizeof(*state) + fpu_kernel_xstate_size + 64, GFP_KERNEL);
+               if (!tmp)
+                       return -ENOMEM;
+               state = PTR_ALIGN(tmp, 64);
 
                if (using_compacted_format()) {
-                       err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
+                       err = copy_user_to_xstate(&state->xsave, buf_fx);
                } else {
-                       err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
+                       err = __copy_from_user(&state->xsave, buf_fx, state_size);
 
                        if (!err && state_size > offsetof(struct xregs_state, header))
-                               err = validate_xstate_header(&fpu->state.xsave.header);
+                               err = validate_xstate_header(&state->xsave.header);
                }
 
                if (err || __copy_from_user(&env, buf, sizeof(env))) {
-                       fpstate_init(&fpu->state);
-                       trace_x86_fpu_init_state(fpu);
                        err = -1;
                } else {
-                       sanitize_restored_xstate(tsk, &env, xfeatures, fx_only);
+                       sanitize_restored_xstate(state, &env, xfeatures, fx_only);
+                       copy_kernel_to_fpregs(state);
                }
 
-               local_bh_disable();
-               fpu->initialized = 1;
-               fpu__restore(fpu);
-               local_bh_enable();
-
+               kfree(tmp);
                return err;
        } else {
                /*