[PATCH v4 0/2] Harden spectrev2 userspace-userspace protection

[PATCH v4 0/2] Harden spectrev2 userspace-userspace protection

[Jiri Kosina]

Currently, linux kernel is basically not preventing userspace-userspace 
spectrev2 attack, because:

- IBPB is basically unused (issued only for tasks that marked themselves 
  explicitly non-dumpable, which is absolutely negligible minority of all 
  software out there), therefore cross-process branch buffer posioning 
  using spectrev2 is possible

- STIBP is completely unused, therefore cross-process branch buffer 
  poisoning using spectrev2 between processess running on two HT siblings 
  thread s is possible

This patchset changes IBPB semantics, so that it's now applied whenever 
context-switching between processess that can't use ptrace() to achieve 
the same. This admittedly comes with extra overhad on a context switch; 
systems that don't care about could disable the mitigation using 
nospectre_v2 boot option.
The IBPB implementaion is heavily based on original patches by Tim Chen.

In addition to that, we unconditionally turn STIBP on so that HT siblings 
always have separate branch buffers.

We've been carrying IBPB implementation with the same semantics in our 
(SUSE) trees since january disclosure; STIBP was more or less ignored up 
to today.

v1->v2:
        include IBPB changes
v2->v3: 
        fix IBPB 'who can trace who' semantics
        wire up STIBP flipping to SMT hotplug
v3->v4:
	dropped ___ptrace_may_access(), as it's not needed
	fixed deadlock with LSM/audit/selinux (Andrea Arcangeli)
	statically patch out the ptrace check if !IBPB

Jiri Kosina (2):
      x86/speculation: apply IBPB more strictly to avoid cross-process data leak
      x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

 arch/x86/kernel/cpu/bugs.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/mm/tlb.c          | 31 ++++++++++++++++++++-----------
 include/linux/ptrace.h     |  4 ++++
 kernel/cpu.c               | 13 ++++++++++++-
 kernel/ptrace.c            | 12 ++++++++----
 5 files changed, 107 insertions(+), 16 deletions(-)

-- 
Jiri Kosina
SUSE Labs

[PATCH v4 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

Currently, we are issuing IBPB only in cases when switching into a non-dumpable
process, the rationale being to protect such 'important and security sensitive'
processess (such as GPG) from data leak into a different userspace process via
spectre v2.

This is however completely insufficient to provide proper userspace-to-userpace
spectrev2 protection, as any process can poison branch buffers before being
scheduled out, and the newly scheduled process immediately becomes spectrev2
victim.

In order to minimize the performance impact (for usecases that do require
spectrev2 protection), issue the barrier only in cases when switching between
processess where the victim can't be ptraced by the potential attacker (as in
such cases, the attacker doesn't have to bother with branch buffers at all).

Fixes: 18bf3c3ea8 ("x86/speculation: Use Indirect Branch Prediction Barrier in context switch")
Originally-by: Tim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

Hopefully properly threaded now, sorry for the duplicates. alpine 2.21 
clearly has a bug that it sometimes eats Reply-to: header :/ Apologizes 
for all the noise

 arch/x86/mm/tlb.c      | 31 ++++++++++++++++++++-----------
 include/linux/ptrace.h |  4 ++++
 kernel/ptrace.c        | 12 ++++++++----
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index e96b99eb800c..ed4444402441 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -7,6 +7,7 @@
 #include <linux/export.h>
 #include <linux/cpu.h>
 #include <linux/debugfs.h>
+#include <linux/ptrace.h>
 
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
@@ -180,6 +181,19 @@ static void sync_current_stack_to_mm(struct mm_struct *mm)
 	}
 }
 
