Re: [patch 20/24] x86/speculation: Split out TIF update

[Jiri Kosina <jikos@kernel.org>]

On Tue, 27 Nov 2018, Jiri Kosina wrote:

> > That's racy and does not prevent the situation because the TIF flags are
> > updated befor the UPDATE bit is set. 
> 
> > So __speculation_ctrl_update() might see the new bits, but not 
> > TIF_SPEC_UPDATE. 
> 
> Hm, right, scratch that. We'd need to do that before updating TIF_SPEC_IB 
> in task_update_spec_tif(), but that has the opposite ordering issue.

I think this should do it (not yet fully tested).


From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] x86/speculation: Always properly update SPEC_CTRL MSR for remote SECCOMP tasks

If seccomp task is setting TIF_SPEC_IB for a task running on remote CPU, 
the value of TIF_SPEC_IB becomes out-of-sync with the actual MSR value on 
that CPU.

This becomes a problem when such task then context switches to another 
task that has TIF_SPEC_IB set, as in such case the value of SPEC_CTRL MSR 
is not updated and the next task starts running with stale value of 
SPEC_CTRL, potentially unprotected by STIBP.

Fix that by "queuing" the needed flags update in 'spec_ctrl' shadow 
variable, and populating proper TI flags with it on context switch to the 
task that has the TIF_SPEC_UPDATE flag (indicating that syncing is 
necessary) set.

Reported-by: Tim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/include/asm/thread_info.h |  5 ++++-
 arch/x86/kernel/cpu/bugs.c         | 13 +++++++++++--
 arch/x86/kernel/process.c          | 15 +++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 6d201699c651..001b053067d7 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -55,6 +55,7 @@ struct task_struct;
 
 struct thread_info {
 	unsigned long		flags;		/* low level flags */
+	unsigned long		spec_flags;	/* spec flags to sync on ctxsw */
 	u32			status;		/* thread synchronous flags */
 };
 
@@ -84,6 +85,7 @@ struct thread_info {
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
 #define TIF_SPEC_IB		9	/* Indirect branch speculation mitigation */
+#define TIF_SPEC_UPDATE		10	/* SPEC_CTRL MSR sync needed on CTXSW */
 #define TIF_USER_RETURN_NOTIFY	11	/* notify kernel of userspace return */
 #define TIF_UPROBE		12	/* breakpointed or singlestepping */
 #define TIF_PATCH_PENDING	13	/* pending live patching update */
@@ -112,6 +114,7 @@ struct thread_info {
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 #define _TIF_SPEC_IB		(1 << TIF_SPEC_IB)
+#define _TIF_SPEC_UPDATE	(1 << TIF_SPEC_UPDATE)
 #define _TIF_USER_RETURN_NOTIFY	(1 << TIF_USER_RETURN_NOTIFY)
 #define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_PATCH_PENDING	(1 << TIF_PATCH_PENDING)
@@ -155,7 +158,7 @@ struct thread_info {
  * Avoid calls to __switch_to_xtra() on UP as STIBP is not evaluated.
  */
 #ifdef CONFIG_SMP
-# define _TIF_WORK_CTXSW	(_TIF_WORK_CTXSW_BASE | _TIF_SPEC_IB)
+# define _TIF_WORK_CTXSW	(_TIF_WORK_CTXSW_BASE | _TIF_SPEC_IB | _TIF_SPEC_UPDATE)
 #else
 # define _TIF_WORK_CTXSW	(_TIF_WORK_CTXSW_BASE)
 #endif
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index b5d2b36618a5..679946135789 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -761,9 +761,11 @@ static void task_update_spec_tif(struct task_struct *tsk, int tifbit, bool on)
 	bool update;
 
 	if (on)
-		update = !test_and_set_tsk_thread_flag(tsk, tifbit);
+		update = !test_and_set_bit(tifbit,
+				&task_thread_info(tsk)->spec_flags);
 	else
-		update = test_and_clear_tsk_thread_flag(tsk, tifbit);
+		update = test_and_clear_bit(tifbit,
+				&task_thread_info(tsk)->spec_flags);
 
 	/*
 	 * Immediately update the speculation control MSRs for the current
@@ -772,9 +774,16 @@ static void task_update_spec_tif(struct task_struct *tsk, int tifbit, bool on)
 	 *
 	 * This can only happen for SECCOMP mitigation. For PRCTL it's
 	 * always the current task.
+	 *
+	 * If we are updating non-current SECCOMP task, set a flag for it to
+	 * always perform the MSR sync on a first context switch to it, in order
+	 * to make sure the TIF_SPEC_IB above is not out of sync with the MSR value.
 	 */
 	if (tsk == current && update)
 		speculation_ctrl_update_current();
+	else
+		set_tsk_thread_flag(tsk, TIF_SPEC_UPDATE);
+
 }
 
 static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 3f5e351bdd37..6c4fcef52b19 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -474,6 +474,21 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p)
 
 	tifn = READ_ONCE(task_thread_info(next_p)->flags);
 	tifp = READ_ONCE(task_thread_info(prev_p)->flags);
+
+	/*
+	 * SECCOMP tasks might have had their spec_ctrl flags updated during
+	 * runtime from a different CPU.
+	 *
+	 * When switching to such a task, populate thread flags with the ones
+	 * that have been temporarily saved in spec_flags by task_update_spec_tif()
+	 * in order to make sure MSR value is always kept up to date.
+	 *
+	 * SECCOMP tasks never disable the mitigation for other threads, only enable.
+	 */
+	if (IS_ENABLED(CONFIG_SECCOMP) &&
+			test_and_clear_tsk_thread_flag(next_p, TIF_SPEC_UPDATE))
+		tifp |= READ_ONCE(task_thread_info(next_p)->spec_flags);
+
 	switch_to_bitmap(prev, next, tifp, tifn);
 
 	propagate_user_return_notify(prev_p, next_p);

-- 
Jiri Kosina
SUSE Labs