[Patch v5 00/16] Provide task property based options to enable Spectre v2 userspace-userspace protection

[Patch v5 00/16] Provide task property based options to enable Spectre v2 userspace-userspace protection

[Tim Chen]

The previous version of this series had a patch to apply TIF_STIBP updates
to all threads affected by a dumpability change, and keeping all the CPUs'
SPEC CTRL MSRs in sync with running task's TIF_STIBP.

However, this feature adds much overhead and complexities
for little gain.  Normally a task making uid/gid or prctl dumpability change
will do so before it starts spawning threads.

So in this version, the TIF_STIBP associated with dumpability change
is only applied to the task that makes the change, and not extended
to its associated threads.

Thomas also pointed out that new cpu_smt_enabled staic key is
created under CONFIG_HOTPLUG_SMT and currently applies only to x86.
So cpu_smt_enabled cannot replace sched_smt_present key which needs to
be used by all architectures, unless the cpu_smt_control setting logic
is moved out of CONFIG_HOTPLUG_SMT.  So I dropped the patch replacing
sched_smt_present with cpu_smt_enabled.

I've also moved the TIF flags re-organization patch to the end
of the series to make it easier for backporting to stable kernels
without needing to reorganize the TIF flags.

Thomas, can you consider this series to be merged to 4.20-rc along
with Jiri's changes on STIBP?

Thanks.

Tim

Patch 1 to 3 are clean up patches.
Patch 4 and 5 disable STIBP for enhacned IBRS.
Patch 6 to 9 reorganize and clean up the code without affecting
 functionality for easier modification later.
Patch 10 introduces the STIBP flag on a task to dynamically
 enable STIBP for that task.
Patch 11 introduces different modes to protect a
 task against Spectre v2 user space attack.
Patch 12 adds prctl interface to turn on Spectre v2 user mode defenses on a task. 
Patch 13-14 add Spectre v2 defenses for non-dumpable tasks.
Patch 15-16 reorganizes the TIF flags, and can be dropped without affecting this series 

Changes:
v5:
1. Drop patch to extend TIF_STIBP changes to all related threads on 
a task's dumpabibility change.
2. Drop patch to replace sched_smt_present with cpu_smt_enabled.
3. Drop export of cpu_smt_control in kernel/cpu.c and replace external
usages of cpu_smt_control with cpu_smt_enabled.
4. Rebase patch series on 4.20-rc2.

v4:
1. Extend STIBP update to all threads of a process changing
it dumpability.
2. Add logic to update SPEC_CTRL MSR on a remote CPU when TIF flags
affecting speculation changes for task running on the remote CPU.
3. Regroup x86 TIF_* flags according to their functions.
4. Various code clean up.

v3:
1. Add logic to skip STIBP when Enhanced IBRS is used.
2. Break up v2 patches into smaller logical patches. 
3. Fix bug in arch_set_dumpable that did not update SPEC_CTRL
MSR right away when according to task's STIBP flag clearing which
caused SITBP to be left on.
4. Various code clean up. 

v2:
1. Extend per process STIBP to AMD cpus
2. Add prctl option to control per process indirect branch speculation
3. Bug fixes and cleanups 

Jiri's patchset to harden Spectre v2 user space mitigation makes IBPB
and STIBP in use for Spectre v2 mitigation on all processes.  IBPB will
be issued for switching to an application that's not ptraceable by the
previous application and STIBP will be always turned on.

However, leaving STIBP on all the time is expensive for certain
applications that have frequent indirect branches. One such application
is perlbench in the SpecInt Rate 2006 test suite which shows a
21% reduction in throughput.
There're also reports of drop in performance on Python and PHP benchmarks:
https://www.phoronix.com/scan.php?page=article&item=linux-420-bisect&num=2

Other applications like bzip2 with minimal indirct branches have
only a 0.7% reduction in throughput. IBPB will also impose
overhead during context switches.

Users may not wish to incur performance overhead from IBPB and STIBP for
general non security sensitive processes and use these mitigations only
for security sensitive processes.

This patchset provides a process property based lite protection mode.
In this mode, IBPB and STIBP mitigation are applied only to security
sensitive non-dumpable processes and processes that users want to protect
by having indirect branch speculation disabled via PRCTL.  So the overhead
from IBPB and STIBP are avoided for low security processes that don't
require extra protection.
Tim Chen (16):
  x86/speculation: Clean up spectre_v2_parse_cmdline()
  x86/speculation: Remove unnecessary ret variable in cpu_show_common()
  x86/speculation: Reorganize cpu_show_common()
  x86/speculation: Add X86_FEATURE_USE_IBRS_ENHANCED
  x86/speculation: Disable STIBP when enhanced IBRS is in use
  x86/speculation: Rename SSBD update functions
  x86/speculation: Reorganize speculation control MSRs update
  smt: Create cpu_smt_enabled static key for SMT specific code
  x86/smt: Convert cpu_smt_control check to cpu_smt_enabled static key
  x86/speculation: Turn on or off STIBP according to a task's TIF_STIBP
  x86/speculation: Add Spectre v2 app to app protection modes
  x86/speculation: Create PRCTL interface to restrict indirect branch
    speculation
  security: Update speculation restriction of a process when modifying
    its dumpability
  x86/speculation: Use STIBP to restrict speculation on non-dumpable
    task
  x86/speculation: Update comment on TIF_SSBD
  x86: Group thread info flags by functionality

 Documentation/admin-guide/kernel-parameters.txt |  21 ++
 Documentation/userspace-api/spec_ctrl.rst       |   9 +
 arch/x86/include/asm/cpufeatures.h              |   1 +
 arch/x86/include/asm/msr-index.h                |   6 +-
 arch/x86/include/asm/nospec-branch.h            |   9 +
 arch/x86/include/asm/spec-ctrl.h                |  18 +-
 arch/x86/include/asm/thread_info.h              |  98 ++++---
 arch/x86/kernel/cpu/bugs.c                      | 323 +++++++++++++++++++++---
 arch/x86/kernel/process.c                       |  58 ++++-
 arch/x86/kvm/vmx.c                              |   2 +-
 arch/x86/mm/tlb.c                               |  23 +-
 fs/exec.c                                       |   3 +
 include/linux/cpu.h                             |  12 +-
 include/linux/sched.h                           |   9 +
 include/uapi/linux/prctl.h                      |   1 +
 kernel/cpu.c                                    |  25 +-
 kernel/cred.c                                   |   5 +-
 kernel/sys.c                                    |   7 +
 tools/include/uapi/linux/prctl.h                |   1 +
 19 files changed, 520 insertions(+), 111 deletions(-)

-- 
2.9.4

[Patch v5 01/16] x86/speculation: Clean up spectre_v2_parse_cmdline()

[Tim Chen]

Remove unnecessary else statement in spectre_v2_parse_cmdline()
to save a indentation level.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index c37e66e..5ac7070 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -283,22 +283,21 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 
 	if (cmdline_find_option_bool(boot_command_line, "nospectre_v2"))
 		return SPECTRE_V2_CMD_NONE;
-	else {
-		ret = cmdline_find_option(boot_command_line, "spectre_v2", arg, sizeof(arg));
-		if (ret < 0)
-			return SPECTRE_V2_CMD_AUTO;
 
-		for (i = 0; i < ARRAY_SIZE(mitigation_options); i++) {
-			if (!match_option(arg, ret, mitigation_options[i].option))
-				continue;
-			cmd = mitigation_options[i].cmd;
-			break;
-		}
+	ret = cmdline_find_option(boot_command_line, "spectre_v2", arg, sizeof(arg));
+	if (ret < 0)
+		return SPECTRE_V2_CMD_AUTO;
 
-		if (i >= ARRAY_SIZE(mitigation_options)) {
-			pr_err("unknown option (%s). Switching to AUTO select\n", arg);
-			return SPECTRE_V2_CMD_AUTO;
-		}
+	for (i = 0; i < ARRAY_SIZE(mitigation_options); i++) {
+		if (!match_option(arg, ret, mitigation_options[i].option))
+			continue;
+		cmd = mitigation_options[i].cmd;
+		break;
+	}
+
+	if (i >= ARRAY_SIZE(mitigation_options)) {
+		pr_err("unknown option (%s). Switching to AUTO select\n", arg);
+		return SPECTRE_V2_CMD_AUTO;
 	}
 
 	if ((cmd == SPECTRE_V2_CMD_RETPOLINE ||
-- 
2.9.4

[Patch v5 02/16] x86/speculation: Remove unnecessary ret variable in cpu_show_common()

[Tim Chen]

Remove unnecessary ret variable in cpu_show_common() to make the
code more concise.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 5ac7070..84e3579 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -856,8 +856,6 @@ static ssize_t l1tf_show_state(char *buf)
 static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
 			       char *buf, unsigned int bug)
 {
-	int ret;
-
 	if (!boot_cpu_has_bug(bug))
 		return sprintf(buf, "Not affected\n");
 
@@ -875,13 +873,12 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
 		return sprintf(buf, "Mitigation: __user pointer sanitization\n");
 
 	case X86_BUG_SPECTRE_V2:
-		ret = sprintf(buf, "%s%s%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
+		return sprintf(buf, "%s%s%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
 			       boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
 			       boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
 			       (x86_spec_ctrl_base & SPEC_CTRL_STIBP) ? ", STIBP" : "",
 			       boot_cpu_has(X86_FEATURE_RSB_CTXSW) ? ", RSB filling" : "",
 			       spectre_v2_module_string());
-		return ret;
 
 	case X86_BUG_SPEC_STORE_BYPASS:
 		return sprintf(buf, "%s\n", ssb_strings[ssb_mode]);
-- 
2.9.4

[Patch v5 03/16] x86/speculation: Reorganize cpu_show_common()

[Tim Chen]

The Spectre V2 printout in cpu_show_common() handles conditionals for the
various mitigation methods directly in the sprintf() argument list. That's
hard to read and will become unreadable if more complex decisions need to
be made for a particular method.

Move the conditionals for STIBP and IBPB string selection into helper
functions, so they can be extended later on.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 84e3579..91a754a 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -853,6 +853,22 @@ static ssize_t l1tf_show_state(char *buf)
 }
 #endif
 
+static char *stibp_state(void)
+{
+	if (x86_spec_ctrl_base & SPEC_CTRL_STIBP)
+		return ", STIBP";
+	else
+		return "";
+}
+
+static char *ibpb_state(void)
+{
+	if (boot_cpu_has(X86_FEATURE_USE_IBPB))
+		return ", IBPB";
+	else
+		return "";
+}
+
 static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
 			       char *buf, unsigned int bug)
 {
@@ -874,9 +890,9 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
 
 	case X86_BUG_SPECTRE_V2:
 		return sprintf(buf, "%s%s%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
-			       boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
+			       ibpb_state(),
 			       boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
-			       (x86_spec_ctrl_base & SPEC_CTRL_STIBP) ? ", STIBP" : "",
+			       stibp_state(),
 			       boot_cpu_has(X86_FEATURE_RSB_CTXSW) ? ", RSB filling" : "",
 			       spectre_v2_module_string());
 
-- 
2.9.4

[Patch v5 04/16] x86/speculation: Add X86_FEATURE_USE_IBRS_ENHANCED

[Tim Chen]

STIBP is not needed when enhanced IBRS is used for Spectre V2 mitigation.
A CPU feature flag to indicate that enhanced IBRS is used will be handy
for skipping STIBP for this case.

Add X86_FEATURE_USE_IBRS_ENHANCED feature bit to indicate
enhanced IBRS is used for Spectre V2 mitigation.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/bugs.c         | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 28c4a50..fe8e064 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -221,6 +221,7 @@
 #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
 #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
 #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
+#define X86_FEATURE_USE_IBRS_ENHANCED	( 7*32+31) /* "" Enhanced IBRS enabled */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 91a754a..3a6f13b 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -387,6 +387,7 @@ static void __init spectre_v2_select_mitigation(void)
 			/* Force it so VMEXIT will restore correctly */
 			x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
 			wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+			setup_force_cpu_cap(X86_FEATURE_USE_IBRS_ENHANCED);
 			goto specv2_set_mode;
 		}
 		if (IS_ENABLED(CONFIG_RETPOLINE))
-- 
2.9.4

[Patch v5 05/16] x86/speculation: Disable STIBP when enhanced IBRS is in use

[Tim Chen]

If enhanced IBRS is engaged, STIBP is redundant in mitigating Spectre
v2 user space exploits from hyperthread sibling.

Disable STIBP when enhanced IBRS is used.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 3a6f13b..5c0eb2f 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -325,9 +325,17 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 
 static bool stibp_needed(void)
 {
+	/*
+	 * Determine if STIBP should be always on.
+	 * Using enhanced IBRS makes using STIBP unnecessary.
+	 */
+
 	if (spectre_v2_enabled == SPECTRE_V2_NONE)
 		return false;
 
+	if (static_cpu_has(X86_FEATURE_USE_IBRS_ENHANCED))
+		return false;
+
 	if (!boot_cpu_has(X86_FEATURE_STIBP))
 		return false;
 
@@ -856,6 +864,9 @@ static ssize_t l1tf_show_state(char *buf)
 
 static char *stibp_state(void)
 {
+	if (spectre_v2_enabled == SPECTRE_V2_IBRS_ENHANCED)
+		return "";
+
 	if (x86_spec_ctrl_base & SPEC_CTRL_STIBP)
 		return ", STIBP";
 	else
-- 
2.9.4

[Patch v5 06/16] x86/speculation: Rename SSBD update functions

[Tim Chen]

During context switch, the SSBD bit in SPEC_CTRL MSR is updated according
to changes in TIF_SSBD flag in the current and next running task.
Currently, only the bit controlling speculative store in SPEC_CTRL MSR
is updated and the related update functions all have "speculative_store"
or "ssb" in their names.

In later patches, other bits controlling STIBP in SPEC_CTRL MSR need
to be updated.  The SPEC_CTRL MSR update functions should get rid of the
speculative store names as they will no longer be limited to SSBD update.

Rename the "speculative_store*" functions to a more generic name.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/include/asm/spec-ctrl.h |  6 +++---
 arch/x86/kernel/cpu/bugs.c       |  4 ++--
 arch/x86/kernel/process.c        | 12 ++++++------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/spec-ctrl.h b/arch/x86/include/asm/spec-ctrl.h
index ae7c2c5..8e2f841 100644
--- a/arch/x86/include/asm/spec-ctrl.h
+++ b/arch/x86/include/asm/spec-ctrl.h
@@ -70,11 +70,11 @@ extern void speculative_store_bypass_ht_init(void);
 static inline void speculative_store_bypass_ht_init(void) { }
 #endif
 
-extern void speculative_store_bypass_update(unsigned long tif);
+extern void speculation_ctrl_update(unsigned long tif);
 
-static inline void speculative_store_bypass_update_current(void)
+static inline void speculation_ctrl_update_current(void)
 {
-	speculative_store_bypass_update(current_thread_info()->flags);
+	speculation_ctrl_update(current_thread_info()->flags);
 }
 
 #endif
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 5c0eb2f..e4cfc4a 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -202,7 +202,7 @@ x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool setguest)
 		tif = setguest ? ssbd_spec_ctrl_to_tif(guestval) :
 				 ssbd_spec_ctrl_to_tif(hostval);
 
-		speculative_store_bypass_update(tif);
+		speculation_ctrl_update(tif);
 	}
 }
 EXPORT_SYMBOL_GPL(x86_virt_spec_ctrl);
@@ -646,7 +646,7 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
 	 * mitigation until it is next scheduled.
 	 */
 	if (task == current && update)
-		speculative_store_bypass_update_current();
+		speculation_ctrl_update_current();
 
 	return 0;
 }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c93fcfd..8aa4960 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -395,27 +395,27 @@ static __always_inline void amd_set_ssb_virt_state(unsigned long tifn)
 	wrmsrl(MSR_AMD64_VIRT_SPEC_CTRL, ssbd_tif_to_spec_ctrl(tifn));
 }
 
-static __always_inline void intel_set_ssb_state(unsigned long tifn)
+static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
 {
 	u64 msr = x86_spec_ctrl_base | ssbd_tif_to_spec_ctrl(tifn);
 
 	wrmsrl(MSR_IA32_SPEC_CTRL, msr);
 }
 
-static __always_inline void __speculative_store_bypass_update(unsigned long tifn)
+static __always_inline void __speculation_ctrl_update(unsigned long tifn)
 {
 	if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
 		amd_set_ssb_virt_state(tifn);
 	else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
 		amd_set_core_ssb_state(tifn);
 	else
-		intel_set_ssb_state(tifn);
+		spec_ctrl_update_msr(tifn);
 }
 
-void speculative_store_bypass_update(unsigned long tif)
+void speculation_ctrl_update(unsigned long tif)
 {
 	preempt_disable();
-	__speculative_store_bypass_update(tif);
+	__speculation_ctrl_update(tif);
 	preempt_enable();
 }
 
@@ -452,7 +452,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
 
 	if ((tifp ^ tifn) & _TIF_SSBD)