+static bool ibpb_needed(struct task_struct *tsk, u64 last_ctx_id)
+{
+	/*
+	 * Check if the current (previous) task has access to the memory
+	 * of the @tsk (next) task. If access is denied, make sure to
+	 * issue a IBPB to stop user->user Spectre-v2 attacks.
+	 *
+	 * Note: __ptrace_may_access() returns 0 or -ERRNO.
+	 */
+	return (tsk && tsk->mm && tsk->mm->context.ctx_id != last_ctx_id &&
+			__ptrace_may_access(tsk, PTRACE_MODE_IBPB));
+}
+
 void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 			struct task_struct *tsk)
 {
@@ -262,18 +276,13 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 		 * one process from doing Spectre-v2 attacks on another.
 		 *
 		 * As an optimization, flush indirect branches only when
-		 * switching into processes that disable dumping. This
-		 * protects high value processes like gpg, without having
-		 * too high performance overhead. IBPB is *expensive*!
-		 *
-		 * This will not flush branches when switching into kernel
-		 * threads. It will also not flush if we switch to idle
-		 * thread and back to the same process. It will flush if we
-		 * switch to a different non-dumpable process.
+		 * switching into a processes that can't be ptrace by the
+		 * current one (as in such case, attacker has much more
+		 * convenient way how to tamper with the next process than
+		 * branch buffer poisoning).
 		 */
-		if (tsk && tsk->mm &&
-		    tsk->mm->context.ctx_id != last_ctx_id &&
-		    get_dumpable(tsk->mm) != SUID_DUMP_USER)
+		if (static_cpu_has(X86_FEATURE_USE_IBPB) &&
+				ibpb_needed(tsk, last_ctx_id))
 			indirect_branch_prediction_barrier();
 
 		if (IS_ENABLED(CONFIG_VMAP_STACK)) {
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 4f36431c380b..983d3f5545a8 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -64,12 +64,15 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
 #define PTRACE_MODE_NOAUDIT	0x04
 #define PTRACE_MODE_FSCREDS 0x08
 #define PTRACE_MODE_REALCREDS 0x10
+#define PTRACE_MODE_NOACCESS_CHK 0x20
 
 /* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */
 #define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)
 #define PTRACE_MODE_READ_REALCREDS (PTRACE_MODE_READ | PTRACE_MODE_REALCREDS)
 #define PTRACE_MODE_ATTACH_FSCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_FSCREDS)
 #define PTRACE_MODE_ATTACH_REALCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS)
+#define PTRACE_MODE_IBPB (PTRACE_MODE_ATTACH | PTRACE_MODE_NOAUDIT \
+			  | PTRACE_MODE_NOACCESS_CHK | PTRACE_MODE_REALCREDS)
 
 /**
  * ptrace_may_access - check whether the caller is permitted to access
@@ -86,6 +89,7 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
  * process_vm_writev or ptrace (and should use the real credentials).
  */
 extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
+extern int __ptrace_may_access(struct task_struct *task, unsigned int mode);
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 21fec73d45d4..5c5e7cb597cd 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -268,7 +268,7 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
 }
 
 /* Returns 0 on success, -errno on denial. */
-static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
+int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
 	struct mm_struct *mm;
@@ -316,7 +316,8 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	    gid_eq(caller_gid, tcred->sgid) &&
 	    gid_eq(caller_gid, tcred->gid))
 		goto ok;
-	if (ptrace_has_cap(tcred->user_ns, mode))
+	if (!(mode & PTRACE_MODE_NOACCESS_CHK) &&
+	     ptrace_has_cap(tcred->user_ns, mode))
 		goto ok;
 	rcu_read_unlock();
 	return -EPERM;
@@ -325,10 +326,13 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	mm = task->mm;
 	if (mm &&
 	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
-	     !ptrace_has_cap(mm->user_ns, mode)))
+	     ((mode & PTRACE_MODE_NOACCESS_CHK) ||
+	       !ptrace_has_cap(mm->user_ns, mode))))
 	    return -EPERM;
 
-	return security_ptrace_access_check(task, mode);
+	if (!(mode & PTRACE_MODE_NOACCESS_CHK))
+		return security_ptrace_access_check(task, mode);
+	return 0;
 }
 
 bool ptrace_may_access(struct task_struct *task, unsigned int mode)

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 1/2] x86/speculation: apply IBPB more strictly to avoid cross-process data leak

[Josh Poimboeuf]

On Thu, Sep 06, 2018 at 10:32:38AM +0200, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> 
> Currently, we are issuing IBPB only in cases when switching into a non-dumpable
> process, the rationale being to protect such 'important and security sensitive'
> processess (such as GPG) from data leak into a different userspace process via
> spectre v2.
> 
> This is however completely insufficient to provide proper userspace-to-userpace
> spectrev2 protection, as any process can poison branch buffers before being
> scheduled out, and the newly scheduled process immediately becomes spectrev2

"becomes a"

> victim.
> 
> In order to minimize the performance impact (for usecases that do require
> spectrev2 protection), issue the barrier only in cases when switching between
> processess where the victim can't be ptraced by the potential attacker (as in

