linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: linux-kernel@vger.kernel.org
Cc: x86@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>,
	"Sebastian Andrzej Siewior" <bigeasy@linutronix.de>
Subject: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
Date: Wed,  7 Nov 2018 20:48:37 +0100	[thread overview]
Message-ID: <20181107194858.9380-3-bigeasy@linutronix.de> (raw)
In-Reply-To: <20181107194858.9380-1-bigeasy@linutronix.de>

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/restore on a context switch which would overwrite state.

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

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/include/asm/fpu/signal.h |  2 +-
 arch/x86/kernel/fpu/regset.c      |  5 ++--
 arch/x86/kernel/fpu/signal.c      | 41 ++++++++++++-------------------
 3 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 44bbc39a57b30..7fb516b6893a8 100644
--- 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 bc02f5144b958..5dbc099178a88 100644
--- 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 61a949d84dfa5..5777ee0c32fed 100644
--- 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(VERIFY_READ, 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),
@@ -314,41 +312,34 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 * thread's fpu state, reconstruct fxstate from the fsave
 		 * header. Validate and sanitize the copied state.
 		 */
+		union fpregs_state *state;
+		void *tmp;
 		struct user_i387_ia32_struct env;
 		int err = 0;
 
-		/*
-		 * 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 = kmalloc(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);
 		}
 
-		fpu->initialized = 1;
-		preempt_disable();
-		fpu__restore(fpu);
-		preempt_enable();
-
+		kfree(tmp);
 		return err;
 	} else {
 		/*
-- 
2.19.1


  parent reply	other threads:[~2018-11-07 19:49 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-07 19:48 [PATCH v4] x86: load FPU registers on return to userland Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 01/23] x86/fpu: Use ULL for shift in xfeature_uncompacted_offset() Sebastian Andrzej Siewior
2018-11-08 11:38   ` Borislav Petkov
2018-11-09 16:56     ` Sebastian Andrzej Siewior
2018-11-07 19:48 ` Sebastian Andrzej Siewior [this message]
2018-11-08 14:57   ` [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig() Borislav Petkov
2018-11-09 17:35     ` Sebastian Andrzej Siewior
2018-11-09 18:52       ` Borislav Petkov
2018-11-09 23:25         ` Sebastian Andrzej Siewior
2018-11-12 15:56           ` [PATCH] x86/fpu: Disable BH while while loading FPU registers " Sebastian Andrzej Siewior
2018-11-12 17:48             ` Dave Hansen
2018-11-19 11:41               ` Sebastian Andrzej Siewior
2018-11-19 15:04                 ` Dave Hansen
2018-11-19 15:06                   ` Sebastian Andrzej Siewior
2018-11-19 15:08                     ` Dave Hansen
2018-11-19 15:19                       ` Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 03/23] x86/fpu: Remove fpu__restore() Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 04/23] x86/entry/32: Remove asm/math_emu.h include Sebastian Andrzej Siewior
2018-11-08 18:45   ` Andy Lutomirski
2018-11-07 19:48 ` [PATCH 05/23] x86/fpu: Remove preempt_disable() in fpu__clear() Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 06/23] x86/fpu: Always init the `state' " Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 07/23] x86/fpu: Remove fpu->initialized usage in copy_fpstate_to_sigframe() Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 08/23] x86/fpu: Remove fpu->initialized Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 09/23] x86/fpu: Remove user_fpu_begin() Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 10/23] x86/entry: Remove _TIF_ALLWORK_MASK Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 11/23] x86/fpu: Add (__)make_fpregs_active helpers Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 12/23] x86/fpu: Make __raw_xsave_addr() use feature number instead of mask Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 13/23] x86/fpu: Make get_xsave_field_ptr() and get_xsave_addr() " Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 14/23] x86/pkeys: Make init_pkru_value static Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 15/23] x86/fpu: Only write PKRU if it is different from current Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 16/23] x86/pkeys: Don't check if PKRU is zero before writting it Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 17/23] x86/fpu: Eager switch PKRU state Sebastian Andrzej Siewior
2018-11-08 11:12   ` Paolo Bonzini
2018-11-19 18:17     ` Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 18/23] x86/entry: Add TIF_NEED_FPU_LOAD Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 19/23] x86/fpu: Always store the registers in copy_fpstate_to_sigframe() Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 20/23] x86/fpu: Prepare copy_fpstate_to_sigframe() for TIF_NEED_FPU_LOAD Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 21/23] x86/fpu: Update xstate's PKRU value on write_pkru() Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 22/23] x86/fpu: Don't restore the FPU state directly from userland in __fpu__restore_sig() Sebastian Andrzej Siewior
2018-11-08 18:25   ` Andy Lutomirski
2018-11-09 19:09     ` Sebastian Andrzej Siewior
2018-11-07 19:48 ` [PATCH 23/23] x86/fpu: Defer FPU state load until return to userspace Sebastian Andrzej Siewior
2018-11-08  8:33   ` Sebastian Andrzej Siewior
2018-11-12  3:02 ` [PATCH v4] x86: load FPU registers on return to userland Wanpeng Li
2018-11-12  3:26   ` Jason A. Donenfeld

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=20181107194858.9380-3-bigeasy@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=Jason@zx2c4.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@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).