-		__speculative_store_bypass_update(tifn);
+		__speculation_ctrl_update(tifn);
 }
 
 /*
-- 
2.9.4

[Patch v5 07/16] x86/speculation: Reorganize speculation control MSRs update

[Tim Chen]

The logic to detect whether there's a change in the previous
and next task's flag relevant to update speculation control
MSRs are spread out across multiple functions.

Consolidate all checks needed for updating speculation
control MSRs to __speculation_ctrl_update().

This makes it easy to pick the right speculation control MSR,
and the bits in the MSR that needs updating based on
TIF flags changes.

Originally-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/process.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 8aa4960..74bef48 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -397,25 +397,48 @@ static __always_inline void amd_set_ssb_virt_state(unsigned long tifn)
 
 static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
 {
-	u64 msr = x86_spec_ctrl_base | ssbd_tif_to_spec_ctrl(tifn);
+	u64 msr = x86_spec_ctrl_base;
+
+	/*
+	 * If X86_FEATURE_SSBD is not set, the SSBD
+	 * bit is not to be touched.
+	 */
+	if (static_cpu_has(X86_FEATURE_SSBD))
+		msr |= ssbd_tif_to_spec_ctrl(tifn);
 
 	wrmsrl(MSR_IA32_SPEC_CTRL, msr);
 }
 
-static __always_inline void __speculation_ctrl_update(unsigned long tifn)
-{
-	if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
-		amd_set_ssb_virt_state(tifn);
-	else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
-		amd_set_core_ssb_state(tifn);
-	else
+/*
+ * Update the MSRs managing speculation control during context switch.
+ *
+ * tifp: previous task's thread flags
+ * tifn: next task's thread flags
+ */
+static __always_inline void __speculation_ctrl_update(unsigned long tifp,
+						      unsigned long tifn)
+{
+	bool updmsr = false;
+
+	/* If TIF_SSBD is different, select the proper mitigation method */
+	if ((tifp ^ tifn) & _TIF_SSBD) {
+		if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
+			amd_set_ssb_virt_state(tifn);
+		else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
+			amd_set_core_ssb_state(tifn);
+		else if (static_cpu_has(X86_FEATURE_SSBD))
+			updmsr  = true;
+	}
+
+	if (updmsr)
 		spec_ctrl_update_msr(tifn);
 }
 
 void speculation_ctrl_update(unsigned long tif)
 {
+	/* Forced update. Make sure all relevant TIF flags are different */
 	preempt_disable();
-	__speculation_ctrl_update(tif);
+	__speculation_ctrl_update(~tif, tif);
 	preempt_enable();
 }
 
@@ -451,8 +474,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	if ((tifp ^ tifn) & _TIF_NOCPUID)
 		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
 
-	if ((tifp ^ tifn) & _TIF_SSBD)
-		__speculation_ctrl_update(tifn);
+	__speculation_ctrl_update(tifp, tifn);
 }
 
 /*
-- 
2.9.4

[Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Tim Chen]

In later code, STIBP will be turned on/off in the context switch code
path when SMT is enabled.  Checks for SMT is best
avoided on such hot paths.

Create cpu_smt_enabled static key to turn on such SMT specific code
statically.

This key is set in code under hot plug, so it is limited in
scope to architecture supporting CPU hotplug, like x86.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 include/linux/cpu.h |  1 +
 kernel/cpu.c        | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 218df7f..b54f085 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -188,5 +188,6 @@ static inline void cpu_smt_disable(bool force) { }
 static inline void cpu_smt_check_topology_early(void) { }
 static inline void cpu_smt_check_topology(void) { }
 #endif
+DECLARE_STATIC_KEY_TRUE(cpu_smt_enabled);
 
 #endif /* _LINUX_CPU_H_ */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 3c7f3b4..e216154 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -370,6 +370,8 @@ static void lockdep_release_cpus_lock(void)
 #ifdef CONFIG_HOTPLUG_SMT
 enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
 EXPORT_SYMBOL_GPL(cpu_smt_control);
+DEFINE_STATIC_KEY_TRUE(cpu_smt_enabled);
+EXPORT_SYMBOL_GPL(cpu_smt_enabled);
 
 static bool cpu_smt_available __read_mostly;
 
@@ -386,6 +388,7 @@ void __init cpu_smt_disable(bool force)
 		pr_info("SMT: disabled\n");
 		cpu_smt_control = CPU_SMT_DISABLED;
 	}
+	static_branch_disable(&cpu_smt_enabled);
 }
 
 /*
@@ -395,8 +398,10 @@ void __init cpu_smt_disable(bool force)
  */
 void __init cpu_smt_check_topology_early(void)
 {
-	if (!topology_smt_supported())
+	if (!topology_smt_supported()) {
 		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
+		static_branch_disable(&cpu_smt_enabled);
+	}
 }
 
 /*
@@ -408,8 +413,10 @@ void __init cpu_smt_check_topology_early(void)
  */
 void __init cpu_smt_check_topology(void)
 {
-	if (!cpu_smt_available)
+	if (!cpu_smt_available) {
 		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
+		static_branch_disable(&cpu_smt_enabled);
+	}
 }
 
 static int __init smt_cmdline_disable(char *str)
@@ -2101,6 +2108,7 @@ static int cpuhp_smt_enable(void)
 
 	cpu_maps_update_begin();
 	cpu_smt_control = CPU_SMT_ENABLED;