"processes"

> such cases, the attacker doesn't have to bother with branch buffers at all).
> 
> Fixes: 18bf3c3ea8 ("x86/speculation: Use Indirect Branch Prediction Barrier in context switch")
> Originally-by: Tim Chen <tim.c.chen@linux.intel.com>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

STIBP is a feature provided by certain Intel ucodes / CPUs. This feature
(once enabled) prevents cross-hyperthread control of decisions made by
indirect branch predictors.

Enable this feature if

- the CPU is vulnerable to spectre v2
- the CPU supports SMT and has SMT siblings online
- spectre_v2 mitigation autoselection is enabled (default)

After some previous discussion, this patch leaves STIBP on all the time,
as wrmsr on crossing kernel boundary is a no-no. This could perhaps later
be a bit more optimized (like disabling it in NOHZ, experiment with
disabling it in idle, etc) if needed.

Note: the code could be made less awkward if it'd be guaranteed that STIBP
could be kept on on a primary thread with SMT sibling being offline, without
potentially imposing performance penalty. This doesn't seem to be defined
anywhere though, so let's better be safe then sorry and always flip STIBP
both on primary and sibling threads on hotplug transitions.

Cc: stable@vger.kernel.org
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

Hopefully properly threaded now, sorry for the duplicates. alpine 2.21
clearly has a bug that it sometimes eats Reply-to: header :/ Apologizes
for all the noise

 arch/x86/kernel/cpu/bugs.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/cpu.c               | 13 +++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 40bdaea97fe7..ba3df0a49a2e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -325,6 +325,56 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 	return cmd;
 }
 
+static bool stibp_needed(void)
+{
+	if (spectre_v2_enabled == SPECTRE_V2_NONE)
+		return false;
+
+	if (cpu_smt_control != CPU_SMT_ENABLED)
+		return false;
+
+	if (!boot_cpu_has(X86_FEATURE_STIBP))
+		return false;
+
+	return true;
+}
+
+/*
+ * The read-modify-write of the MSR doesn't need any race protection here,
+ * as we're running in atomic context.
+ */
+static void enable_stibp(void *info)
+{
+	u64 mask;
+	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
+	mask |= SPEC_CTRL_STIBP;
+	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
+}
+
+static void disable_stibp(void *info)
+{
+	u64 mask;
+	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
+	mask &= ~SPEC_CTRL_STIBP;
+	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
+}
+
+void arch_smt_enable_errata(void)
+{
+	if (stibp_needed()) {
+		pr_info("Spectre v2 cross-process SMT mitigation: Enabling STIBP\n");
+		on_each_cpu(enable_stibp, NULL, 1);
+	}
+}
+
+void arch_smt_disable_errata(void)
+{
+	if (stibp_needed()) {
+		pr_info("Spectre v2 cross-process SMT mitigation: Disabling STIBP\n");
+		on_each_cpu(disable_stibp, NULL, 1);
+	}
+}
+
 static void __init spectre_v2_select_mitigation(void)
 {
 	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
@@ -424,6 +474,9 @@ static void __init spectre_v2_select_mitigation(void)
 		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
 		pr_info("Enabling Restricted Speculation for firmware calls\n");
 	}
+
+	/* Enable STIBP on BP if needed */
+	arch_smt_enable_errata();
 }
 
 #undef pr_fmt
@@ -655,6 +708,16 @@ void x86_spec_ctrl_setup_ap(void)
 
 	if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
 		x86_amd_ssb_disable();
+
+	/*
+	 * If we are here during system bootup, enable STIBP.
+	 *
+	 * If we are here because of SMT hotplug, STIBP will be enabled by the
+	 * SMT control code (enabling here would not be sufficient, as it
+	 * needs to happen on primary threads as well).
+	 */
+	if (stibp_needed() && system_state < SYSTEM_RUNNING)
+		enable_stibp(NULL);
 }
 
 #undef pr_fmt
diff --git a/kernel/cpu.c b/kernel/cpu.c
index aa7fe85ad62e..d3613d546829 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2025,17 +2025,27 @@ static void cpuhp_online_cpu_device(unsigned int cpu)
 	kobject_uevent(&dev->kobj, KOBJ_ONLINE);
 }
 