+	static_branch_enable(&cpu_smt_enabled);
 	arch_smt_update();
 	for_each_present_cpu(cpu) {
 		/* Skip online CPUs and CPUs on offline nodes */
-- 
2.9.4

Re: [Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Thomas Gleixner]

On Fri, 16 Nov 2018, Tim Chen wrote:
> In later code, STIBP will be turned on/off in the context switch code
> path when SMT is enabled.  Checks for SMT is best
> avoided on such hot paths.
> 
> Create cpu_smt_enabled static key to turn on such SMT specific code
> statically.
> 
> This key is set in code under hot plug, so it is limited in
> scope to architecture supporting CPU hotplug, like x86.

The key is enabled by default and its scope has nothing to do with CPU
hotplug. It depends on CONFIG_HOTPLUG_SMT which is the extra SMT control
code which is currently only enabled on x86.

> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 218df7f..b54f085 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -188,5 +188,6 @@ static inline void cpu_smt_disable(bool force) { }
>  static inline void cpu_smt_check_topology_early(void) { }
>  static inline void cpu_smt_check_topology(void) { }
>  #endif
> +DECLARE_STATIC_KEY_TRUE(cpu_smt_enabled);

And here it is declared unconditionally which allows code to use it despite
CONFIG_HOTPLUG_SMT=n and subsequently breaks the build.
  
Thanks,

	tglx

Re: [Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Tim Chen]

On 11/19/2018 04:58 AM, Thomas Gleixner wrote:

>> +DECLARE_STATIC_KEY_TRUE(cpu_smt_enabled);
> 
> And here it is declared unconditionally which allows code to use it despite
> CONFIG_HOTPLUG_SMT=n and subsequently breaks the build.

Will put this under #ifdef CONFIG_HOTPLUG_SMT

Tim

Re: [Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Peter Zijlstra]

On Fri, Nov 16, 2018 at 05:53:51PM -0800, Tim Chen wrote:
> In later code, STIBP will be turned on/off in the context switch code
> path when SMT is enabled.  Checks for SMT is best
> avoided on such hot paths.
> 
> Create cpu_smt_enabled static key to turn on such SMT specific code
> statically.

AFAICT this patch only follows the SMT control knob but not the actual
topology state.

And, as I previously wrote, we already have sched_smt_present, which is
supposed to do much the same.

All you need is the below to make it accurately track the topology.

---
Subject: sched/smt: Make sched_smt_present track topology

Currently the sched_smt_present static key is only enabled when we
encounter SMT topology. However there is demand to also disable the key
when the topology changes such that there is no SMT present anymore.

Implement this by making the key count the number of cores that have SMT
enabled.

In particular, the SMT topology bits are set before we enable
interrrupts and similarly, are cleared after we disable interrupts for
the last time and die.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/core.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f12225f26b70..77e552928c67 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5738,15 +5738,10 @@ int sched_cpu_activate(unsigned int cpu)
 
 #ifdef CONFIG_SCHED_SMT
 	/*
-	 * The sched_smt_present static key needs to be evaluated on every
-	 * hotplug event because at boot time SMT might be disabled when
-	 * the number of booted CPUs is limited.
-	 *
-	 * If then later a sibling gets hotplugged, then the key would stay
-	 * off and SMT scheduling would never be functional.
+	 * When going up, increment the number of cores with SMT present.
 	 */
-	if (cpumask_weight(cpu_smt_mask(cpu)) > 1)
-		static_branch_enable_cpuslocked(&sched_smt_present);
+	if (cpumask_weight(cpu_smt_mask(cpu)) == 2)
+		static_branch_inc_cpuslocked(&sched_smt_present);
 #endif
 	set_cpu_active(cpu, true);
 
@@ -5790,6 +5785,14 @@ int sched_cpu_deactivate(unsigned int cpu)
 	 */
 	synchronize_rcu_mult(call_rcu, call_rcu_sched);
 
+#ifdef CONFIG_SCHED_SMT
+	/*
+	 * When going down, decrement the number of cores with SMT present.
+	 */
+	if (cpumask_weight(cpu_smt_mask(cpu)) == 2)
+		static_branch_dec_cpuslocked(&sched_smt_present);
+#endif
+
 	if (!sched_smp_initialized)
 		return 0;
 

Re: [Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Tim Chen]

On 11/19/2018 06:57 AM, Peter Zijlstra wrote:
> On Fri, Nov 16, 2018 at 05:53:51PM -0800, Tim Chen wrote:
>> In later code, STIBP will be turned on/off in the context switch code
>> path when SMT is enabled.  Checks for SMT is best
>> avoided on such hot paths.
>>
>> Create cpu_smt_enabled static key to turn on such SMT specific code
>> statically.
> 
> AFAICT this patch only follows the SMT control knob but not the actual
> topology state.
> 
> And, as I previously wrote, we already have sched_smt_present, which is
> supposed to do much the same.
> 
> All you need is the below to make it accurately track the topology.
> 
> ---
> Subject: sched/smt: Make sched_smt_present track topology
> 
> Currently the sched_smt_present static key is only enabled when we
> encounter SMT topology. However there is demand to also disable the key
> when the topology changes such that there is no SMT present anymore.
> 
> Implement this by making the key count the number of cores that have SMT
> enabled.
> 
> In particular, the SMT topology bits are set before we enable
> interrrupts and similarly, are cleared after we disable interrupts for
> the last time and die.


Peter & Thomas,

Any objection if I export sched_smt_present after including
Peter's patch and use it in spec_ctrl_update_msr instead.

Something like this?

Tim

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 943e90d..62fc3af 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -410,8 +410,7 @@ static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
 	 * Need STIBP defense against Spectre v2 attack
 	 * if SMT is in use and enhanced IBRS is unsupported.
 	 */
-	if (static_branch_likely(&cpu_smt_enabled) &&
-	    !static_cpu_has(X86_FEATURE_USE_IBRS_ENHANCED))
+	if (cpu_smt_present() && !static_cpu_has(X86_FEATURE_USE_IBRS_ENHANCED))
 		msr |= stibp_tif_to_spec_ctrl(tifn);
 
 	wrmsrl(MSR_IA32_SPEC_CTRL, msr);
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 3d90155..e3d985e 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -68,6 +68,27 @@ struct device *cpu_device_create(struct device *parent, void *drvdata,
 extern ssize_t arch_cpu_release(const char *, size_t);
 #endif
 
+#ifdef CONFIG_SCHED_SMT
+
+extern struct static_key_false sched_smt_present;
+
+static inline bool cpu_smt_present(void)
+{
+	if (static_branch_unlikely(&sched_smt_present))
+		return true;
+	else
+		return false;
+}
+
+#else
+
+static inline bool cpu_smt_present(void)
+{
+	return false;
+}
+
+#endif
+
 /*
  * These states are not related to the core CPU hotplug mechanism. They are
  * used by various (sub)architectures to track internal state
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 618577f..e1e3f09 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -937,8 +937,6 @@ static inline int cpu_of(struct rq *rq)
 
 #ifdef CONFIG_SCHED_SMT
 
-extern struct static_key_false sched_smt_present;
-
 extern void __update_idle_core(struct rq *rq);
 
 static inline void update_idle_core(struct rq *rq)

Re: [Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Thomas Gleixner]

Tim,

On Mon, 19 Nov 2018, Tim Chen wrote:
> On 11/19/2018 06:57 AM, Peter Zijlstra wrote:
> > In particular, the SMT topology bits are set before we enable
> > interrrupts and similarly, are cleared after we disable interrupts for
> > the last time and die.
> 
> 
> Peter & Thomas,
> 
> Any objection if I export sched_smt_present after including
> Peter's patch and use it in spec_ctrl_update_msr instead.
> 
> Something like this?

>  
> +#ifdef CONFIG_SCHED_SMT
> +
> +extern struct static_key_false sched_smt_present;
> +
> +static inline bool cpu_smt_present(void)
> +{
> +	if (static_branch_unlikely(&sched_smt_present))
> +		return true;
> +	else
> +		return false;

What's wrong with

       return static_branch_unlikely(&sched_smt_present);

???

But that's just a stylistic nitpick. The real issue is that you prevent the
mitigation when CONFIG_SCHED_SMT=n.

Thanks,

	tglx

Re: [Patch v5 08/16] smt: Create cpu_smt_enabled static key for SMT specific code

[Tim Chen]

Thomas,


>>  
>> +#ifdef CONFIG_SCHED_SMT
>> +
>> +extern struct static_key_false sched_smt_present;
>> +
>> +static inline bool cpu_smt_present(void)
>> +{
>> +	if (static_branch_unlikely(&sched_smt_present))
>> +		return true;
>> +	else
>> +		return false;
> 
> What's wrong with
> 
>        return static_branch_unlikely(&sched_smt_present);
> 
> ???
> 
> But that's just a stylistic nitpick. The real issue is that you prevent the
> mitigation when CONFIG_SCHED_SMT=n.
> 

Right. The sched_smt_present is just a scheduler construct and the sibling
cpu will still be brought online.  Scratch this.

Tim

[Patch v5 09/16] x86/smt: Convert cpu_smt_control check to cpu_smt_enabled static key

[Tim Chen]

The checks to cpu_smt_control outside of kernel/cpu.c can be converted
to use cpu_smt_enabled key to run SMT specific code.

Save the export of cpu_smt_control and convert usage of cpu_smt_control
to cpu_smt_enabled outside of kernel/cpu.c.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 21 +++++++++++++++------
 arch/x86/kvm/vmx.c         |  2 +-
 include/linux/cpu.h        |  8 --------
 kernel/cpu.c               |  9 ++++++++-
 4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index e4cfc4a..6e1b910 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -356,15 +356,16 @@ void arch_smt_update(void)
 
 	mutex_lock(&spec_ctrl_mutex);
 	mask = x86_spec_ctrl_base;
-	if (cpu_smt_control == CPU_SMT_ENABLED)
+	if (static_branch_likely(&cpu_smt_enabled))
 		mask |= SPEC_CTRL_STIBP;
 	else
 		mask &= ~SPEC_CTRL_STIBP;
 
 	if (mask != x86_spec_ctrl_base) {
-		pr_info("Spectre v2 cross-process SMT mitigation: %s STIBP\n",
-				cpu_smt_control == CPU_SMT_ENABLED ?
-				"Enabling" : "Disabling");
+		if (static_branch_likely(&cpu_smt_enabled))
+			pr_info("Spectre v2 cross-process SMT mitigation: Enabling STIBP\n");
+		else
+			pr_info("Spectre v2 cross-process SMT mitigation: Disabling STIBP\n");
 		x86_spec_ctrl_base = mask;
 		on_each_cpu(update_stibp_msr, NULL, 1);
 	}
@@ -840,6 +841,14 @@ static const char *l1tf_vmx_states[] = {
 	[VMENTER_L1D_FLUSH_NOT_REQUIRED]	= "flush not necessary"
 };
 
+static char *l1tf_show_smt_vulnerable(void)
+{
+	if (static_branch_likely(&cpu_smt_enabled))
+		return "vulnerable";
+	else
+		return "disabled";
+}
+
 static ssize_t l1tf_show_state(char *buf)
 {
 	if (l1tf_vmx_mitigation == VMENTER_L1D_FLUSH_AUTO)
@@ -847,13 +856,13 @@ static ssize_t l1tf_show_state(char *buf)
 
 	if (l1tf_vmx_mitigation == VMENTER_L1D_FLUSH_EPT_DISABLED ||
 	    (l1tf_vmx_mitigation == VMENTER_L1D_FLUSH_NEVER &&
-	     cpu_smt_control == CPU_SMT_ENABLED))
+	     static_branch_likely(&cpu_smt_enabled)))
 		return sprintf(buf, "%s; VMX: %s\n", L1TF_DEFAULT_MSG,
 			       l1tf_vmx_states[l1tf_vmx_mitigation]);
 
 	return sprintf(buf, "%s; VMX: %s, SMT %s\n", L1TF_DEFAULT_MSG,
 		       l1tf_vmx_states[l1tf_vmx_mitigation],
-		       cpu_smt_control == CPU_SMT_ENABLED ? "vulnerable" : "disabled");
+		       l1tf_show_smt_vulnerable());
 }
 #else
 static ssize_t l1tf_show_state(char *buf)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4555077..accfd2c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11607,7 +11607,7 @@ static int vmx_vm_init(struct kvm *kvm)
 			 * Warn upon starting the first VM in a potentially
 			 * insecure environment.
 			 */
-			if (cpu_smt_control == CPU_SMT_ENABLED)
+			if (static_branch_likely(&cpu_smt_enabled))
 				pr_warn_once(L1TF_MSG_SMT);
 			if (l1tf_vmx_mitigation == VMENTER_L1D_FLUSH_NEVER)
 				pr_warn_once(L1TF_MSG_L1D);
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index b54f085..00af2ae 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -170,15 +170,7 @@ void cpuhp_report_idle_dead(void);
 static inline void cpuhp_report_idle_dead(void) { }
 #endif /* #ifdef CONFIG_HOTPLUG_CPU */
 
-enum cpuhp_smt_control {
-	CPU_SMT_ENABLED,
-	CPU_SMT_DISABLED,
-	CPU_SMT_FORCE_DISABLED,
-	CPU_SMT_NOT_SUPPORTED,
-};
-
 #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
-extern enum cpuhp_smt_control cpu_smt_control;
 extern void cpu_smt_disable(bool force);
 extern void cpu_smt_check_topology_early(void);
 extern void cpu_smt_check_topology(void);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index e216154..54cf3f6 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -368,8 +368,15 @@ static void lockdep_release_cpus_lock(void)
 #endif	/* CONFIG_HOTPLUG_CPU */
 
 #ifdef CONFIG_HOTPLUG_SMT
+
+enum cpuhp_smt_control {
+        CPU_SMT_ENABLED,
+        CPU_SMT_DISABLED,
+        CPU_SMT_FORCE_DISABLED,
+        CPU_SMT_NOT_SUPPORTED,
+};
+
 enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
-EXPORT_SYMBOL_GPL(cpu_smt_control);
 DEFINE_STATIC_KEY_TRUE(cpu_smt_enabled);
 EXPORT_SYMBOL_GPL(cpu_smt_enabled);
 
-- 
2.9.4

Re: [Patch v5 09/16] x86/smt: Convert cpu_smt_control check to cpu_smt_enabled static key

[Thomas Gleixner]

Tim,

On Fri, 16 Nov 2018, Tim Chen wrote:
>  
> +static char *l1tf_show_smt_vulnerable(void)
> +{
> +	if (static_branch_likely(&cpu_smt_enabled))
> +		return "vulnerable";
> +	else
> +		return "disabled";

so an UP kernel will now report vulnerable.

> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11607,7 +11607,7 @@ static int vmx_vm_init(struct kvm *kvm)
>  			 * Warn upon starting the first VM in a potentially
>  			 * insecure environment.
>  			 */
> -			if (cpu_smt_control == CPU_SMT_ENABLED)
> +			if (static_branch_likely(&cpu_smt_enabled))
>  				pr_warn_once(L1TF_MSG_SMT);

Ditto.

>  #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
> -extern enum cpuhp_smt_control cpu_smt_control;
>  extern void cpu_smt_disable(bool force);
>  extern void cpu_smt_check_topology_early(void);
>  extern void cpu_smt_check_topology(void);

What about the same thing in the else path?

> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index e216154..54cf3f6 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -368,8 +368,15 @@ static void lockdep_release_cpus_lock(void)
>  #endif	/* CONFIG_HOTPLUG_CPU */
>  
>  #ifdef CONFIG_HOTPLUG_SMT
> +
> +enum cpuhp_smt_control {
> +        CPU_SMT_ENABLED,
> +        CPU_SMT_DISABLED,
> +        CPU_SMT_FORCE_DISABLED,
> +        CPU_SMT_NOT_SUPPORTED,
> +};
> +
>  enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;

And this needs to be global because?

> -EXPORT_SYMBOL_GPL(cpu_smt_control);

Thanks,

	tglx

Re: [Patch v5 09/16] x86/smt: Convert cpu_smt_control check to cpu_smt_enabled static key

[Thomas Gleixner]

On Mon, 19 Nov 2018, Thomas Gleixner wrote:

> Tim,
> 
> On Fri, 16 Nov 2018, Tim Chen wrote:
> >  
> > +static char *l1tf_show_smt_vulnerable(void)
> > +{
> > +	if (static_branch_likely(&cpu_smt_enabled))
> > +		return "vulnerable";
> > +	else
> > +		return "disabled";
> 
> so an UP kernel will now report vulnerable.

Actually it will not do that because the UP build fails in the linker
stage.

arch/x86/Kconfig:       select HOTPLUG_SMT                      if SMP

Thanks,

	tglx

[Patch v5 10/16] x86/speculation: Turn on or off STIBP according to a task's TIF_STIBP

[Tim Chen]

If STIBP is used all the time, tasks that do not need STIBP
protection will get unnecessarily slowed down by STIBP.

To apply STIBP only to tasks that need it, a new task TIF_STIBP flag is
created. A x86 CPU uses STIBP only for tasks labeled with TIF_STIBP.

During context switch, this flag is checked and the STIBP bit in SPEC_CTRL
MSR is updated according to changes in this flag between previous and
next task.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/include/asm/msr-index.h   |  6 +++++-
 arch/x86/include/asm/spec-ctrl.h   | 12 ++++++++++++
 arch/x86/include/asm/thread_info.h |  5 ++++-
 arch/x86/kernel/process.c          | 10 +++++++++-
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 80f4a4f..501c9d3 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -41,7 +41,11 @@
 
 #define MSR_IA32_SPEC_CTRL		0x00000048 /* Speculation Control */
 #define SPEC_CTRL_IBRS			(1 << 0)   /* Indirect Branch Restricted Speculation */
-#define SPEC_CTRL_STIBP			(1 << 1)   /* Single Thread Indirect Branch Predictors */
+#define SPEC_CTRL_STIBP_SHIFT		1          /*
+						    * Single Thread Indirect Branch
+						    * Predictor (STIBP) bit
+						    */
+#define SPEC_CTRL_STIBP			(1 << SPEC_CTRL_STIBP_SHIFT) /* STIBP mask */
 #define SPEC_CTRL_SSBD_SHIFT		2	   /* Speculative Store Bypass Disable bit */
 #define SPEC_CTRL_SSBD			(1 << SPEC_CTRL_SSBD_SHIFT)   /* Speculative Store Bypass Disable */
 
diff --git a/arch/x86/include/asm/spec-ctrl.h b/arch/x86/include/asm/spec-ctrl.h
index 8e2f841..b593779 100644
--- a/arch/x86/include/asm/spec-ctrl.h
+++ b/arch/x86/include/asm/spec-ctrl.h
@@ -53,12 +53,24 @@ static inline u64 ssbd_tif_to_spec_ctrl(u64 tifn)
 	return (tifn & _TIF_SSBD) >> (TIF_SSBD - SPEC_CTRL_SSBD_SHIFT);
 }
 
+static inline u64 stibp_tif_to_spec_ctrl(u64 tifn)
+{
+	BUILD_BUG_ON(TIF_STIBP < SPEC_CTRL_STIBP_SHIFT);
+	return (tifn & _TIF_STIBP) >> (TIF_STIBP - SPEC_CTRL_STIBP_SHIFT);
+}
+
 static inline unsigned long ssbd_spec_ctrl_to_tif(u64 spec_ctrl)
 {
 	BUILD_BUG_ON(TIF_SSBD < SPEC_CTRL_SSBD_SHIFT);
 	return (spec_ctrl & SPEC_CTRL_SSBD) << (TIF_SSBD - SPEC_CTRL_SSBD_SHIFT);
 }
 
+static inline unsigned long stibp_spec_ctrl_to_tif(u64 spec_ctrl)
+{
+	BUILD_BUG_ON(TIF_STIBP < SPEC_CTRL_STIBP_SHIFT);
+	return (spec_ctrl & SPEC_CTRL_STIBP) << (TIF_STIBP - SPEC_CTRL_STIBP_SHIFT);
+}
+
 static inline u64 ssbd_tif_to_amd_ls_cfg(u64 tifn)
 {
 	return (tifn & _TIF_SSBD) ? x86_amd_ls_cfg_ssbd_mask : 0ULL;
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 2ff2a30..8736d23 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -83,6 +83,7 @@ struct thread_info {
 #define TIF_SYSCALL_EMU		6	/* syscall emulation active */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
+#define TIF_STIBP		9	/* Single thread indirect branch speculation */
 #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 */
@@ -110,6 +111,7 @@ struct thread_info {
 #define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_STIBP		(1 << TIF_STIBP)
 #define _TIF_USER_RETURN_NOTIFY	(1 << TIF_USER_RETURN_NOTIFY)
 #define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_PATCH_PENDING	(1 << TIF_PATCH_PENDING)
@@ -146,7 +148,8 @@ struct thread_info {
 
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\
-	(_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP|_TIF_SSBD)
+	(_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP|		\
+	 _TIF_SSBD|_TIF_STIBP)
 
 #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
 #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 74bef48..943e90d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -406,6 +406,14 @@ static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
 	if (static_cpu_has(X86_FEATURE_SSBD))
 		msr |= ssbd_tif_to_spec_ctrl(tifn);
 
+	/*
+	 * Need STIBP defense against Spectre v2 attack
+	 * if SMT is in use and enhanced IBRS is unsupported.
+	 */
+	if (static_branch_likely(&cpu_smt_enabled) &&
+	    !static_cpu_has(X86_FEATURE_USE_IBRS_ENHANCED))
+		msr |= stibp_tif_to_spec_ctrl(tifn);
+
 	wrmsrl(MSR_IA32_SPEC_CTRL, msr);
 }
 
@@ -418,7 +426,7 @@ static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
 static __always_inline void __speculation_ctrl_update(unsigned long tifp,
 						      unsigned long tifn)
 {
-	bool updmsr = false;
+	bool updmsr = !!((tifp ^ tifn) & _TIF_STIBP);
 
 	/* If TIF_SSBD is different, select the proper mitigation method */
 	if ((tifp ^ tifn) & _TIF_SSBD) {
-- 
2.9.4

[Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

Add new protection modes for Spectre v2 mitigations against
Spectre v2 attacks on user processes.  There are three modes:

	strict mode:
	In this mode, IBPB and STIBP are deployed full
	time to protect all processes.

	lite mode:
	In this mode, IBPB and STIBP are only deployed on
	processes marked with TIF_STIBP flag.

	none mode:
	In this mode, no mitigations are deployed.

The protection mode can be specified by the spectre_v2_app2app
boot parameter with the following semantics:

spectre_v2_app2app=
	off    - Turn off mitigation
	lite   - Protect processes which are marked non-dumpable
	strict - Protect all processes
	auto   - Kernel selects the mode

	Not specifying this option is equivalent to
	spectre_v2_app2app=auto.

	Setting spectre_v2=off will also turn off this mitigation.

	Setting spectre_v2=on implies unconditionally enabling
	this mitigation.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  20 ++++
 arch/x86/include/asm/nospec-branch.h            |   9 ++
 arch/x86/kernel/cpu/bugs.c                      | 153 +++++++++++++++++++++---
 arch/x86/mm/tlb.c                               |  23 +++-
 4 files changed, 185 insertions(+), 20 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 81d1d5a..9c306e3 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4215,6 +4215,26 @@
 			Not specifying this option is equivalent to
 			spectre_v2=auto.
 
+	spectre_v2_app2app=
+			[X86] Control mitigation of Spectre variant 2
+		        application to application (indirect branch speculation)
+			vulnerability.
+
+			off    - Unconditionally disable mitigations
+			lite   - Protect tasks which have requested restricted
+				 indirect branch speculation via the
+				 PR_SET_SPECULATION_CTRL prctl(). 
+			strict - Protect all processes
+			auto   - Kernel selects the mode
+
+			Not specifying this option is equivalent to
+			spectre_v2_app2app=auto.
+
+			Setting spectre_v2=off will also turn off this mitigation.
+
+			Setting spectre_v2=on implies unconditionally enabling
+			this mitigation.
+
 	spec_store_bypass_disable=
 			[HW] Control Speculative Store Bypass (SSB) Disable mitigation
 			(Speculative Store Bypass vulnerability)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 80dc144..587629d 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -3,6 +3,7 @@
 #ifndef _ASM_X86_NOSPEC_BRANCH_H_
 #define _ASM_X86_NOSPEC_BRANCH_H_
 
+#include <linux/static_key.h>
 #include <asm/alternative.h>
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
@@ -226,6 +227,12 @@ enum spectre_v2_mitigation {
 	SPECTRE_V2_IBRS_ENHANCED,
 };
 
+enum spectre_v2_app2app_mitigation {
+	SPECTRE_V2_APP2APP_NONE,
+	SPECTRE_V2_APP2APP_LITE,
+	SPECTRE_V2_APP2APP_STRICT,
+};
+
 /* The Speculative Store Bypass disable variants */
 enum ssb_mitigation {
 	SPEC_STORE_BYPASS_NONE,
@@ -237,6 +244,8 @@ enum ssb_mitigation {
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+DECLARE_STATIC_KEY_FALSE(spectre_v2_app_lite);
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6e1b910..21caade 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -133,6 +133,14 @@ enum spectre_v2_mitigation_cmd {
 	SPECTRE_V2_CMD_RETPOLINE_AMD,
 };
 
+enum spectre_v2_app2app_mitigation_cmd {
+	SPECTRE_V2_APP2APP_CMD_NONE,
+	SPECTRE_V2_APP2APP_CMD_AUTO,
+	SPECTRE_V2_APP2APP_CMD_FORCE,
+	SPECTRE_V2_APP2APP_CMD_LITE,
+	SPECTRE_V2_APP2APP_CMD_STRICT,
+};
+
 static const char *spectre_v2_strings[] = {
 	[SPECTRE_V2_NONE]			= "Vulnerable",
 	[SPECTRE_V2_RETPOLINE_MINIMAL]		= "Vulnerable: Minimal generic ASM retpoline",
@@ -142,12 +150,24 @@ static const char *spectre_v2_strings[] = {
 	[SPECTRE_V2_IBRS_ENHANCED]		= "Mitigation: Enhanced IBRS",
 };
 
+static const char *spectre_v2_app2app_strings[] = {
+	[SPECTRE_V2_APP2APP_NONE]   = "App-App Vulnerable",
+	[SPECTRE_V2_APP2APP_LITE]   = "App-App Mitigation: Protect branch speculation restricted tasks",
+	[SPECTRE_V2_APP2APP_STRICT] = "App-App Mitigation: Full app to app attack protection",
+};
+
+DEFINE_STATIC_KEY_FALSE(spectre_v2_app_lite);
+EXPORT_SYMBOL_GPL(spectre_v2_app_lite);
+
 #undef pr_fmt
 #define pr_fmt(fmt)     "Spectre V2 : " fmt
 
 static enum spectre_v2_mitigation spectre_v2_enabled __ro_after_init =
 	SPECTRE_V2_NONE;
 
+static enum spectre_v2_app2app_mitigation
+	spectre_v2_app2app_enabled __ro_after_init = SPECTRE_V2_APP2APP_NONE;
+
 void
 x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool setguest)
 {
@@ -169,6 +189,9 @@ x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool setguest)
 		    static_cpu_has(X86_FEATURE_AMD_SSBD))
 			hostval |= ssbd_tif_to_spec_ctrl(ti->flags);
 
+		if (static_branch_unlikely(&spectre_v2_app_lite))
+			hostval |= stibp_tif_to_spec_ctrl(ti->flags);
+
 		if (hostval != guestval) {
 			msrval = setguest ? guestval : hostval;
 			wrmsrl(MSR_IA32_SPEC_CTRL, msrval);
@@ -275,6 +298,66 @@ static const struct {
 	{ "auto",              SPECTRE_V2_CMD_AUTO,              false },
 };
 
+static const struct {
+	const char *option;
+	enum spectre_v2_app2app_mitigation_cmd cmd;
+	bool secure;
+} app2app_options[] = {
+	{ "off",	SPECTRE_V2_APP2APP_CMD_NONE,   false },
+	{ "lite",	SPECTRE_V2_APP2APP_CMD_LITE,   false },
+	{ "strict",	SPECTRE_V2_APP2APP_CMD_STRICT, false },
+	{ "auto",	SPECTRE_V2_APP2APP_CMD_AUTO,   false },
+	/*
+	 * The "on" option is kept as last entry. It is implied by
+	 * spectre_v2=on boot parameter and it is not checked
+	 * in spectre_v2_app2app boot parameter.
+	 */
+	{ "on",		SPECTRE_V2_APP2APP_CMD_FORCE,  true  },
+};
+
+static enum spectre_v2_app2app_mitigation_cmd __init
+	    spectre_v2_parse_app2app_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
+{
+	enum spectre_v2_app2app_mitigation_cmd cmd;
+	char arg[20];
+	int ret, i;
+
+	if (v2_cmd == SPECTRE_V2_CMD_FORCE) {
+		cmd = SPECTRE_V2_APP2APP_CMD_FORCE;
+		goto show_cmd;
+	}
+
+	cmd = SPECTRE_V2_APP2APP_CMD_AUTO;
+	ret = cmdline_find_option(boot_command_line, "spectre_v2_app2app",
+				  arg, sizeof(arg));
+	if (ret < 0)
+		return SPECTRE_V2_APP2APP_CMD_AUTO;
+
+	/*
+	 * Don't check the last entry for forced mitigation. It is infered from
+	 * v2_cmd == SPECTRE_V2_CMD_FORCE
+	 */
+	for (i = 0; i < ARRAY_SIZE(app2app_options)-1; i++) {
+		if (!match_option(arg, ret, app2app_options[i].option))
+			continue;
+		cmd = app2app_options[i].cmd;
+		break;
+	}
+
+	if (i >= ARRAY_SIZE(app2app_options)) {
+		pr_err("unknown app to app protection option (%s). Switching to AUTO select\n", arg);
+		return SPECTRE_V2_APP2APP_CMD_AUTO;
+	}
+
+show_cmd:
+	if (app2app_options[i].secure)
+		spec2_print_if_secure(app2app_options[i].option);
+	else
+		spec2_print_if_insecure(app2app_options[i].option);
+
+	return cmd;
+}
+
 static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 {
 	char arg[20];
@@ -328,14 +411,19 @@ static bool stibp_needed(void)
 	/*
 	 * Determine if STIBP should be always on.
 	 * Using enhanced IBRS makes using STIBP unnecessary.
+	 * For lite option, STIBP is used only for task with
+	 * TIF_STIBP flag. STIBP is not always on for that case.
 	 */
 
-	if (spectre_v2_enabled == SPECTRE_V2_NONE)
+	if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
 		return false;
 
 	if (static_cpu_has(X86_FEATURE_USE_IBRS_ENHANCED))
 		return false;
 
+	if (static_branch_unlikely(&spectre_v2_app_lite))
+		return false;
+
 	if (!boot_cpu_has(X86_FEATURE_STIBP))
 		return false;
 
@@ -376,6 +464,8 @@ static void __init spectre_v2_select_mitigation(void)
 {
 	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
 	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
+	enum spectre_v2_app2app_mitigation_cmd app2app_cmd;
+	enum spectre_v2_app2app_mitigation app2app_mode;
 
 	/*
 	 * If the CPU is not affected and the command line mode is NONE or AUTO
@@ -452,12 +542,6 @@ static void __init spectre_v2_select_mitigation(void)
 	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
 
-	/* Initialize Indirect Branch Prediction Barrier if supported */
-	if (boot_cpu_has(X86_FEATURE_IBPB)) {
-		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
-		pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");
-	}
-
 	/*
 	 * Retpoline means the kernel is safe because it has no indirect
 	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
@@ -474,6 +558,43 @@ static void __init spectre_v2_select_mitigation(void)
 		pr_info("Enabling Restricted Speculation for firmware calls\n");
 	}
 
+	app2app_mode = SPECTRE_V2_APP2APP_NONE;
+	if (!boot_cpu_has(X86_FEATURE_IBPB) ||
+	    !boot_cpu_has(X86_FEATURE_STIBP))
+		goto set_app2app_mode;
+
+	app2app_cmd = spectre_v2_parse_app2app_cmdline(cmd);
+
+	switch (app2app_cmd) {
+	case SPECTRE_V2_APP2APP_CMD_NONE:
+		break;
+
+	case SPECTRE_V2_APP2APP_CMD_LITE:
+	case SPECTRE_V2_APP2APP_CMD_AUTO:
+		app2app_mode = SPECTRE_V2_APP2APP_LITE;
+		break;
+
+	case SPECTRE_V2_APP2APP_CMD_STRICT:
+	case SPECTRE_V2_APP2APP_CMD_FORCE:
+		app2app_mode = SPECTRE_V2_APP2APP_STRICT;
+		break;
+	}
+
+	/*
+	 * Initialize Indirect Branch Prediction Barrier if supported
+	 * and not disabled explicitly
+	 */
+	if (app2app_mode != SPECTRE_V2_APP2APP_NONE) {
+		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
+		pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");
+	}
+
+set_app2app_mode:
+	spectre_v2_app2app_enabled = app2app_mode;
+	pr_info("%s\n", spectre_v2_app2app_strings[app2app_mode]);
+	if (app2app_mode == SPECTRE_V2_APP2APP_LITE)
+		static_branch_enable(&spectre_v2_app_lite);
+
 	/* Enable STIBP if appropriate */
 	arch_smt_update();
 }
@@ -873,21 +994,23 @@ static ssize_t l1tf_show_state(char *buf)
 
 static char *stibp_state(void)
 {
-	if (spectre_v2_enabled == SPECTRE_V2_IBRS_ENHANCED)
+	if (spectre_v2_enabled == SPECTRE_V2_IBRS_ENHANCED ||
+	    spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
 		return "";
-
-	if (x86_spec_ctrl_base & SPEC_CTRL_STIBP)
-		return ", STIBP";
+	else if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_LITE)
+		return ", STIBP-lite";
 	else
-		return "";
+		return ", STIBP-strict";
 }
 
 static char *ibpb_state(void)
 {
-	if (boot_cpu_has(X86_FEATURE_USE_IBPB))
-		return ", IBPB";
-	else
+	if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
 		return "";
+	else if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_LITE)
+		return ", IBPB-lite";
+	else
+		return ", IBPB-strict";
 }
 
 static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr,
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index bddd6b3..9201470 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -184,14 +184,27 @@ 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.
+	 * Don't issue IBPB when switching to kernel threads or staying in the
+	 * same mm context.
+	 */
+	if (!tsk || !tsk->mm || tsk->mm->context.ctx_id == last_ctx_id)
+		return false;
+
+	/*
+	 * If lite protection mode is enabled, check the STIBP thread flag.
+	 *
+	 * Otherwise check if the current (previous) task has access to the
+	 * the memory of the @tsk (next) task for strict app to app protection.
+	 * 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_sched(tsk, PTRACE_MODE_SPEC_IBPB));
+
+	if (static_branch_unlikely(&spectre_v2_app_lite))
+		return test_tsk_thread_flag(tsk, TIF_STIBP);
+	else
+		return ptrace_may_access_sched(tsk, PTRACE_MODE_SPEC_IBPB);
 }
 
 void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
-- 
2.9.4

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Fri, 16 Nov 2018, Tim Chen wrote:

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 81d1d5a..9c306e3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4215,6 +4215,26 @@
>  			Not specifying this option is equivalent to
>  			spectre_v2=auto.
>  
> +	spectre_v2_app2app=
> +			[X86] Control mitigation of Spectre variant 2
> +		        application to application (indirect branch speculation)
> +			vulnerability.
> +
> +			off    - Unconditionally disable mitigations
> +			lite   - Protect tasks which have requested restricted
> +				 indirect branch speculation via the
> +				 PR_SET_SPECULATION_CTRL prctl(). 

Don't we also want to do the same for SECCOMP processess, analogically how 
we do it for SSBD?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Sat, 17 Nov 2018, Jiri Kosina wrote:

> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 81d1d5a..9c306e3 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -4215,6 +4215,26 @@
> >  			Not specifying this option is equivalent to
> >  			spectre_v2=auto.
> >  
> > +	spectre_v2_app2app=
> > +			[X86] Control mitigation of Spectre variant 2
> > +		        application to application (indirect branch speculation)
> > +			vulnerability.
> > +
> > +			off    - Unconditionally disable mitigations
> > +			lite   - Protect tasks which have requested restricted
> > +				 indirect branch speculation via the
> > +				 PR_SET_SPECULATION_CTRL prctl(). 
> 
> Don't we also want to do the same for SECCOMP processess, analogically how 
> we do it for SSBD?

IOW, how about patch below on top of your series? Thanks.


From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] x86/speculation: enforce STIBP for SECCOMP tasks in lite mode

If 'lite' mode of app2app protection from spectre_v2 is selected on
kernel command-line, we are currently applying STIBP protection to
non-dumpable tasks, and tasks that have explicitly requested such
protection via

	prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);

Let's extend this to cover also SECCOMP tasks (analogically to how we
apply SSBD protection).

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 Documentation/admin-guide/kernel-parameters.txt | 9 +++++----
 arch/x86/kernel/cpu/bugs.c                      | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 102f9a169eec..74f547e5c8f6 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4221,10 +4221,11 @@
 			vulnerability.
 
 			off    - Unconditionally disable mitigations
-			lite   - Protect tasks which are marked non-dumpable
-				 and tasks which have requested restricted
-				 indirect branch speculation via the
-				 PR_SET_SPECULATION_CTRL prctl(). 
+			lite   - Protect tasks which are marked non-dumpable,
+				 tasks which have requested restricted indirect
+				 branch speculation via the
+				 PR_SET_SPECULATION_CTRL prctl() and seccomp
+				 tasks.
 			strict - Protect all processes
 			auto   - Kernel selects the mode
 
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index e7f9334f18c0..3ec952108e87 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -863,6 +863,8 @@ void arch_seccomp_spec_mitigate(struct task_struct *task)
 {
 	if (ssb_mode == SPEC_STORE_BYPASS_SECCOMP)
 		ssb_prctl_set(task, PR_SPEC_FORCE_DISABLE);
+	if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_LITE)
+		set_task_stibp(task, true);
 }
 #endif
 

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Sun, 18 Nov 2018, Jiri Kosina wrote:
> On Sat, 17 Nov 2018, Jiri Kosina wrote:

> Subject: [PATCH] x86/speculation: enforce STIBP for SECCOMP tasks in lite mode
> 
> If 'lite' mode of app2app protection from spectre_v2 is selected on
> kernel command-line, we are currently applying STIBP protection to
> non-dumpable tasks, and tasks that have explicitly requested such
> protection via
> 
> 	prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);
> 
> Let's extend this to cover also SECCOMP tasks (analogically to how we
> apply SSBD protection).

Right. And SSBD does not fiddle with dumpable.

Willy had concerns about the (ab)use of dumpable so I'm holding off on that
bit for now.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Mon, 19 Nov 2018, Thomas Gleixner wrote:

> > On Sat, 17 Nov 2018, Jiri Kosina wrote:
> 
> > Subject: [PATCH] x86/speculation: enforce STIBP for SECCOMP tasks in lite mode
> > 
> > If 'lite' mode of app2app protection from spectre_v2 is selected on
> > kernel command-line, we are currently applying STIBP protection to
> > non-dumpable tasks, and tasks that have explicitly requested such
> > protection via
> > 
> > 	prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);
> > 
> > Let's extend this to cover also SECCOMP tasks (analogically to how we
> > apply SSBD protection).
> 
> Right. And SSBD does not fiddle with dumpable.
> 
> Willy had concerns about the (ab)use of dumpable so I'm holding off on that
> bit for now.

Yeah. IBPB implementation used to check the dumpability of tasks during 
rescheduling, but that went away later.

I still think that ideally that 'app2app' setting would toggle how IBPB is 
being used as well, something along the lines:

lite:
	- STIBP for the ones marked via prctl() and SECCOMP with the TIF_ 
	  flag
	- ibpb_needed() returning true for the same

strict:
	- STIBP: as currently implemented
	- ibpb_needed() returning always true

off:
	- neither STIBP nor IBPB applied ever

That's give us also some % of performance lost via IBPB back.

Makes sense?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Mon, 19 Nov 2018, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Thomas Gleixner wrote:
> 
> > > On Sat, 17 Nov 2018, Jiri Kosina wrote:
> > 
> > > Subject: [PATCH] x86/speculation: enforce STIBP for SECCOMP tasks in lite mode
> > > 
> > > If 'lite' mode of app2app protection from spectre_v2 is selected on
> > > kernel command-line, we are currently applying STIBP protection to
> > > non-dumpable tasks, and tasks that have explicitly requested such
> > > protection via
> > > 
> > > 	prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);
> > > 
> > > Let's extend this to cover also SECCOMP tasks (analogically to how we
> > > apply SSBD protection).
> > 
> > Right. And SSBD does not fiddle with dumpable.
> > 
> > Willy had concerns about the (ab)use of dumpable so I'm holding off on that
> > bit for now.
> 
> Yeah. IBPB implementation used to check the dumpability of tasks during 
> rescheduling, but that went away later.
> 
> I still think that ideally that 'app2app' setting would toggle how IBPB is 
> being used as well, something along the lines:
> 
> lite:
> 	- STIBP for the ones marked via prctl() and SECCOMP with the TIF_ 
> 	  flag
> 	- ibpb_needed() returning true for the same
> 
> strict:
> 	- STIBP: as currently implemented
> 	- ibpb_needed() returning always true
> 
> off:
> 	- neither STIBP nor IBPB applied ever
> 
> That's give us also some % of performance lost via IBPB back.
> 
> Makes sense?

Except for the naming convention, yes. See other mail.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Mon, 19 Nov 2018, Thomas Gleixner wrote:

> > Yeah. IBPB implementation used to check the dumpability of tasks during 
> > rescheduling, but that went away later.
> > 
> > I still think that ideally that 'app2app' setting would toggle how IBPB is 
> > being used as well, something along the lines:
> > 
> > lite:
> > 	- STIBP for the ones marked via prctl() and SECCOMP with the TIF_ 
> > 	  flag
> > 	- ibpb_needed() returning true for the same
> > 
> > strict:
> > 	- STIBP: as currently implemented
> > 	- ibpb_needed() returning always true
> > 
> > off:
> > 	- neither STIBP nor IBPB applied ever
> > 
> > That's give us also some % of performance lost via IBPB back.
> > 
> > Makes sense?
> 
> Except for the naming convention, yes. See other mail.

Actually Tim's patchset seems to already deal with IBPB in a consistent 
way as well in

	[11/16] x86/speculation: Add Spectre v2 app to app protection modes

but the fact that it's still using TIF_STIBP makes it a bit confusing and 
hidden. So I'd suggest to fold something like below into it.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] x86/speculation: rename TIF_STIBP to TIF_SPEC_INDIR_BRANCH

TIF_STIBP is being used not only for making decisions about STIBP, but also
for determining whether IBPB should be issued during reschedule. Let's
not pretend the TIF flag is specific to STIBP.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/include/asm/spec-ctrl.h   |  8 ++++----
 arch/x86/include/asm/thread_info.h |  6 +++---
 arch/x86/kernel/cpu/bugs.c         | 14 +++++++-------
 arch/x86/kernel/process.c          |  2 +-
 arch/x86/mm/tlb.c                  |  2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/spec-ctrl.h b/arch/x86/include/asm/spec-ctrl.h
index b59377952665..41b993e3756f 100644
--- a/arch/x86/include/asm/spec-ctrl.h
+++ b/arch/x86/include/asm/spec-ctrl.h
@@ -55,8 +55,8 @@ static inline u64 ssbd_tif_to_spec_ctrl(u64 tifn)
 
 static inline u64 stibp_tif_to_spec_ctrl(u64 tifn)
 {
-	BUILD_BUG_ON(TIF_STIBP < SPEC_CTRL_STIBP_SHIFT);
-	return (tifn & _TIF_STIBP) >> (TIF_STIBP - SPEC_CTRL_STIBP_SHIFT);
+	BUILD_BUG_ON(TIF_SPEC_INDIR_BRANCH < SPEC_CTRL_STIBP_SHIFT);
+	return (tifn & _TIF_SPEC_INDIR_BRANCH) >> (TIF_SPEC_INDIR_BRANCH - SPEC_CTRL_STIBP_SHIFT);
 }
 
 static inline unsigned long ssbd_spec_ctrl_to_tif(u64 spec_ctrl)
@@ -67,8 +67,8 @@ static inline unsigned long ssbd_spec_ctrl_to_tif(u64 spec_ctrl)
 
 static inline unsigned long stibp_spec_ctrl_to_tif(u64 spec_ctrl)
 {
-	BUILD_BUG_ON(TIF_STIBP < SPEC_CTRL_STIBP_SHIFT);
-	return (spec_ctrl & SPEC_CTRL_STIBP) << (TIF_STIBP - SPEC_CTRL_STIBP_SHIFT);
+	BUILD_BUG_ON(TIF_SPEC_INDIR_BRANCH < SPEC_CTRL_STIBP_SHIFT);
+	return (spec_ctrl & SPEC_CTRL_STIBP) << (TIF_SPEC_INDIR_BRANCH - SPEC_CTRL_STIBP_SHIFT);
 }
 
 static inline u64 ssbd_tif_to_amd_ls_cfg(u64 tifn)
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 453616fdbbd0..796ba73c5a26 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -110,7 +110,7 @@ struct thread_info {
 /* Security mode */
 #define TIF_SECCOMP		24	/* Secure computing */
 #define TIF_SSBD		25	/* Speculative store bypass disable */
-#define TIF_STIBP		26	/* Single thread indirect branch speculation */
+#define TIF_SPEC_INDIR_BRANCH		26	/* Indirect branch speculation */
 
 #define _TIF_NOCPUID		(1 << TIF_NOCPUID)
 #define _TIF_NOTSC		(1 << TIF_NOTSC)
@@ -142,7 +142,7 @@ struct thread_info {
 
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
 #define _TIF_SSBD		(1 << TIF_SSBD)
-#define _TIF_STIBP		(1 << TIF_STIBP)
+#define _TIF_SPEC_INDIR_BRANCH		(1 << TIF_SPEC_INDIR_BRANCH)
 
 /*
  * work to do in syscall_trace_enter().  Also includes TIF_NOHZ for
@@ -164,7 +164,7 @@ struct thread_info {
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\
 	(_TIF_IO_BITMAP|_TIF_NOCPUID|_TIF_NOTSC|_TIF_BLOCKSTEP|		\
-	 _TIF_SSBD|_TIF_STIBP)
+	 _TIF_SSBD|_TIF_SPEC_INDIR_BRANCH)
 
 #define _TIF_WORK_CTXSW_PREV (_TIF_WORK_CTXSW|_TIF_USER_RETURN_NOTIFY)
 #define _TIF_WORK_CTXSW_NEXT (_TIF_WORK_CTXSW)
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 3ec952108e87..ae10f5cef3a0 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -410,10 +410,10 @@ static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 static bool stibp_needed(void)
 {
 	/*
-	 * Determine if STIBP should be always on.
-	 * Using enhanced IBRS makes using STIBP unnecessary.
-	 * For lite option, STIBP is used only for task with
-	 * TIF_STIBP flag. STIBP is not always on for that case.
+	 * Determine if STIBP should be always on.  Using enhanced IBRS makes
+	 * using STIBP unnecessary.  For lite option, STIBP is used only for
+	 * task with TIF_SPEC_INDIR_BRANCH flag. STIBP is not always on for
+	 * that case.
 	 */
 
 	if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
@@ -779,9 +779,9 @@ static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
 	bool update = false;
 
 	if (stibp_on)
-		update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
+		update = !test_and_set_tsk_thread_flag(tsk, TIF_SPEC_INDIR_BRANCH);
 	else if (!task_spec_indir_branch_disable(tsk))
-		update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
+		update = test_and_clear_tsk_thread_flag(tsk, TIF_SPEC_INDIR_BRANCH);
 
 	if (tsk == current && update)
 		speculation_ctrl_update_current();
@@ -898,7 +898,7 @@ static int indir_branch_prctl_get(struct task_struct *task)
 	case SPECTRE_V2_APP2APP_LITE:
 		if (task_spec_indir_branch_force_disable(task))
 			return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE;
-		if (test_tsk_thread_flag(task, TIF_STIBP))
+		if (test_tsk_thread_flag(task, TIF_SPEC_INDIR_BRANCH))
 			return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
 		return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
 	case SPECTRE_V2_APP2APP_STRICT:
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 943e90dd2e85..15fc1b30c999 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -426,7 +426,7 @@ static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
 static __always_inline void __speculation_ctrl_update(unsigned long tifp,
 						      unsigned long tifn)
 {
-	bool updmsr = !!((tifp ^ tifn) & _TIF_STIBP);
+	bool updmsr = !!((tifp ^ tifn) & _TIF_SPEC_INDIR_BRANCH);
 
 	/* If TIF_SSBD is different, select the proper mitigation method */
 	if ((tifp ^ tifn) & _TIF_SSBD) {
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 920147059567..616694c7497c 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -202,7 +202,7 @@ static bool ibpb_needed(struct task_struct *tsk, u64 last_ctx_id)
 	 */
 
 	if (static_branch_unlikely(&spectre_v2_app_lite))
-		return test_tsk_thread_flag(tsk, TIF_STIBP);
+		return test_tsk_thread_flag(tsk, TIF_SPEC_INDIR_BRANCH);
 	else
 		return ptrace_may_access_sched(tsk, PTRACE_MODE_SPEC_IBPB);
 }

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 06:00 AM, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Thomas Gleixner wrote:
> 
>>> Yeah. IBPB implementation used to check the dumpability of tasks during 
>>> rescheduling, but that went away later.
>>>
>>> I still think that ideally that 'app2app' setting would toggle how IBPB is 
>>> being used as well, something along the lines:
>>>
>>> lite:
>>> 	- STIBP for the ones marked via prctl() and SECCOMP with the TIF_ 
>>> 	  flag
>>> 	- ibpb_needed() returning true for the same
>>>
>>> strict:
>>> 	- STIBP: as currently implemented
>>> 	- ibpb_needed() returning always true
>>>
>>> off:
>>> 	- neither STIBP nor IBPB applied ever
>>>
>>> That's give us also some % of performance lost via IBPB back.
>>>
>>> Makes sense?
>>
>> Except for the naming convention, yes. See other mail.
> 
> Actually Tim's patchset seems to already deal with IBPB in a consistent 
> way as well in
> 
> 	[11/16] x86/speculation: Add Spectre v2 app to app protection modes
> 
> but the fact that it's still using TIF_STIBP makes it a bit confusing and 
> hidden. So I'd suggest to fold something like below into it.
> 

Makes sense.  Will rename the flag.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Andrea Arcangeli]

Hello everyone,

On Mon, Nov 19, 2018 at 02:49:36PM +0100, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Thomas Gleixner wrote:
> 
> > > On Sat, 17 Nov 2018, Jiri Kosina wrote:
> > 
> > > Subject: [PATCH] x86/speculation: enforce STIBP for SECCOMP tasks in lite mode
> > > 
> > > If 'lite' mode of app2app protection from spectre_v2 is selected on
> > > kernel command-line, we are currently applying STIBP protection to
> > > non-dumpable tasks, and tasks that have explicitly requested such
> > > protection via
> > > 
> > > 	prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);
> > > 
> > > Let's extend this to cover also SECCOMP tasks (analogically to how we
> > > apply SSBD protection).
> > 
> > Right. And SSBD does not fiddle with dumpable.
> > 
> > Willy had concerns about the (ab)use of dumpable so I'm holding off on that
> > bit for now.
> 
> Yeah. IBPB implementation used to check the dumpability of tasks during 
> rescheduling, but that went away later.
> 
> I still think that ideally that 'app2app' setting would toggle how IBPB is 
> being used as well, something along the lines:
> 
> lite:
> 	- STIBP for the ones marked via prctl() and SECCOMP with the TIF_ 
> 	  flag

Note: STIBP is not SSBD: STIBP enabled inside the SECCOMP jail only
makes the code inside SECCOMP immune from attack from processes
outside the SECCOMP jail.

The specs don't say if by making it immune from BTB mistraining, it
also could prevent to mistrain the BTB in order to attack what's
outside the SECCOMP jail. Probably it won't and I doubt we can rely on
it even if some implementation could do that.

Generally speaking the untrusted code that would try to use spectrev2
to attack the other processes is more likely to run inside SECCOMP
jail than outside, so if SECCOMP should be used as a best effort
heuristic to decide when to enable STIBP, it would make more sense to
enable STIBP outside SECCOMP, and not inside. I.e. the exact opposite
of what you're proposing above.

Code running under SECCOMP is not necessarily less performance
critical so if we don't protect what's outside, I doubt we should
protect what's inside.

In short I doubt we can make an association between SECCOMP and STIBP
here and we should leave SECCOMP alone this time.

The prctl is a nice to have feature, but it is more for specialized
apps like gpg that aren't performance critical anyway.

The system wide default is more a decision the admin should do without
having to add prctl wrappers to all apps or change config
files. Clearly the prctl won't hurt especially if it can be overridden
(and forced off) with the global tweak like with the ssbd options. The
only thing I don't understand is why one has to reboot to change the
global defaults for ssbd and now STIBP (unless you're already planning
to make the global tweak runtime tunable) despite there's no runtime
benefit by forcing these decision at boot time only. Would be nice to
add runtime tweak options for at least STIBP and SSBD that aren't
confined to the prctl.

Thanks,
Andrea

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Mon, 19 Nov 2018, Andrea Arcangeli wrote:

> Generally speaking the untrusted code that would try to use spectrev2
> to attack the other processes is more likely to run inside SECCOMP
> jail than outside, so if SECCOMP should be used as a best effort
> heuristic to decide when to enable STIBP, it would make more sense to
> enable STIBP outside SECCOMP, and not inside. I.e. the exact opposite
> of what you're proposing above.

Hmm, that's a very good point. But I actually don't see why both 
directions wouldn't be possible real-blackhat-world scenarios. So perhaps 
we'd want, under the basic asumption that "SECCOMP should really be 
sandboxed from outisde interventions and from causing them from inside as 
well", flush on both switch-to-seccomp and switch-from-seccomp?

> Code running under SECCOMP is not necessarily less performance critical 
> so if we don't protect what's outside, I doubt we should protect what's 
> inside.
> 
> In short I doubt we can make an association between SECCOMP and STIBP
> here and we should leave SECCOMP alone this time.
> 
> The prctl is a nice to have feature, but it is more for specialized
> apps like gpg that aren't performance critical anyway.
> 
> The system wide default is more a decision the admin should do without 
> having to add prctl wrappers to all apps or change config files. Clearly 
> the prctl won't hurt especially if it can be overridden (and forced off) 
> with the global tweak like with the ssbd options. The only thing I don't 
> understand is why one has to reboot to change the global defaults for 
> ssbd and now STIBP (unless you're already planning to make the global 
> tweak runtime tunable) despite there's no runtime benefit by forcing 
> these decision at boot time only. Would be nice to add runtime tweak 
> options for at least STIBP and SSBD that aren't confined to the prctl.

So if I understand you correctly, what you are proposing here is to keep 
the current code, but just switch the default, and make it 
runtime/boottime togglable?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Andrea Arcangeli]

On Mon, Nov 19, 2018 at 08:39:41PM +0100, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Andrea Arcangeli wrote:
> 
> > Generally speaking the untrusted code that would try to use spectrev2
> > to attack the other processes is more likely to run inside SECCOMP
> > jail than outside, so if SECCOMP should be used as a best effort
> > heuristic to decide when to enable STIBP, it would make more sense to
> > enable STIBP outside SECCOMP, and not inside. I.e. the exact opposite
> > of what you're proposing above.
> 
> Hmm, that's a very good point. But I actually don't see why both 
> directions wouldn't be possible real-blackhat-world scenarios. So perhaps 
> we'd want, under the basic asumption that "SECCOMP should really be 
> sandboxed from outisde interventions and from causing them from inside as 
> well", flush on both switch-to-seccomp and switch-from-seccomp?

STIBP doesn't flush so I don't see how "flush" and "switch" fits the
STIBP discussion.

Flush as in IPBP on switch-to-seccomp and switch-from-seccomp? IBPB is
not going to solve the HT attack and STIBP is only about the HT
attack. IBPB only solves the user-to-user context switch attack.

I just don't see SECCOMP as a good fit for a default-on heuristic
because there would be more arguments to enable STIBP outside seccomp
than inside and even if you ignore that, SECCOMP is used by pretty
much everything including wrapping through containers and systemd so
it would still leave lots of software running with STIBP (and for all
the wrong reasons too).

As opposed the not dumpable was a much better fit for a per-process
enablement heuristic, because the not dumpable code is more likely to
be the one that needs protection from attack and it's less likely to
be the very malicious code that got exploited (or was untrusted to
begin with like DRM binary blobs or public cloud usages). However like
mentioned in this thread suid calls can set the non dumpable flag, so
it's not ideal either. We'd need to track which processes turned off
the not dumpable flag with SUID_DUMP_DISABLE explicitly.

> So if I understand you correctly, what you are proposing here is to keep 
> the current code, but just switch the default, and make it 
> runtime/boottime togglable?

Deciding the default on this stuff is nightmarish, there's no good
default and the best system-wide default is data and workload
dependent.

And this is precisly why this should be runtime toggable and not just
boot-time toggable in my view.

I don't disagree with default disabled, that may be safer to avoid
breaking workloads near full capacity (same reason for why HT isn't
disabled by default for L1TF), we've to draw a line somewhere with the
default.

The ASLR argument from Tim's patchset cover letter combined with PID
namespaces should go a long way to mitigate the HT attack too even
without STIBP.

In my understanding you need to know what's running on the sibling
thread to derandomize ASLR, otherwise you'd be potentially attacking
glibc or some lib that yes is always mapped by all processes but it's
not mapped at the same address in all processes. You need to restrict
the measurement during ASLR derandomization to the exact time there's
the target process running in the sibling (any thread of the process
would be good).

Now assuming there's no pid namespace that prevents to see what's
running on the sibling thread, it depends on the scheduling jittering
and on the size and hw hashfn of the BTB (which varies across CPUs)
how hard it is to derandomize ASLR. According to the original paper,
some non-Linux OS has many low significant bits of their ASLR not
randomized and the high bits don't go into the hashfn of the BTB
(incidentally the ASLR derandomization technique to attack userland is
apparently not tested for Linux). We should be randomizing all bits
down to bit 12 (not bit 15), so for us the derandomizing should be 256
times more expensive? (At least until the day we map .text into
filesystem THP pagecache...) The more bits randomized that are part of
the BTB hashfn input, the more computational expensive it becomes to
derandomize ASLR, the more the random scheduling jittering will
interfere with the longer measurement, so hopefully the complexity of
the attack grows more than linearly with the number of random ASLR low
bits that gets into the BTB hashfn input. This is an optimistic guess
though.

Overall for on-prem cloud usages where no random malicious code can
run in the CPU by design, and this is only a post-exploitation
robustness issue, it doesn't seem a major concern if STIBP is disabled
if pid namespaces and ASLR have been fully leveraged by default in
Kubernetes containers. I'm curious to hear other people opinion on
this too however.

Downstream we always provided ibrs_enabled=2/3 which already implies
STIBP implicitly enabled at all times too, and that unlike STIBP
alone, also protects against guest attack on host userland too within
the same context. It's tunable at runtime. It's not enabled by default
for similar considerations as above for STIBP. I think it's good to
give the users the choice to be 100% secure against everything as an
opt-in (ideally requiring a reboot, that actually helps the evaluation
of the performance impact, which is obviously workload dependent too).

As an alternative to STIBP it would also be possible to alter the
scheduler so it never runs different processes in different siblings
of the same core, unless they can ptrace each other (same exact ptrace
check as the one to decide if to run IBPB to protect against the
context switch spectrev2 attack, except it needs to be checked in both
directions here). This way you could still have zero penalty for all
kernel builds and all threaded programs etc.. while still retaining
full security against the HT attack (and IBPB takes care of the rest
with the same ptrace check). With several containerized single
threaded workloads it would be slower than STIBP though by leaving all
siblings idle. However we could also let any process under pid
namespace to run along with any other processes under pid namespaces
even if they cannot ptrace each other to take care of that detail. Not
sure if it's worth it, but it remains a possibility that may perform
better than STIBP. It would also take care in general of cache attacks
on non-constant time algorithms etc.. not just spectrev2 HT attack.

Thanks,
Andrea

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Dave Hansen]

On 11/19/18 11:32 AM, Andrea Arcangeli wrote:
> The specs don't say if by making it immune from BTB mistraining, it
> also could prevent to mistrain the BTB in order to attack what's
> outside the SECCOMP jail. Probably it won't and I doubt we can rely on
> it even if some implementation could do that.

I just talked with Andi and Tim about this.  The *current* spec for
STIBP[1] states that it bidirectional: setting it on one thread provides
mitigation against any threads attacking any other thread on the core.

This means that it provides protection for victims being in and out of
SECCOMP jail when the attacker is either in or out of SECCOMP jail.

However, the current spec[1], differs from the *original* spec PDF that
Intel released last year.  Both are correct in that they describe all
current (Intel) implementations of STIBP.  However, the new
_description_ of STIBP is stronger than it was originally.

Here's the current description:

> Setting ... STIBP ... on a logical processor prevents the predicted
> targets of indirect branches on any logical processor of that core
> from being controlled by software that executes (or executed
> previously) on another logical processor of the same core.

1.
https://software.intel.com/security-software-guidance/insights/deep-dive-single-thread-indirect-branch-predictors

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Andrea Arcangeli]

On Mon, Nov 19, 2018 at 01:33:08PM -0800, Dave Hansen wrote:
> On 11/19/18 11:32 AM, Andrea Arcangeli wrote:
> > The specs don't say if by making it immune from BTB mistraining, it
> > also could prevent to mistrain the BTB in order to attack what's
> > outside the SECCOMP jail. Probably it won't and I doubt we can rely on
> > it even if some implementation could do that.
> 
> I just talked with Andi and Tim about this.  The *current* spec for
> STIBP[1] states that it bidirectional: setting it on one thread provides
> mitigation against any threads attacking any other thread on the core.
                     ^^^                   ^^^
> 
> This means that it provides protection for victims being in and out of
> SECCOMP jail when the attacker is either in or out of SECCOMP jail.
> 
> However, the current spec[1], differs from the *original* spec PDF that
> Intel released last year.  Both are correct in that they describe all
> current (Intel) implementations of STIBP.  However, the new
> _description_ of STIBP is stronger than it was originally.
> 
> Here's the current description:
> 
> > Setting ... STIBP ... on a logical processor prevents the predicted
> > targets of indirect branches on any logical processor of that core
                                    ^^^
> > from being controlled by software that executes (or executed
> > previously) on another logical processor of the same core.
                   ^^^^^^^
> 
> 1.
> https://software.intel.com/security-software-guidance/insights/deep-dive-single-thread-indirect-branch-predictors

I'm not used to official specs in a "insight & deep dive"
blog-post-like webpage, so I didn't notice this deep dive detail.

You use "any" vs "any" but the spec you quoted still says "any" vs
"another".

If I shall take the above literally it still means that if I set STIBP
inside SECCOMP, as long as it's set, it prevents indirect branches of
all siblings to be controlled from code outside the SECCOMP jail
running in another sibling (or that run previously in another
sibling). I.e. the "deep dive" stronger semantics of STIBP just mean
the code outside SECCOMP cannot attack itself while the code inside
SECCOMP runs and keeps STIBP set.

So you may want to ask why it wasn't written as your "any" vs "any" email:

Setting ... STIBP ... on a logical processor prevents the predicted
targets of indirect branches on any logical processor of that core
from being controlled by software that executes (or executed
previously) on any logical processor of the same core.

A s/another/any/ is still missing to match your exact interpretation,
otherwise it still seems a gray area and I've no idea why it wasn't
written as "any" which would clear any risk for misunderstanding and
instead it kind of uses a word that matches the somewhat more strict
pdf specs.

If this is clarified the concern that remains is that lots of
potentially performance critical stuff runs under SECCOMP but of
course it changes everything in terms of possibly enabling STIBP under
SECCOMP by default, it certainly would make more sense then.

Thanks,
Andrea

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Dave Hansen]

On 11/19/18 3:16 PM, Andrea Arcangeli wrote:
> So you may want to ask why it wasn't written as your "any" vs "any" email:

Presumably because the authors really and truly meant what they said.  I
was not being as careful in my wording as they were.  :)

There is nothing in the spec that says that STIBP disables branch
prediction itself, or that it keeps a thread from influencing *itself*.

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Andrea Arcangeli]

On Mon, Nov 19, 2018 at 03:25:41PM -0800, Dave Hansen wrote:
> On 11/19/18 3:16 PM, Andrea Arcangeli wrote:
> > So you may want to ask why it wasn't written as your "any" vs "any" email:
> 
> Presumably because the authors really and truly meant what they said.  I
> was not being as careful in my wording as they were.  :)
> 
> There is nothing in the spec that says that STIBP disables branch
> prediction itself, or that it keeps a thread from influencing *itself*.

Just in case, another thing come to mind, what about mistraining the
BTB with STIBP set inside the SECCOMP jail and then going to sleep or
being migrated by the scheduler to another core which clears STIBP on
the core? Can the mistraining happened inside the SECCOMP jail with
STIBP set influence the code outside SECCOMP after STIBP is cleared?

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Mon, 19 Nov 2018, Andrea Arcangeli wrote:
> On Mon, Nov 19, 2018 at 01:33:08PM -0800, Dave Hansen wrote:
> > Here's the current description:
> > 
> > > Setting ... STIBP ... on a logical processor prevents the predicted
> > > targets of indirect branches on any logical processor of that core
>                                     ^^^
> > > from being controlled by software that executes (or executed
> > > previously) on another logical processor of the same core.
>                    ^^^^^^^
> > 
> > 1.
> > https://software.intel.com/security-software-guidance/insights/deep-dive-single-thread-indirect-branch-predictors
> 
> I'm not used to official specs in a "insight & deep dive"
> blog-post-like webpage, so I didn't notice this deep dive detail.

But you might have noticed the wording in:

  Speculative Execution Side Channel Mitigations
  Revision 2.0
  May 2018

which says:

  Setting bit 1 (STIBP) of the IA32_SPEC_CTRL MSR on a logical processor
  prevents the predicted targets of indirect branches on any logical
  processor of that core from being controlled by software that executes (or
  executed previously) on another logical processor of the same core.

i.e. exactly what Dave quoted from the deep dive thingy.

> You use "any" vs "any" but the spec you quoted still says "any" vs
> "another".
>
> If I shall take the above literally it still means that if I set STIBP
> inside SECCOMP, as long as it's set, it prevents indirect branches of
> all siblings to be controlled from code outside the SECCOMP jail
> running in another sibling (or that run previously in another
> sibling). I.e. the "deep dive" stronger semantics of STIBP just mean
> the code outside SECCOMP cannot attack itself while the code inside
> SECCOMP runs and keeps STIBP set.

The point is that when you enable STIBP on any sibling then all siblings of
the same core are protected against each other simply because the predictor
is shared. Otherwise you would hardly have sibling to sibling influence.

Yes, I agree the documentation is lousy, but I also agree with the
interpretation from Dave, Andi and Tim.

> If this is clarified the concern that remains is that lots of
> potentially performance critical stuff runs under SECCOMP but of
> course it changes everything in terms of possibly enabling STIBP under
> SECCOMP by default, it certainly would make more sense then.

Right. If this holds, which I assume, then enabling it for seccomp tasks
would make sense.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

Tim,

On Fri, 16 Nov 2018, Tim Chen wrote:

> Add new protection modes for Spectre v2 mitigations against
> Spectre v2 attacks on user processes.  There are three modes:
> 
> 	strict mode:
> 	In this mode, IBPB and STIBP are deployed full
> 	time to protect all processes.
> 
> 	lite mode:
> 	In this mode, IBPB and STIBP are only deployed on
> 	processes marked with TIF_STIBP flag.
> 
> 	none mode:
> 	In this mode, no mitigations are deployed.
>
> The protection mode can be specified by the spectre_v2_app2app
> boot parameter with the following semantics:
> 
> spectre_v2_app2app=
> 	off    - Turn off mitigation
> 	lite   - Protect processes which are marked non-dumpable
> 	strict - Protect all processes
> 	auto   - Kernel selects the mode

Is there any reason why we need yet another naming convention?

pti= 				on, off, auto

spectre_v2=			on, off, auto

spec_store_bypass_disable =	on, off, auto, prctl, seccomp


> 	Not specifying this option is equivalent to
> 	spectre_v2_app2app=auto.

For better understanding it's nowhere documented what auto does.

> +	spectre_v2_app2app=
> +			[X86] Control mitigation of Spectre variant 2
> +		        application to application (indirect branch speculation)
> +			vulnerability.
> +
> +			off    - Unconditionally disable mitigations
> +			lite   - Protect tasks which have requested restricted
> +				 indirect branch speculation via the
> +				 PR_SET_SPECULATION_CTRL prctl(). 
> +			strict - Protect all processes
> +			auto   - Kernel selects the mode
> +
> +			Not specifying this option is equivalent to
> +			spectre_v2_app2app=auto.
> +
> +			Setting spectre_v2=off will also turn off this mitigation.
> +
> +			Setting spectre_v2=on implies unconditionally enabling
> +			this mitigation.

Can we please have a full documentation for all the spectre_v2 stuff
similar to l1tf?

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 05:32 AM, Thomas Gleixner wrote:
> Tim,
> 
> On Fri, 16 Nov 2018, Tim Chen wrote:
> 
>> Add new protection modes for Spectre v2 mitigations against
>> Spectre v2 attacks on user processes.  There are three modes:
>>
>> 	strict mode:
>> 	In this mode, IBPB and STIBP are deployed full
>> 	time to protect all processes.
>>
>> 	lite mode:
>> 	In this mode, IBPB and STIBP are only deployed on
>> 	processes marked with TIF_STIBP flag.
>>
>> 	none mode:
>> 	In this mode, no mitigations are deployed.
>>
>> The protection mode can be specified by the spectre_v2_app2app
>> boot parameter with the following semantics:
>>
>> spectre_v2_app2app=
>> 	off    - Turn off mitigation
>> 	lite   - Protect processes which are marked non-dumpable
>> 	strict - Protect all processes
>> 	auto   - Kernel selects the mode
> 
> Is there any reason why we need yet another naming convention?
> 
> pti= 				on, off, auto
> 
> spectre_v2=			on, off, auto
> 
> spec_store_bypass_disable =	on, off, auto, prctl, seccomp

The "on" option is set by spectre_v2=on so is not specified here.
What will you like to name the "lite" and "strict" option instead?

> 
> 
>> 	Not specifying this option is equivalent to
>> 	spectre_v2_app2app=auto.
> 
> For better understanding it's nowhere documented what auto does.

I'll add the documentation.

> 
>> +	spectre_v2_app2app=
>> +			[X86] Control mitigation of Spectre variant 2
>> +		        application to application (indirect branch speculation)
>> +			vulnerability.
>> +
>> +			off    - Unconditionally disable mitigations
>> +			lite   - Protect tasks which have requested restricted
>> +				 indirect branch speculation via the
>> +				 PR_SET_SPECULATION_CTRL prctl(). 
>> +			strict - Protect all processes
>> +			auto   - Kernel selects the mode
>> +
>> +			Not specifying this option is equivalent to
>> +			spectre_v2_app2app=auto.
>> +
>> +			Setting spectre_v2=off will also turn off this mitigation.
>> +
>> +			Setting spectre_v2=on implies unconditionally enabling
>> +			this mitigation.
> 
> Can we please have a full documentation for all the spectre_v2 stuff
> similar to l1tf?
> 

Sure.  Can we do that as a separate patch?  I'll need some time 
and internal review for any spectre_v2 documentation that's produced.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Mon, 19 Nov 2018, Tim Chen wrote:
> On 11/19/2018 05:32 AM, Thomas Gleixner wrote:
> > On Fri, 16 Nov 2018, Tim Chen wrote:
> >> The protection mode can be specified by the spectre_v2_app2app
> >> boot parameter with the following semantics:
> >>
> >> spectre_v2_app2app=
> >> 	off    - Turn off mitigation
> >> 	lite   - Protect processes which are marked non-dumpable
> >> 	strict - Protect all processes
> >> 	auto   - Kernel selects the mode
> > 
> > Is there any reason why we need yet another naming convention?
> > 
> > pti= 				on, off, auto
> > 
> > spectre_v2=			on, off, auto
> > 
> > spec_store_bypass_disable =	on, off, auto, prctl, seccomp
> 
> The "on" option is set by spectre_v2=on so is not specified here.

What has spectre_v2=on to do with spectre_v2_app2app=on?

Exactly nothing. You can have 'on' for both. The only side effect of
spectre_v2=on is that it also forces spectre_v2_app2app to 'on'
irrespective of what eventually was added for spectre_v2_app2app= on the
command line.

> What will you like to name the "lite" and "strict" option instead?

'prctl' and 'on' and if we add 'seccomp' then this is exactly the same as
we have for ssbd.

> > Can we please have a full documentation for all the spectre_v2 stuff
> > similar to l1tf?
> > 
> Sure.  Can we do that as a separate patch?  I'll need some time 
> and internal review for any spectre_v2 documentation that's produced.

I'm not taking that stuff without proper documentation. I complained about
that vs. L1TF and got told that no sysadmin cares, but L1TF has shown that
they care very much and appreciate proper documentation.

Nobody can oracle the protection scope out of that inconsistent command
line maze.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 04:30 PM, Thomas Gleixner wrote:
> 
> What has spectre_v2=on to do with spectre_v2_app2app=on?
> 
> Exactly nothing. You can have 'on' for both. The only side effect of
> spectre_v2=on is that it also forces spectre_v2_app2app to 'on'
> irrespective of what eventually was added for spectre_v2_app2app= on the
> command line.
> 
>> What will you like to name the "lite" and "strict" option instead?
> 
> 'prctl' and 'on' and if we add 'seccomp' then this is exactly the same as
> we have for ssbd.
> 
>>> Can we please have a full documentation for all the spectre_v2 stuff
>>> similar to l1tf?
>>>
>> Sure.  Can we do that as a separate patch?  I'll need some time 
>> and internal review for any spectre_v2 documentation that's produced.
> 
> I'm not taking that stuff without proper documentation. I complained about
> that vs. L1TF and got told that no sysadmin cares, but L1TF has shown that
> they care very much and appreciate proper documentation.
> 
> Nobody can oracle the protection scope out of that inconsistent command
> line maze.
> 

Thomas,

I'm not a good writer and it will take me some time to have the documentation
and boot options code ironed out.  In the mean time, will it make sense
to revert Jiri's patchset, till we have this patchset and associated
documentation updated to everyone's liking?

Thanks.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Andi Kleen]

> I'm not taking that stuff without proper documentation.

Ok can you just revert Jiri's code then instead?

This is after all mainly an emergency fixup patch for that disaster,
which got fast tracked without any of these considerations
which now suddenly appear. Requiring new documentation for old
for old existing code as part of emergency patches definitely seems to
be unwarranted scope creep.

> I complained about
> that vs. L1TF and got told that no sysadmin cares, but L1TF has shown that
> they care very much and appreciate proper documentation.

I suspect what they mainly want is a single global switch to turn it all off...
which we sadly lack.

-Andi

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Fri, 16 Nov 2018, Tim Chen wrote:
> +DEFINE_STATIC_KEY_FALSE(spectre_v2_app_lite);
> +EXPORT_SYMBOL_GPL(spectre_v2_app_lite);

Why would this be exported? The only usage site outside of this code is in
tlb.c which is hardly modular.

> @@ -328,14 +411,19 @@ static bool stibp_needed(void)
>  	/*
>  	 * Determine if STIBP should be always on.
>  	 * Using enhanced IBRS makes using STIBP unnecessary.
> +	 * For lite option, STIBP is used only for task with
> +	 * TIF_STIBP flag. STIBP is not always on for that case.

Having the comment detached from the code is really not helpful.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 07:00 AM, Thomas Gleixner wrote:
> On Fri, 16 Nov 2018, Tim Chen wrote:
>> +DEFINE_STATIC_KEY_FALSE(spectre_v2_app_lite);
>> +EXPORT_SYMBOL_GPL(spectre_v2_app_lite);
> 
> Why would this be exported? The only usage site outside of this code is in
> tlb.c which is hardly modular.

That was my initial thought too.  Ingo suggested to export it in
review of v2.  Wonder Ingo has some reason?

https://lore.kernel.org/patchwork/patch/991759/

> 
>> @@ -328,14 +411,19 @@ static bool stibp_needed(void)
>>  	/*
>>  	 * Determine if STIBP should be always on.
>>  	 * Using enhanced IBRS makes using STIBP unnecessary.
>> +	 * For lite option, STIBP is used only for task with
>> +	 * TIF_STIBP flag. STIBP is not always on for that case.
> 
> Having the comment detached from the code is really not helpful.
> 

Sorry, would have the comment moved.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Mon, 19 Nov 2018, Tim Chen wrote:

> >> +DEFINE_STATIC_KEY_FALSE(spectre_v2_app_lite);
> >> +EXPORT_SYMBOL_GPL(spectre_v2_app_lite);
> > 
> > Why would this be exported? The only usage site outside of this code is in
> > tlb.c which is hardly modular.
> 
> That was my initial thought too.  Ingo suggested to export it in
> review of v2.  Wonder Ingo has some reason?
> 
> https://lore.kernel.org/patchwork/patch/991759/

But why does it have to be exported at all, irrespective whether _GPL() or 
not?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Fri, 16 Nov 2018, Tim Chen wrote:
> +static enum spectre_v2_app2app_mitigation_cmd __init
> +	    spectre_v2_parse_app2app_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
> +{
> +	enum spectre_v2_app2app_mitigation_cmd cmd;
> +	char arg[20];
> +	int ret, i;
> +
> +	if (v2_cmd == SPECTRE_V2_CMD_FORCE) {
> +		cmd = SPECTRE_V2_APP2APP_CMD_FORCE;
> +		goto show_cmd;

What initializes 'i' in this case? Compiler warnings are overrated.

What exactly handles the SPECTRE_V2_NONE case which you documented in the
commandline bits?

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 12:21 PM, Thomas Gleixner wrote:
> On Fri, 16 Nov 2018, Tim Chen wrote:
>> +static enum spectre_v2_app2app_mitigation_cmd __init
>> +	    spectre_v2_parse_app2app_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
>> +{
>> +	enum spectre_v2_app2app_mitigation_cmd cmd;
>> +	char arg[20];
>> +	int ret, i;
>> +
>> +	if (v2_cmd == SPECTRE_V2_CMD_FORCE) {
>> +		cmd = SPECTRE_V2_APP2APP_CMD_FORCE;
>> +		goto show_cmd;
> 
> What initializes 'i' in this case? Compiler warnings are overrated.

Will fix.

> 
> What exactly handles the SPECTRE_V2_NONE case which you documented in the
> commandline bits?
> 

The default spectre_v2_app2app_enabled is set to SPECTRE_V2_APP2APP_NONE.

When we have SPECTRE_V2_NONE case, we will have return in spectre_v2_select_mitigation
without calling spectre_v2_parse_app2app_cmdline. So we will have
SPECTRE_V2_APP2APP_NONE.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Fri, 16 Nov 2018, Tim Chen wrote:
> +static const struct {
> +	const char *option;
> +	enum spectre_v2_app2app_mitigation_cmd cmd;
> +	bool secure;
> +} app2app_options[] = {
> +	{ "off",	SPECTRE_V2_APP2APP_CMD_NONE,   false },
> +	{ "lite",	SPECTRE_V2_APP2APP_CMD_LITE,   false },
> +	{ "strict",	SPECTRE_V2_APP2APP_CMD_STRICT, false },
> +	{ "auto",	SPECTRE_V2_APP2APP_CMD_AUTO,   false },
> +	/*
> +	 * The "on" option is kept as last entry. It is implied by
> +	 * spectre_v2=on boot parameter and it is not checked
> +	 * in spectre_v2_app2app boot parameter.
> +	 */
> +	{ "on",		SPECTRE_V2_APP2APP_CMD_FORCE,  true  },

FORCE is the same as STRICT. What's the point?

> @@ -376,6 +464,8 @@ static void __init spectre_v2_select_mitigation(void)
>  {
>  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
>  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
> +	enum spectre_v2_app2app_mitigation_cmd app2app_cmd;
> +	enum spectre_v2_app2app_mitigation app2app_mode;
>  
>  	/*
>  	 * If the CPU is not affected and the command line mode is NONE or AUTO
> @@ -452,12 +542,6 @@ static void __init spectre_v2_select_mitigation(void)
>  	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
>  	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
>  
> -	/* Initialize Indirect Branch Prediction Barrier if supported */
> -	if (boot_cpu_has(X86_FEATURE_IBPB)) {
> -		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> -		pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");
> -	}
> -
>  	/*
>  	 * Retpoline means the kernel is safe because it has no indirect
>  	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
> @@ -474,6 +558,43 @@ static void __init spectre_v2_select_mitigation(void)
>  		pr_info("Enabling Restricted Speculation for firmware calls\n");
>  	}
>  
> +	app2app_mode = SPECTRE_V2_APP2APP_NONE;
> +	if (!boot_cpu_has(X86_FEATURE_IBPB) ||
> +	    !boot_cpu_has(X86_FEATURE_STIBP))
> +		goto set_app2app_mode;

So before that change IBPB was usable without STIBP, now not longer. What's
the rationale?

This patch changes a gazillion things at once and is completely
unreviewable.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Mon, 19 Nov 2018, Thomas Gleixner wrote:

> > @@ -452,12 +542,6 @@ static void __init spectre_v2_select_mitigation(void)
> >  	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
> >  	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
> >  
> > -	/* Initialize Indirect Branch Prediction Barrier if supported */
> > -	if (boot_cpu_has(X86_FEATURE_IBPB)) {
> > -		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> > -		pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");
> > -	}
> > -
> >  	/*
> >  	 * Retpoline means the kernel is safe because it has no indirect
> >  	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > @@ -474,6 +558,43 @@ static void __init spectre_v2_select_mitigation(void)
> >  		pr_info("Enabling Restricted Speculation for firmware calls\n");
> >  	}
> >  
> > +	app2app_mode = SPECTRE_V2_APP2APP_NONE;
> > +	if (!boot_cpu_has(X86_FEATURE_IBPB) ||
> > +	    !boot_cpu_has(X86_FEATURE_STIBP))
> > +		goto set_app2app_mode;
> 
> So before that change IBPB was usable without STIBP, now not longer. What's
> the rationale?
> 
> This patch changes a gazillion things at once and is completely
> unreviewable.

The patchset actually ties together IBPB and STIBP pretty closely, which 
is IMO a good thing; there is no good reason why anone would want just one 
of those (or each in a different mode), at least before this magical 
coscheduling exists.

But I guess this fact should be documented somewhere.

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Mon, 19 Nov 2018, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Thomas Gleixner wrote:
> 
> > > @@ -452,12 +542,6 @@ static void __init spectre_v2_select_mitigation(void)
> > >  	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
> > >  	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
> > >  
> > > -	/* Initialize Indirect Branch Prediction Barrier if supported */
> > > -	if (boot_cpu_has(X86_FEATURE_IBPB)) {
> > > -		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> > > -		pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");
> > > -	}
> > > -
> > >  	/*
> > >  	 * Retpoline means the kernel is safe because it has no indirect
> > >  	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > > @@ -474,6 +558,43 @@ static void __init spectre_v2_select_mitigation(void)
> > >  		pr_info("Enabling Restricted Speculation for firmware calls\n");
> > >  	}
> > >  
> > > +	app2app_mode = SPECTRE_V2_APP2APP_NONE;
> > > +	if (!boot_cpu_has(X86_FEATURE_IBPB) ||
> > > +	    !boot_cpu_has(X86_FEATURE_STIBP))
> > > +		goto set_app2app_mode;
> > 
> > So before that change IBPB was usable without STIBP, now not longer. What's
> > the rationale?
> > 
> > This patch changes a gazillion things at once and is completely
> > unreviewable.
> 
> The patchset actually ties together IBPB and STIBP pretty closely, which 
> is IMO a good thing; there is no good reason why anone would want just one 
> of those (or each in a different mode), at least before this magical 
> coscheduling exists.
> 
> But I guess this fact should be documented somewhere.

That and it can be split in pieces so it actually becomes reviewable.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 12:55 PM, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Thomas Gleixner wrote:
> 
>>
>> So before that change IBPB was usable without STIBP, now not longer. What's
>> the rationale?
>>
>> This patch changes a gazillion things at once and is completely
>> unreviewable.
> 
> The patchset actually ties together IBPB and STIBP pretty closely, which 
> is IMO a good thing; there is no good reason why anone would want just one 
> of those (or each in a different mode), at least before this magical 
> coscheduling exists.
> 
> But I guess this fact should be documented somewhere.
> 

Yes, it wouldn't make sense for having just one of those if a task
is worried about attack from user space.

I'll document it.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Mon, 19 Nov 2018, Tim Chen wrote:
> On 11/19/2018 12:55 PM, Jiri Kosina wrote:
> > On Mon, 19 Nov 2018, Thomas Gleixner wrote:
> > 
> >>
> >> So before that change IBPB was usable without STIBP, now not longer. What's
> >> the rationale?
> >>
> >> This patch changes a gazillion things at once and is completely
> >> unreviewable.
> > 
> > The patchset actually ties together IBPB and STIBP pretty closely, which 
> > is IMO a good thing; there is no good reason why anone would want just one 
> > of those (or each in a different mode), at least before this magical 
> > coscheduling exists.
> > 
> > But I guess this fact should be documented somewhere.
> > 
> 
> Yes, it wouldn't make sense for having just one of those if a task
> is worried about attack from user space.
> 
> I'll document it.

What? IBPB makes tons of sense even without STIBP.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Tue, 20 Nov 2018, Thomas Gleixner wrote:

> What? IBPB makes tons of sense even without STIBP.

On non-SMT, yes. But this patchset ties those two the other (sensible) way 
around AFAICS ("STIBP iff (IBPB && SMT)").

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Tue, 20 Nov 2018, Jiri Kosina wrote:

> On Tue, 20 Nov 2018, Thomas Gleixner wrote:
> 
> > What? IBPB makes tons of sense even without STIBP.
> 
> On non-SMT, yes. But this patchset ties those two the other (sensible) way 
> around AFAICS ("STIBP iff (IBPB && SMT)").

Errm. No.

The patches disable IBPB if STIBP is not available and that has absolutely
nothing to do with SMT simply because the static key controlling IBPB is
only flipped on when both IBPB and STIBP are available.

For SMT=off STIBP is pointless, but IBPB still makes a lot of sense.

That's a change which is neither documented nor correct.

Thanks,

	tglx

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Tim Chen]

On 11/19/2018 04:00 PM, Thomas Gleixner wrote:
> On Tue, 20 Nov 2018, Jiri Kosina wrote:
> 
>> On Tue, 20 Nov 2018, Thomas Gleixner wrote:
>>
>>> What? IBPB makes tons of sense even without STIBP.
>>
>> On non-SMT, yes. But this patchset ties those two the other (sensible) way 
>> around AFAICS ("STIBP iff (IBPB && SMT)").
> 
> Errm. No.
> 
> The patches disable IBPB if STIBP is not available and that has absolutely
> nothing to do with SMT simply because the static key controlling IBPB is
> only flipped on when both IBPB and STIBP are available.

Ok, I assume you will like this change

@@ -560,7 +559,7 @@ static void __init spectre_v2_select_mitigation(void)
        }
 
        app2app_mode = SPECTRE_V2_APP2APP_NONE;
-       if (!boot_cpu_has(X86_FEATURE_IBPB) ||
+       if (!boot_cpu_has(X86_FEATURE_IBPB) &&
            !boot_cpu_has(X86_FEATURE_STIBP))
                goto set_app2app_mode;
 
before setting the IBPB feature.  So we will still do the
IBPB feature set even if STIBP feature is unavailable.

        /*
         * Initialize Indirect Branch Prediction Barrier if supported
         * and not disabled explicitly
         */
        if (app2app_mode != SPECTRE_V2_APP2APP_NONE) {
                setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
                pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");
        }

In that case I'll need to add a static_cpu_has feature flag check when
we are using STIBP and can't assume that when lite mode is on, STIBP is there.
We already check the cpu feature flag in IBPB path so we're good there.

When I first wrote the code, I had in mind that CPU that has one will have
the other.  But that assumption is best avoided.

Tim

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Dave Hansen]

On 11/19/18 3:01 PM, Thomas Gleixner wrote:
>> Yes, it wouldn't make sense for having just one of those if a task
>> is worried about attack from user space.
>>
>> I'll document it.
> What? IBPB makes tons of sense even without STIBP.

I'm lost. :)

I don't think anyone is talking about using STIBP *everywhere* that IBPB
is in-use.

We're just guessing that, if anybody is paranoid enough to ask for IBPB,
*and* they have SMT, they almost certainly want STIBP too.

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Jiri Kosina]

On Mon, 19 Nov 2018, Dave Hansen wrote:

> > What? IBPB makes tons of sense even without STIBP.
> 
> I'm lost. :)
> 
> I don't think anyone is talking about using STIBP *everywhere* that IBPB
> is in-use.
> 
> We're just guessing that, if anybody is paranoid enough to ask for IBPB,
> *and* they have SMT, they almost certainly want STIBP too.

I think you are not lost :) and this is exactly what makes sense, and what 
Tim's patchset implements.

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 11/16] x86/speculation: Add Spectre v2 app to app protection modes

[Thomas Gleixner]

On Tue, 20 Nov 2018, Jiri Kosina wrote:
> On Mon, 19 Nov 2018, Dave Hansen wrote:
> 
> > > What? IBPB makes tons of sense even without STIBP.
> > 
> > I'm lost. :)
> > 
> > I don't think anyone is talking about using STIBP *everywhere* that IBPB
> > is in-use.
> > 
> > We're just guessing that, if anybody is paranoid enough to ask for IBPB,
> > *and* they have SMT, they almost certainly want STIBP too.
> 
> I think you are not lost :) and this is exactly what makes sense, and what 
> Tim's patchset implements.

Tries to implement perhaps. Unless IBPB is never available when STIBP is
not available, but according to documentation that's unlikely because STIBP
can be unset when the CPU does not support HT at all.

Thanks,

	tglx

[Patch v5 12/16] x86/speculation: Create PRCTL interface to restrict indirect branch speculation

[Tim Chen]

Create PRCTL interface to restrict an application's indirect branch
speculation.  This will protect the application against spectre v2 attack
from another application.

Invocations:
Check indirect branch speculation status with
- prctl(PR_GET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, 0, 0, 0);

Enable indirect branch speculation with
- prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);

Disable indirect branch speculation with
- prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_DISABLE, 0, 0);