+/*
+ * Architectures that need SMT-specific errata handling during SMT hotplug
+ * should override these.
+ */
+void __weak arch_smt_enable_errata(void) { };
+void __weak arch_smt_disable_errata(void) { };
+
 static int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
 {
 	int cpu, ret = 0;
 
 	cpu_maps_update_begin();
+	arch_smt_disable_errata();
 	for_each_online_cpu(cpu) {
 		if (topology_is_primary_thread(cpu))
 			continue;
 		ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
-		if (ret)
+		if (ret) {
+			arch_smt_enable_errata();
 			break;
+		}
 		/*
 		 * As this needs to hold the cpu maps lock it's impossible
 		 * to call device_offline() because that ends up calling
@@ -2073,6 +2083,7 @@ static int cpuhp_smt_enable(void)
 		/* See comment in cpuhp_smt_disable() */
 		cpuhp_online_cpu_device(cpu);
 	}
+	arch_smt_enable_errata();
 	cpu_maps_update_done();
 	return ret;
 }

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Josh Poimboeuf]

On Thu, Sep 06, 2018 at 10:33:39AM +0200, Jiri Kosina wrote:
> +/*
> + * The read-modify-write of the MSR doesn't need any race protection here,
> + * as we're running in atomic context.
> + */
> +static void enable_stibp(void *info)
> +{
> +	u64 mask;
> +	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
> +	mask |= SPEC_CTRL_STIBP;
> +	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
> +}
> +
> +static void disable_stibp(void *info)
> +{
> +	u64 mask;
> +	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
> +	mask &= ~SPEC_CTRL_STIBP;
> +	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
> +}
> +
> +void arch_smt_enable_errata(void)
> +{
> +	if (stibp_needed()) {
> +		pr_info("Spectre v2 cross-process SMT mitigation: Enabling STIBP\n");
> +		on_each_cpu(enable_stibp, NULL, 1);
> +	}
> +}

Shouldn't this add the STIBP bit to 'x86_spec_ctrl_base'?  Otherwise
won't it get overwritten in places like vmexit,
firmware_restrict_branch_speculation_end(), intel_set_ssb_state(), etc?

-- 
Josh

Re: [PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Thomas Gleixner]

On Thu, 6 Sep 2018, Jiri Kosina wrote:
> +/*
> + * The read-modify-write of the MSR doesn't need any race protection here,
> + * as we're running in atomic context.
> + */
> +static void enable_stibp(void *info)
> +{
> +	u64 mask;
> +	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
> +	mask |= SPEC_CTRL_STIBP;
> +	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
> +}

You need to write x86_spec_ctrl_base and you have to set/clear the STIBP
bit before issuing the IPIs, i.e. you only need a single function

static void update_specctrl_msr(void *unused)
{
	wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
}

If you don't update x86_spec_ctrl_base then any consecutive update of the
MSR will clear it. It's written in various nospec() functions and on
vmexit.

Right now the variable is ro_after_init because it's only modified during
init when the mitigations are selected. So that needs to be removed.

For the case at hand we could just rely on the hotplug serialization, but I
fear that's going to break sooner than later. I rather prefer to have some
local protection here right away.

void arch_smt_control(bool enable)
{
	mutex_lock(&spec_ctrl_mutex);
	if (enable)
		x86_spec_ctrl_base |= SPEC_CTRL_STIBP;
	else
		x86_spec_ctrl_base &= ~SPEC_CTRL_STIBP;

	on_each_cpu(update_specctrl_msr, NULL, 1);
	mutex_unlock(&spec_ctrl_mutex);
}

or something like that.

>  #undef pr_fmt
> @@ -655,6 +708,16 @@ void x86_spec_ctrl_setup_ap(void)
>  
>  	if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
>  		x86_amd_ssb_disable();
> +
> +	/*
> +	 * If we are here during system bootup, enable STIBP.
> +	 *
> +	 * If we are here because of SMT hotplug, STIBP will be enabled by the
> +	 * SMT control code (enabling here would not be sufficient, as it
> +	 * needs to happen on primary threads as well).
> +	 */
> +	if (stibp_needed() && system_state < SYSTEM_RUNNING)
> +		enable_stibp(NULL);

That's broken and pointless. If a CPU is hotplugged for whatever reason,
then it needs the update of the IA32_SPEC_CTRL msr and it's already there:

void x86_spec_ctrl_setup_ap(void)
{
        if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
                wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);

so again. x86_spec_ctrl_base needs the STIPB bit update when the
enable/disable changes.