Force disable indirect branch speculation with
- prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_FORCE_DISABLE, 0, 0);

See Documentation/userspace-api/spec_ctrl.rst.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 Documentation/userspace-api/spec_ctrl.rst |  9 ++++
 arch/x86/kernel/cpu/bugs.c                | 80 +++++++++++++++++++++++++++++++
 include/linux/sched.h                     |  9 ++++
 include/uapi/linux/prctl.h                |  1 +
 tools/include/uapi/linux/prctl.h          |  1 +
 5 files changed, 100 insertions(+)

diff --git a/Documentation/userspace-api/spec_ctrl.rst b/Documentation/userspace-api/spec_ctrl.rst
index 32f3d55..8a4e268 100644
--- a/Documentation/userspace-api/spec_ctrl.rst
+++ b/Documentation/userspace-api/spec_ctrl.rst
@@ -92,3 +92,12 @@ Speculation misfeature controls
    * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_STORE_BYPASS, PR_SPEC_ENABLE, 0, 0);
    * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_STORE_BYPASS, PR_SPEC_DISABLE, 0, 0);
    * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_STORE_BYPASS, PR_SPEC_FORCE_DISABLE, 0, 0);
+
+- PR_SPEC_INDIR_BRANCH: Indirect Branch Speculation in User Processes
+                        (Mitigate Spectre V2 style attacks against user processes)
+
+  Invocations:
+   * prctl(PR_GET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, 0, 0, 0);
+   * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);
+   * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_DISABLE, 0, 0);
+   * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_FORCE_DISABLE, 0, 0);
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 21caade..8f5187e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -773,12 +773,69 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
 	return 0;
 }
 
+static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
+{
+	bool update = false;
+
+	if (stibp_on)
+		update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
+	else
+		update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
+
+	if (tsk == current && update)
+		speculation_ctrl_update_current();
+}
+
+static int indir_branch_prctl_set(struct task_struct *task, unsigned long ctrl)
+{
+	switch (ctrl) {
+	case PR_SPEC_ENABLE:
+		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
+			return 0;
+		/*
+		 * Indirect branch speculation is always disabled in
+		 * strict mode.
+		 */
+		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_STRICT)
+			return -EPERM;
+		task_clear_spec_indir_branch_disable(task);
+		set_task_stibp(task, false);
+		break;
+	case PR_SPEC_DISABLE:
+		/*
+		 * Indirect branch speculation is always allowed when
+		 * mitigation is force disabled.
+		 */
+		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
+			return -EPERM;
+		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_STRICT)
+			return 0;
+		task_set_spec_indir_branch_disable(task);
+		set_task_stibp(task, true);
+		break;
+	case PR_SPEC_FORCE_DISABLE:
+		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
+			return -EPERM;
+		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_STRICT)
+			return 0;
+		task_set_spec_indir_branch_disable(task);
+		task_set_spec_indir_branch_force_disable(task);
+		set_task_stibp(task, true);
+		break;
+	default:
+		return -ERANGE;
+	}
+	return 0;
+}
+
 int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 			     unsigned long ctrl)
 {
 	switch (which) {
 	case PR_SPEC_STORE_BYPASS:
 		return ssb_prctl_set(task, ctrl);
+	case PR_SPEC_INDIR_BRANCH:
+		return indir_branch_prctl_set(task, ctrl);
 	default:
 		return -ENODEV;
 	}
@@ -811,11 +868,34 @@ static int ssb_prctl_get(struct task_struct *task)
 	}
 }
 