> +/*
> + * Architectures that need SMT-specific errata handling during SMT hotplug
> + * should override these.
> + */
> +void __weak arch_smt_enable_errata(void) { };
> +void __weak arch_smt_disable_errata(void) { };
> +
>  static int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
>  {
>  	int cpu, ret = 0;
>  
>  	cpu_maps_update_begin();
> +	arch_smt_disable_errata();
>  	for_each_online_cpu(cpu) {
>  		if (topology_is_primary_thread(cpu))
>  			continue;
>  		ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
> -		if (ret)
> +		if (ret) {
> +			arch_smt_enable_errata();
>  			break;
> +		}

Why don't you do the disable_errata call _after_ the loop and only on
success? That's more logical and spares 50% IPIs

> @@ -2073,6 +2083,7 @@ static int cpuhp_smt_enable(void)
>                /* See comment in cpuhp_smt_disable() */
>                cpuhp_online_cpu_device(cpu);
>        }
> +       arch_smt_enable_errata();
>         cpu_maps_update_done();
>        return ret;

If you do that _before_ the loop then you spare 50% IPIs again because the
siblings will do the right thing via x86_spec_ctrl_base.

Thanks,

	tglx

Re: [PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Jiri Kosina]

On Fri, 7 Sep 2018, Thomas Gleixner wrote:

> > + * The read-modify-write of the MSR doesn't need any race protection here,
> > + * as we're running in atomic context.
> > + */
> > +static void enable_stibp(void *info)
> > +{
> > +	u64 mask;
> > +	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
> > +	mask |= SPEC_CTRL_STIBP;
> > +	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
> > +}
> 
> You need to write x86_spec_ctrl_base 

Agreed (Josh pointed that out as well) -- otherwise this gets lost during 
VMEXIT at least.

> >  #undef pr_fmt
> > @@ -655,6 +708,16 @@ void x86_spec_ctrl_setup_ap(void)
> >  
> >  	if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
> >  		x86_amd_ssb_disable();
> > +
> > +	/*
> > +	 * If we are here during system bootup, enable STIBP.
> > +	 *
> > +	 * If we are here because of SMT hotplug, STIBP will be enabled by the
> > +	 * SMT control code (enabling here would not be sufficient, as it
> > +	 * needs to happen on primary threads as well).
> > +	 */
> > +	if (stibp_needed() && system_state < SYSTEM_RUNNING)
> > +		enable_stibp(NULL);
> 
> That's broken and pointless. If a CPU is hotplugged for whatever reason,
> then it needs the update of the IA32_SPEC_CTRL msr and it's already there:
> 
> void x86_spec_ctrl_setup_ap(void)
> {
>         if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
>                 wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> 
> so again. x86_spec_ctrl_base needs the STIPB bit update when the
> enable/disable changes.

Agreed, but that's not sufficient. We need to update the MSR value on 
primary threads as well (which we do in sysfs store path), so this is 
merely an optimization so that we don't do it pointlessly twice on 
siblings.

> > +/*
> > + * Architectures that need SMT-specific errata handling during SMT hotplug
> > + * should override these.
> > + */
> > +void __weak arch_smt_enable_errata(void) { };
> > +void __weak arch_smt_disable_errata(void) { };
> > +
> >  static int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
> >  {
> >  	int cpu, ret = 0;
> >  
> >  	cpu_maps_update_begin();
> > +	arch_smt_disable_errata();
> >  	for_each_online_cpu(cpu) {
> >  		if (topology_is_primary_thread(cpu))
> >  			continue;
> >  		ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
> > -		if (ret)
> > +		if (ret) {
> > +			arch_smt_enable_errata();
> >  			break;
> > +		}
> 
> Why don't you do the disable_errata call _after_ the loop and only on
> success? That's more logical and spares 50% IPIs
> 
> > @@ -2073,6 +2083,7 @@ static int cpuhp_smt_enable(void)
> >                /* See comment in cpuhp_smt_disable() */
> >                cpuhp_online_cpu_device(cpu);
> >        }
> > +       arch_smt_enable_errata();
> >         cpu_maps_update_done();
> >        return ret;
> 
> If you do that _before_ the loop then you spare 50% IPIs again because the
> siblings will do the right thing via x86_spec_ctrl_base.

So I will go through the whole codepath again, but I fear your suggestion 
would not work -- see the check for cpu_smt_control in stibp_needed(). We 
need to see the old (or new, depending on the direction of the transition) 
value of cpu_smt_contol, which will break if we move 
arch_smt_enable_errata() (and thus the check).

But I am not 100% sure about this now, will double-check it tomorrow.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Thomas Gleixner]

On Fri, 7 Sep 2018, Jiri Kosina wrote:

> On Fri, 7 Sep 2018, Thomas Gleixner wrote:
> 
> > > + * The read-modify-write of the MSR doesn't need any race protection here,
> > > + * as we're running in atomic context.
> > > + */
> > > +static void enable_stibp(void *info)
> > > +{
> > > +	u64 mask;
> > > +	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
> > > +	mask |= SPEC_CTRL_STIBP;
> > > +	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
> > > +}
> > 
> > You need to write x86_spec_ctrl_base 
> 
> Agreed (Josh pointed that out as well) -- otherwise this gets lost during 
> VMEXIT at least.

It's not only virt. The firmware crap as well.

> > so again. x86_spec_ctrl_base needs the STIPB bit update when the
> > enable/disable changes.
> 
> Agreed, but that's not sufficient. We need to update the MSR value on 
> primary threads as well (which we do in sysfs store path), so this is 
> merely an optimization so that we don't do it pointlessly twice on 
> siblings.

No. It's an correctness issue. If after changing the SMT to enable a normal
hotplug operation happens then you need to update the MSR as well.

> > If you do that _before_ the loop then you spare 50% IPIs again because the
> > siblings will do the right thing via x86_spec_ctrl_base.
> 
> So I will go through the whole codepath again, but I fear your suggestion 
> would not work -- see the check for cpu_smt_control in stibp_needed(). We 
> need to see the old (or new, depending on the direction of the transition) 
> value of cpu_smt_contol, which will break if we move 
> arch_smt_enable_errata() (and thus the check).

That's bogus. The arch_smt_control(enable) function does not need to look
at the SMT control state at all. The caller hands the not yet populated new
state in and that's enough to make the decision whether to set or clear the
bit in x86_spec_ctrl_base.

Thanks,

	tglx

Re: [PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Jiri Kosina]

On Sat, 8 Sep 2018, Thomas Gleixner wrote:

> If after changing the SMT to enable a normal hotplug operation happens 
> then you need to update the MSR as well.

Ah, right you are, thanks. Will fix in v5.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Thomas Gleixner]

On Sat, 8 Sep 2018, Thomas Gleixner wrote:
> On Fri, 7 Sep 2018, Jiri Kosina wrote:
> > So I will go through the whole codepath again, but I fear your suggestion 
> > would not work -- see the check for cpu_smt_control in stibp_needed(). We 
> > need to see the old (or new, depending on the direction of the transition) 
> > value of cpu_smt_contol, which will break if we move 
> > arch_smt_enable_errata() (and thus the check).
> 
> That's bogus. The arch_smt_control(enable) function does not need to look
> at the SMT control state at all. The caller hands the not yet populated new
> state in and that's enough to make the decision whether to set or clear the
> bit in x86_spec_ctrl_base.

And thinking more about it, the function does not even need an argument.

smt_disable()

	res = shut_down_siblings();
	if (!res) {
		state = disabled;
		arch_smt_update();
	}

smt_enable()

	state = enabled;
	arch_smt_update();
	bringup_siblings();

So the update function has the correct state in both cases and then does:

	if (!cpu_is_trainwreck() || !cpu_has(stibp))
       		return;

	lock();
	ctrl = spec_ctrl_base;
	if (smt_disabled())
		ctrl &= ~STIPB;
	else
		ctrl |= STIPB;
	if (ctrl != spec_ctrl_base) {
		spec_ctrl_base = ctrl;
		on_each_cpu(write_spec_msr);
	}
	unlock();

See?

Thanks,

	tglx

[PATCH v4 2/2] x86/speculation: Enable cross-hyperthread spectre v2 STIBP mitigation

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

STIBP is a feature provided by certain Intel ucodes / CPUs. This feature
(once enabled) prevents cross-hyperthread control of decisions made by
indirect branch predictors.

Enable this feature if

- the CPU is vulnerable to spectre v2
- the CPU supports SMT and has SMT siblings online
- spectre_v2 mitigation autoselection is enabled (default)

After some previous discussion, this patch leaves STIBP on all the time,
as wrmsr on crossing kernel boundary is a no-no. This could perhaps later
be a bit more optimized (like disabling it in NOHZ, experiment with
disabling it in idle, etc) if needed.

Note: the code could be made less awkward if it'd be guaranteed that STIBP
could be kept on on a primary thread with SMT sibling being offline, without
potentially imposing performance penalty. This doesn't seem to be defined
anywhere though, so let's better be safe then sorry and always flip STIBP
both on primary and sibling threads on hotplug transitions.

Cc: stable@vger.kernel.org
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/kernel/cpu/bugs.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/cpu.c               | 13 +++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 40bdaea97fe7..ba3df0a49a2e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -325,6 +325,56 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 	return cmd;
 }
 
+static bool stibp_needed(void)
+{
+	if (spectre_v2_enabled == SPECTRE_V2_NONE)
+		return false;
+
+	if (cpu_smt_control != CPU_SMT_ENABLED)
+		return false;
+
+	if (!boot_cpu_has(X86_FEATURE_STIBP))
+		return false;
+
+	return true;
+}
+
+/*
+ * The read-modify-write of the MSR doesn't need any race protection here,
+ * as we're running in atomic context.
+ */
+static void enable_stibp(void *info)
+{
+	u64 mask;
+	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
+	mask |= SPEC_CTRL_STIBP;
+	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
+}
+
+static void disable_stibp(void *info)
+{
+	u64 mask;
+	rdmsrl(MSR_IA32_SPEC_CTRL, mask);
+	mask &= ~SPEC_CTRL_STIBP;
+	wrmsrl(MSR_IA32_SPEC_CTRL, mask);
+}
+
+void arch_smt_enable_errata(void)
+{
+	if (stibp_needed()) {
+		pr_info("Spectre v2 cross-process SMT mitigation: Enabling STIBP\n");
+		on_each_cpu(enable_stibp, NULL, 1);
+	}
+}
+
+void arch_smt_disable_errata(void)
+{
+	if (stibp_needed()) {
+		pr_info("Spectre v2 cross-process SMT mitigation: Disabling STIBP\n");
+		on_each_cpu(disable_stibp, NULL, 1);
+	}
+}
+
 static void __init spectre_v2_select_mitigation(void)
 {
 	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
@@ -424,6 +474,9 @@ static void __init spectre_v2_select_mitigation(void)
 		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
 		pr_info("Enabling Restricted Speculation for firmware calls\n");
 	}