+static int indir_branch_prctl_get(struct task_struct *task)
+{
+	if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
+		return PR_SPEC_NOT_AFFECTED;
+
+	switch (spectre_v2_app2app_enabled) {
+	case SPECTRE_V2_APP2APP_NONE:
+		return PR_SPEC_ENABLE;
+	case SPECTRE_V2_APP2APP_LITE:
+		if (task_spec_indir_branch_force_disable(task))
+			return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE;
+		if (test_tsk_thread_flag(task, TIF_STIBP))
+			return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
+		return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
+	case SPECTRE_V2_APP2APP_STRICT:
+		return PR_SPEC_DISABLE;
+	default:
+		return PR_SPEC_NOT_AFFECTED;
+	}
+}
+
 int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which)
 {
 	switch (which) {
 	case PR_SPEC_STORE_BYPASS:
 		return ssb_prctl_get(task);
+	case PR_SPEC_INDIR_BRANCH:
+		return indir_branch_prctl_get(task);
 	default:
 		return -ENODEV;
 	}
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a51c13c..e92e4bf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1453,6 +1453,8 @@ static inline bool is_percpu_thread(void)
 #define PFA_SPREAD_SLAB			2	/* Spread some slab caches over cpuset */
 #define PFA_SPEC_SSB_DISABLE		3	/* Speculative Store Bypass disabled */
 #define PFA_SPEC_SSB_FORCE_DISABLE	4	/* Speculative Store Bypass force disabled*/
+#define PFA_SPEC_INDIR_BRANCH_DISABLE	5	/* Indirect branch speculation restricted in apps */
+#define PFA_SPEC_INDIR_BRANCH_FORCE_DISABLE 6	/* Indirect branch speculation restricted in apps forced */
 
 #define TASK_PFA_TEST(name, func)					\
 	static inline bool task_##func(struct task_struct *p)		\
@@ -1484,6 +1486,13 @@ TASK_PFA_CLEAR(SPEC_SSB_DISABLE, spec_ssb_disable)
 TASK_PFA_TEST(SPEC_SSB_FORCE_DISABLE, spec_ssb_force_disable)
 TASK_PFA_SET(SPEC_SSB_FORCE_DISABLE, spec_ssb_force_disable)
 
+TASK_PFA_TEST(SPEC_INDIR_BRANCH_DISABLE, spec_indir_branch_disable)
+TASK_PFA_SET(SPEC_INDIR_BRANCH_DISABLE, spec_indir_branch_disable)
+TASK_PFA_CLEAR(SPEC_INDIR_BRANCH_DISABLE, spec_indir_branch_disable)
+
+TASK_PFA_TEST(SPEC_INDIR_BRANCH_FORCE_DISABLE, spec_indir_branch_force_disable)
+TASK_PFA_SET(SPEC_INDIR_BRANCH_FORCE_DISABLE, spec_indir_branch_force_disable)
+
 static inline void
 current_restore_flags(unsigned long orig_flags, unsigned long flags)
 {
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index c0d7ea0..577f2ca 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -212,6 +212,7 @@ struct prctl_mm_map {
 #define PR_SET_SPECULATION_CTRL		53
 /* Speculation control variants */
 # define PR_SPEC_STORE_BYPASS		0
+# define PR_SPEC_INDIR_BRANCH		1
 /* Return and control values for PR_SET/GET_SPECULATION_CTRL */
 # define PR_SPEC_NOT_AFFECTED		0
 # define PR_SPEC_PRCTL			(1UL << 0)
diff --git a/tools/include/uapi/linux/prctl.h b/tools/include/uapi/linux/prctl.h
index c0d7ea0..577f2ca 100644
--- a/tools/include/uapi/linux/prctl.h
+++ b/tools/include/uapi/linux/prctl.h
@@ -212,6 +212,7 @@ struct prctl_mm_map {
 #define PR_SET_SPECULATION_CTRL		53
 /* Speculation control variants */
 # define PR_SPEC_STORE_BYPASS		0
+# define PR_SPEC_INDIR_BRANCH		1
 /* Return and control values for PR_SET/GET_SPECULATION_CTRL */
 # define PR_SPEC_NOT_AFFECTED		0
 # define PR_SPEC_PRCTL			(1UL << 0)
-- 
2.9.4

Re: [Patch v5 12/16] x86/speculation: Create PRCTL interface to restrict indirect branch speculation

[Jiri Kosina]

On Fri, 16 Nov 2018, Tim Chen wrote:

> Create PRCTL interface to restrict an application's indirect branch
> speculation.  This will protect the application against spectre v2 attack
> from another application.
> 
> Invocations:
> Check indirect branch speculation status with
> - prctl(PR_GET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, 0, 0, 0);
> 
> Enable indirect branch speculation with
> - prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_ENABLE, 0, 0);
> 
> Disable indirect branch speculation with
> - prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_DISABLE, 0, 0);
> 
> Force disable indirect branch speculation with
> - prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_INDIR_BRANCH, PR_SPEC_FORCE_DISABLE, 0, 0);
> 
> See Documentation/userspace-api/spec_ctrl.rst.

I think that the fact that this talks about "indirect branch predictions" 
in general terms, but really controls only the SMT aspect of it (STIBP), 
as quite confusing.

So I believe it should either be renamed, or actually control semantics of 
IBPB as well, no?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [Patch v5 12/16] x86/speculation: Create PRCTL interface to restrict indirect branch speculation

[Tim Chen]

On 11/17/2018 01:53 AM, Jiri Kosina wrote:
> On Fri, 16 Nov 2018, Tim Chen wrote:
> 
> 
> I think that the fact that this talks about "indirect branch predictions" 
> in general terms, but really controls only the SMT aspect of it (STIBP), 
> as quite confusing.
> 
> So I believe it should either be renamed, or actually control semantics of 
> IBPB as well, no?

It does control STIBP.  You have suggestion on an alternative name?

Tim

[Patch v5 13/16] security: Update speculation restriction of a process when modifying its dumpability

[Tim Chen]

When a task is made non-dumpable, a higher level of security is implied
implicitly as its memory is imposed with access restriction.  Many
daemons touching sensitive data (e.g. sshd) make theselves non-dumpable.
Such tasks should have speculative execution restricted to protect them
from attacks taking advantage of CPU speculation side channels.

Add calls to arch_update_spec_restiction() to put speculative restriction
on a task when changing its dumpability.  Restrict speculative execution
on a non-dumpable task and relax the restrictions on a dumpable task.

A change to dumpability occurs via setgid, setuid, or
prctl(SUID_SET_DUMPABLE) syscalls.  The user should expect associated
change in speculative restriction occurs only on the task that issued
such syscall. Speculative restriction changes are not extended to other
threads in the same process.  This should not be a problem as such
changes should be made before spawning additional threads.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 fs/exec.c           | 3 +++
 include/linux/cpu.h | 3 +++
 kernel/cpu.c        | 4 ++++
 kernel/cred.c       | 5 ++++-
 kernel/sys.c        | 7 +++++++
 5 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index fc281b7..d72e20d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -62,6 +62,7 @@
 #include <linux/oom.h>
 #include <linux/compat.h>
 #include <linux/vmalloc.h>
+#include <linux/cpu.h>
 
 #include <linux/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1366,6 +1367,8 @@ void setup_new_exec(struct linux_binprm * bprm)
 	else
 		set_dumpable(current->mm, SUID_DUMP_USER);
 
+	arch_update_spec_restriction(current);
+
 	arch_setup_new_exec();
 	perf_event_exec();
 	__set_task_comm(current, kbasename(bprm->filename), true);
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 00af2ae..3d90155 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -182,4 +182,7 @@ static inline void cpu_smt_check_topology(void) { }
 #endif
 DECLARE_STATIC_KEY_TRUE(cpu_smt_enabled);
 
+/* Update CPU's speculation restrictions on a task based on task's properties */
+extern int arch_update_spec_restriction(struct task_struct *task);
+
 #endif /* _LINUX_CPU_H_ */
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 54cf3f6..d57ab2c 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2291,6 +2291,10 @@ void init_cpu_online(const struct cpumask *src)
 	cpumask_copy(&__cpu_online_mask, src);
 }
 