+
+	/* Enable STIBP on BP if needed */
+	arch_smt_enable_errata();
 }
 
 #undef pr_fmt
@@ -655,6 +708,16 @@ void x86_spec_ctrl_setup_ap(void)
 
 	if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
 		x86_amd_ssb_disable();
+
+	/*
+	 * If we are here during system bootup, enable STIBP.
+	 *
+	 * If we are here because of SMT hotplug, STIBP will be enabled by the
+	 * SMT control code (enabling here would not be sufficient, as it
+	 * needs to happen on primary threads as well).
+	 */
+	if (stibp_needed() && system_state < SYSTEM_RUNNING)
+		enable_stibp(NULL);
 }
 
 #undef pr_fmt
diff --git a/kernel/cpu.c b/kernel/cpu.c
index aa7fe85ad62e..d3613d546829 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2025,17 +2025,27 @@ static void cpuhp_online_cpu_device(unsigned int cpu)
 	kobject_uevent(&dev->kobj, KOBJ_ONLINE);
 }
 
+/*
+ * Architectures that need SMT-specific errata handling during SMT hotplug
+ * should override these.
+ */
+void __weak arch_smt_enable_errata(void) { };
+void __weak arch_smt_disable_errata(void) { };
+
 static int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
 {
 	int cpu, ret = 0;
 
 	cpu_maps_update_begin();
+	arch_smt_disable_errata();
 	for_each_online_cpu(cpu) {
 		if (topology_is_primary_thread(cpu))
 			continue;
 		ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
-		if (ret)
+		if (ret) {
+			arch_smt_enable_errata();
 			break;
+		}
 		/*
 		 * As this needs to hold the cpu maps lock it's impossible
 		 * to call device_offline() because that ends up calling
@@ -2073,6 +2083,7 @@ static int cpuhp_smt_enable(void)
 		/* See comment in cpuhp_smt_disable() */
 		cpuhp_online_cpu_device(cpu);
 	}
+	arch_smt_enable_errata();
 	cpu_maps_update_done();
 	return ret;
 }

-- 
Jiri Kosina
SUSE Labs