+int __weak arch_update_spec_ctrl_restriction(struct task_struct *task)
+{
+}
+
 /*
  * Activate the first processor.
  */
diff --git a/kernel/cred.c b/kernel/cred.c
index ecf0365..bc47653 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -19,6 +19,7 @@
 #include <linux/security.h>
 #include <linux/binfmts.h>
 #include <linux/cn_proc.h>
+#include <linux/cpu.h>
 
 #if 0
 #define kdebug(FMT, ...)						\
@@ -445,8 +446,10 @@ int commit_creds(struct cred *new)
 	    !uid_eq(old->fsuid, new->fsuid) ||
 	    !gid_eq(old->fsgid, new->fsgid) ||
 	    !cred_cap_issubset(old, new)) {
-		if (task->mm)
+		if (task->mm) {
 			set_dumpable(task->mm, suid_dumpable);
+			arch_update_spec_restriction(task);
+		}
 		task->pdeath_signal = 0;
 		smp_wmb();
 	}
diff --git a/kernel/sys.c b/kernel/sys.c
index 123bd73..621ea94 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2290,6 +2290,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			break;
 		}
 		set_dumpable(me->mm, arg2);
+		/*
+		 * Any speculative execution restriction updates
+		 * associated with change in dumpability
+		 * applies only to the current task that issues
+		 * the request.
+		 */
+		arch_update_spec_restriction(me);
 		break;
 
 	case PR_SET_UNALIGN:
-- 
2.9.4

[Patch v5 14/16] x86/speculation: Use STIBP to restrict speculation on non-dumpable task

[Tim Chen]

When a task changes its dumpability, arch_update_spec_ctrl_restriction()
is called to place restriction on the task's speculative execution
according to dumpability changes.

Implements arch_update_spec_restriction() for x86.  Use STIBP to
restrict speculative execution when running a task set to non-dumpable,
or clear the restriction if the task is set to dumpable.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  3 ++-
 arch/x86/kernel/cpu/bugs.c                      | 21 +++++++++++++++++++--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9c306e3..102f9a1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4221,7 +4221,8 @@
 			vulnerability.
 
 			off    - Unconditionally disable mitigations
-			lite   - Protect tasks which have requested restricted
+			lite   - Protect tasks which are marked non-dumpable
+				 and tasks which have requested restricted
 				 indirect branch speculation via the
 				 PR_SET_SPECULATION_CTRL prctl(). 
 			strict - Protect all processes
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 8f5187e..e7f9334 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/nospec.h>
 #include <linux/prctl.h>
+#include <linux/coredump.h>
 
 #include <asm/spec-ctrl.h>
 #include <asm/cmdline.h>
@@ -152,7 +153,7 @@ static const char *spectre_v2_strings[] = {
 
 static const char *spectre_v2_app2app_strings[] = {
 	[SPECTRE_V2_APP2APP_NONE]   = "App-App Vulnerable",
-	[SPECTRE_V2_APP2APP_LITE]   = "App-App Mitigation: Protect branch speculation restricted tasks",
+	[SPECTRE_V2_APP2APP_LITE]   = "App-App Mitigation: Protect non-dumpable and branch speculation restricted tasks",
 	[SPECTRE_V2_APP2APP_STRICT] = "App-App Mitigation: Full app to app attack protection",
 };
 
@@ -779,13 +780,29 @@ static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
 
 	if (stibp_on)
 		update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
-	else
+	else if (!task_spec_indir_branch_disable(tsk))
 		update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
 
 	if (tsk == current && update)
 		speculation_ctrl_update_current();
 }
 
+int arch_update_spec_restriction(struct task_struct *task)
+{
+	if (!static_branch_unlikely(&spectre_v2_app_lite))
+		return 0;
+
+	if (!task->mm)
+		return -EINVAL;
+
+	if (get_dumpable(task->mm) != SUID_DUMP_USER)
+		set_task_stibp(task, true);
+	else
+		set_task_stibp(task, false);
+
+	return 0;
+}
+
 static int indir_branch_prctl_set(struct task_struct *task, unsigned long ctrl)
 {
 	switch (ctrl) {
-- 
2.9.4

[Patch v5 15/16] x86/speculation: Update comment on TIF_SSBD

[Tim Chen]

The comment to TIF_SSBD uses the obsoleted name
"Reduced Data Speculation".

Update comment use the correct name
"Speculative store bypass disable".

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/include/asm/thread_info.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 8736d23..a8c5e52 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -79,7 +79,7 @@ struct thread_info {
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
 #define TIF_SINGLESTEP		4	/* reenable singlestep on user return*/
-#define TIF_SSBD			5	/* Reduced data speculation */
+#define TIF_SSBD		5	/* Speculative store bypass disable */
 #define TIF_SYSCALL_EMU		6	/* syscall emulation active */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
-- 
2.9.4

[Patch v5 16/16] x86: Group thread info flags by functionality

[Tim Chen]

The thread info flags are currently randomly distributed in the header
file arch/x86/include/asm/thread_info.h.

Group TIF flags together according to the following categories:
operation mode, syscall mode, pending work, task status and security
status.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/include/asm/thread_info.h | 97 ++++++++++++++++++++++----------------
 1 file changed, 56 insertions(+), 41 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index a8c5e52..453616f 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -74,60 +74,75 @@ struct thread_info {
  * - these are process state flags that various assembly files
  *   may need to access
  */
-#define TIF_SYSCALL_TRACE	0	/* syscall trace active */
-#define TIF_NOTIFY_RESUME	1	/* callback before returning to user */
-#define TIF_SIGPENDING		2	/* signal pending */
-#define TIF_NEED_RESCHED	3	/* rescheduling necessary */
-#define TIF_SINGLESTEP		4	/* reenable singlestep on user return*/
-#define TIF_SSBD		5	/* Speculative store bypass disable */
-#define TIF_SYSCALL_EMU		6	/* syscall emulation active */
-#define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
-#define TIF_SECCOMP		8	/* secure computing */
-#define TIF_STIBP		9	/* Single thread indirect branch speculation */
-#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 */
-#define TIF_NOCPUID		15	/* CPUID is not accessible in userland */
-#define TIF_NOTSC		16	/* TSC is not accessible in userland */
-#define TIF_IA32		17	/* IA32 compatibility process */
-#define TIF_NOHZ		19	/* in adaptive nohz mode */
-#define TIF_MEMDIE		20	/* is terminating due to OOM killer */
-#define TIF_POLLING_NRFLAG	21	/* idle is polling for TIF_NEED_RESCHED */
-#define TIF_IO_BITMAP		22	/* uses I/O bitmap */
-#define TIF_FORCED_TF		24	/* true if TF in eflags artificially */
-#define TIF_BLOCKSTEP		25	/* set when we want DEBUGCTLMSR_BTF */
-#define TIF_LAZY_MMU_UPDATES	27	/* task is updating the mmu lazily */
-#define TIF_SYSCALL_TRACEPOINT	28	/* syscall tracepoint instrumentation */
-#define TIF_ADDR32		29	/* 32-bit address space on 64 bits */
-#define TIF_X32			30	/* 32-bit native x86-64 binary */
-#define TIF_FSCHECK		31	/* Check FS is USER_DS on return */
+
+/* Operation mode */
+#define TIF_NOCPUID		0	/* CPUID is not accessible in userland */
+#define TIF_NOTSC		1	/* TSC is not accessible in userland */
+#define TIF_IA32		2	/* IA32 compatibility process */
+#define TIF_NOHZ		3	/* In adaptive nohz mode */
+#define TIF_ADDR32		4	/* 32-bit address space on 64 bits */
+#define TIF_X32			5	/* 32-bit native x86-64 binary */
+
+/* Syscall mode */
+#define TIF_SYSCALL_TRACE	6	/* Syscall trace active */
+#define TIF_SYSCALL_EMU		7	/* Syscall emulation active */
+#define TIF_SYSCALL_AUDIT	8	/* Syscall auditing active */
+#define TIF_SYSCALL_TRACEPOINT	9	/* Syscall tracepoint instrumentation */
+
+/* Pending work */
+#define TIF_NOTIFY_RESUME	10	/* Callback before returning to user */
+#define TIF_SIGPENDING		11	/* Signal pending */
+#define TIF_NEED_RESCHED	12	/* Rescheduling necessary */
+#define TIF_SINGLESTEP		13	/* Reenable singlestep on user return*/
+#define TIF_USER_RETURN_NOTIFY	14	/* Notify kernel of userspace return */
+#define TIF_PATCH_PENDING	15	/* Pending live patching update */
+#define TIF_FSCHECK		16	/* Check FS is USER_DS on return */
+
+/* Task status */
+#define TIF_UPROBE		17	/* Breakpointed or singlestepping */
+#define TIF_MEMDIE		18	/* Is terminating due to OOM killer */
+#define TIF_POLLING_NRFLAG	19	/* Idle is polling for TIF_NEED_RESCHED */
+#define TIF_IO_BITMAP		20	/* Uses I/O bitmap */
+#define TIF_FORCED_TF		21	/* True if TF in eflags artificially */
+#define TIF_BLOCKSTEP		22	/* Set when we want DEBUGCTLMSR_BTF */
+#define TIF_LAZY_MMU_UPDATES	23	/* Task is updating the mmu lazily */
+
+/* Security mode */
+#define TIF_SECCOMP		24	/* Secure computing */
+#define TIF_SSBD		25	/* Speculative store bypass disable */
+#define TIF_STIBP		26	/* Single thread indirect branch speculation */
+
+#define _TIF_NOCPUID		(1 << TIF_NOCPUID)
+#define _TIF_NOTSC		(1 << TIF_NOTSC)
+#define _TIF_IA32		(1 << TIF_IA32)
+#define _TIF_NOHZ		(1 << TIF_NOHZ)
+#define _TIF_ADDR32		(1 << TIF_ADDR32)
+#define _TIF_X32		(1 << TIF_X32)
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
+#define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
+#define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
+#define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
+
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_SINGLESTEP		(1 << TIF_SINGLESTEP)
-#define _TIF_SSBD		(1 << TIF_SSBD)
-#define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
-#define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
-#define _TIF_SECCOMP		(1 << TIF_SECCOMP)
-#define _TIF_STIBP		(1 << TIF_STIBP)
 #define _TIF_USER_RETURN_NOTIFY	(1 << TIF_USER_RETURN_NOTIFY)
-#define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_PATCH_PENDING	(1 << TIF_PATCH_PENDING)
-#define _TIF_NOCPUID		(1 << TIF_NOCPUID)
-#define _TIF_NOTSC		(1 << TIF_NOTSC)
-#define _TIF_IA32		(1 << TIF_IA32)
-#define _TIF_NOHZ		(1 << TIF_NOHZ)
+#define _TIF_FSCHECK		(1 << TIF_FSCHECK)
+
+#define _TIF_UPROBE		(1 << TIF_UPROBE)
+#define _TIF_MEMDIE		(1 << TIF_MEMDIE)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_IO_BITMAP		(1 << TIF_IO_BITMAP)
 #define _TIF_FORCED_TF		(1 << TIF_FORCED_TF)
 #define _TIF_BLOCKSTEP		(1 << TIF_BLOCKSTEP)
 #define _TIF_LAZY_MMU_UPDATES	(1 << TIF_LAZY_MMU_UPDATES)
-#define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
-#define _TIF_ADDR32		(1 << TIF_ADDR32)
-#define _TIF_X32		(1 << TIF_X32)
-#define _TIF_FSCHECK		(1 << TIF_FSCHECK)
+
+#define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_SSBD		(1 << TIF_SSBD)
+#define _TIF_STIBP		(1 << TIF_STIBP)
 
 /*
  * work to do in syscall_trace_enter().  Also includes TIF_NOHZ for
-- 
2.9.4

Re: [Patch v5 00/16] Provide task property based options to enable Spectre v2 userspace-userspace protection

[Jiri Kosina]

On Fri, 16 Nov 2018, Tim Chen wrote:

> Thomas, can you consider this series to be merged to 4.20-rc along
> with Jiri's changes on STIBP?

I plan go through the v5 of the series ASAP, but I tend to agree that if 
it passes the review it should be considered for 4.20 still, to get some 
of the lost performance back (I am pretty sure it will be taken for all 
-stable branches as well anyway).

Thanks,

-- 
Jiri Kosina
SUSE Labs