[SRU][Xenial][PATCH 0/3] Cleanups for CVE-2017-5715 (Spectre v2)

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH 0/3] Cleanups for CVE-2017-5715 (Spectre v2)

Juerg Haefliger
This patchset cleans up the Ubuntu-specific IBRS and IBPB runtime controls.
The runtime controls from the embargoed patches are messy and spread all
over the place. These patches consolidate the modifications into the
proper places (commandline options in arch/x86/kernel/cpu/bugs.c instead of
kernel/smp.c, speculation macros in arch/x86/include/asm/nospec-branch.h
instead of open-coded) and merge them with the additional spectre-related
changes that went in recently.

In addtion, the 2nd patch adds an entry to
/sys/devices/system/cpu/vulnerabilities/spectre_v2 when IBRS is enabled
via procfs to return the full set of enabled mitigations.

Compile-tested all architectures. Ran release tests to verify no
regression is introduced. Fiddled with the ibrs_enabled and ibpb_enabled
procfs controls to verify proper behaviour.

Signed-off-by: Juerg Haefliger <[hidden email]>


Juerg Haefliger (3):
  UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling
  UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling
  UBUNTU: SAUCE: x86/speculation: Move RSB_CTXSW hunk

 arch/x86/include/asm/mwait.h         |   6 +-
 arch/x86/include/asm/nospec-branch.h |  28 +++-
 arch/x86/include/asm/spec_ctrl.h     |  11 +-
 arch/x86/kernel/cpu/amd.c            |   5 +-
 arch/x86/kernel/cpu/bugs.c           |  87 +++++++-----
 arch/x86/kernel/cpu/microcode/core.c |  23 ---
 arch/x86/kernel/process.c            |  10 +-
 arch/x86/kernel/smpboot.c            |   6 +-
 arch/x86/kvm/svm.c                   |   6 +-
 arch/x86/kvm/vmx.c                   |   3 +-
 arch/x86/lib/delay.c                 |   8 +-
 arch/x86/mm/tlb.c                    |   2 +-
 include/linux/smp.h                  |  83 -----------
 kernel/smp.c                         |  46 ------
 kernel/sysctl.c                      | 201 ++++++++++++++++-----------
 15 files changed, 223 insertions(+), 302 deletions(-)

--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH 1/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling

Juerg Haefliger
In Ubuntu, we have runtime control for enabling/disabling IBPB via the
commandline ("noibpb") and through the proc interface
/proc/sys/kernel/ibpb_enabled. This commit simplifies the current
(broken) implementation by merging it with all the IBPB-related upstream
changes from previous commits.

What we have now is the upstream implementation for detecting the presence
of IBPB support which is used in the alternative MSR write in
indirect_branch_prediction_barrier() to enable IBPB. On top of that, this
commit adds a global state variable 'ibpb_enabled' which is set to 1 if
the CPU supports IBPB but can be overridden via the commandline "noibpb"
switch or by writting 0 or 1 to /proc/sys/kernel/ibpb_enabled at runtime.

If ibpb_enabled is 0, indirect_branch_prediction_barrier() is turned into a
no-op, same as if the platform doesn't support IBPB.

CVE-2017-5715

Signed-off-by: Juerg Haefliger <[hidden email]>
---
 arch/x86/include/asm/nospec-branch.h |  8 ++-
 arch/x86/include/asm/spec_ctrl.h     |  1 -
 arch/x86/kernel/cpu/amd.c            |  5 +-
 arch/x86/kernel/cpu/bugs.c           | 31 +++++++---
 arch/x86/kernel/cpu/microcode/core.c | 12 ----
 arch/x86/kvm/svm.c                   |  6 +-
 arch/x86/kvm/vmx.c                   |  3 +-
 arch/x86/mm/tlb.c                    |  2 +-
 include/linux/smp.h                  | 37 -----------
 kernel/smp.c                         | 18 ------
 kernel/sysctl.c                      | 92 +++++++++++++++++-----------
 11 files changed, 93 insertions(+), 122 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 8600916c073a..dcc7b0348fbc 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -184,6 +184,10 @@
 # define THUNK_TARGET(addr) [thunk_target] "rm" (addr)
 #endif
 
+/* The IBPB runtime control knob */
+extern unsigned int ibpb_enabled;
+int set_ibpb_enabled(unsigned int);
+
 /* The Spectre V2 mitigation variants */
 enum spectre_v2_mitigation {
  SPECTRE_V2_NONE,
@@ -243,7 +247,9 @@ static inline void indirect_branch_prediction_barrier(void)
 {
  u64 val = PRED_CMD_IBPB;
 
- alternative_msr_write(MSR_IA32_PRED_CMD, val, X86_FEATURE_USE_IBPB);
+ if (ibpb_enabled)
+ alternative_msr_write(MSR_IA32_PRED_CMD, val,
+      X86_FEATURE_USE_IBPB);
 }
 
 /*
diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
index c18bc77bb98e..49c3b0a83e9f 100644
--- a/arch/x86/include/asm/spec_ctrl.h
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -9,7 +9,6 @@
 #ifdef __ASSEMBLY__
 
 .extern use_ibrs
-.extern use_ibpb
 
 #define __ASM_ENABLE_IBRS \
  pushq %rax; \
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 5d01efd966a9..609b31deeb25 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -21,6 +21,9 @@
 
 #include "cpu.h"
 
+/* "noibpb" commandline parameter present (1) or not (0) */
+extern unsigned int noibpb;
+
 /*
  * nodes_per_socket: Stores the number of nodes per socket.
  * Refer to Fam15h Models 00-0fh BKDG - CPUID Fn8000_001E_ECX
@@ -842,7 +845,7 @@ static void init_amd(struct cpuinfo_x86 *c)
  * speculative control features, IBPB type support can be achieved by
  * disabling indirect branch predictor support.
  */
- if (!ibpb_disabled && !cpu_has(c, X86_FEATURE_SPEC_CTRL) &&
+ if (!noibpb && !cpu_has(c, X86_FEATURE_SPEC_CTRL) &&
     !cpu_has(c, X86_FEATURE_IBPB)) {
  u64 val;
 
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index a90514f3514c..41c57b5c870c 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -30,6 +30,16 @@
 #include <asm/intel-family.h>
 #include <asm/e820.h>
 
+unsigned int noibpb = 0;
+
+static int __init noibpb_handler(char *str)
+{
+ noibpb = 1;
+ return 0;
+}
+
+early_param("noibpb", noibpb_handler);
+
 static void __init spectre_v2_select_mitigation(void);
 static void __init ssb_select_mitigation(void);
 static void __init l1tf_select_mitigation(void);
@@ -390,14 +400,18 @@ specv2_set_mode:
  spectre_v2_enabled = mode;
  pr_info("%s\n", spectre_v2_strings[mode]);
 
- /* Initialize Indirect Branch Prediction Barrier if supported */
+ /*
+ * Initialize Indirect Branch Prediction Barrier if supported and not
+ * disabled on the commandline
+ */
  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");
-
- set_ibpb_supported();
- if (ibpb_inuse)
- sysctl_ibpb_enabled = 1;
+ if (noibpb) {
+ /* IBPB disabled via commandline */
+ set_ibpb_enabled(0);
+ } else {
+ set_ibpb_enabled(1);
+ }
  }
 
  /* Initialize Indirect Branch Restricted Speculation if supported */
@@ -409,8 +423,7 @@ specv2_set_mode:
  sysctl_ibrs_enabled = 1;
         }
 
- pr_info("Spectre v2 mitigation: Speculation control IBPB %s IBRS %s",
-        ibpb_supported ? "supported" : "not-supported",
+ pr_info("Spectre v2 mitigation: Speculation control IBRS %s",
         ibrs_supported ? "supported" : "not-supported");
 
  /*
@@ -863,7 +876,7 @@ 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\n", spectre_v2_strings[spectre_v2_enabled],
-       ibpb_inuse ? ", IBPB (Intel v4)" : "",
+       ibpb_enabled ? ", IBPB" : "",
        boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
        spectre_v2_module_string());
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 63e3db171945..84bd97f8eeab 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -439,18 +439,6 @@ static ssize_t reload_store(struct device *dev,
  if (!ret)
  perf_check_microcode();
 
- /* Initialize Indirect Branch Prediction Barrier if supported */
- if (boot_cpu_has(X86_FEATURE_IBPB)) {
- setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
- pr_info("Enabling Indirect Branch Prediction Barrier\n");
-
- mutex_lock(&spec_ctrl_mutex);
- set_ibpb_supported();
- if (ibpb_inuse)
- sysctl_ibpb_enabled = 1;
- mutex_unlock(&spec_ctrl_mutex);
- }
-
  /* Initialize Indirect Branch Restricted Speculation if supported */
  if (boot_cpu_has(X86_FEATURE_IBRS)) {
  pr_info("Enabling Indirect Branch Restricted Speculation\n");
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 189e197d9193..034e3116415b 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1230,8 +1230,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
  * The VMCB could be recycled, causing a false negative in svm_vcpu_load;
  * block speculative execution.
  */
- if (ibpb_inuse)
- wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
+ indirect_branch_prediction_barrier();
 }
 
 static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
@@ -1265,8 +1264,7 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
  if (sd->current_vmcb != svm->vmcb) {
  sd->current_vmcb = svm->vmcb;
- if (ibpb_inuse)
- wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
+ indirect_branch_prediction_barrier();
  }
 }
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index fccd9224e4b3..22012bcc4ef6 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2245,8 +2245,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
  if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
  per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
  vmcs_load(vmx->loaded_vmcs->vmcs);
- if (ibpb_inuse)
- native_wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
+ indirect_branch_prediction_barrier();
  }
 
  if (vmx->loaded_vmcs->cpu != cpu) {
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 4762bb43bffa..c229094ad12d 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -127,7 +127,7 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
  */
  if (tsk && tsk->mm &&
     tsk->mm->context.ctx_id != last_ctx_id &&
-    get_dumpable(tsk->mm) != SUID_DUMP_USER && ibpb_inuse)
+    get_dumpable(tsk->mm) != SUID_DUMP_USER)
  indirect_branch_prediction_barrier();
 
  /*
diff --git a/include/linux/smp.h b/include/linux/smp.h
index a3b959424808..98ac8ff2afab 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -94,43 +94,6 @@ static inline void clear_ibrs_disabled(void)
  use_ibrs |= IBxx_INUSE;
 }
 
-/* indicate usage of IBPB to control execution speculation */
-extern int use_ibpb;
-extern u32 sysctl_ibpb_enabled;
-
-static inline int __check_ibpb_inuse(void)
-{
- if (use_ibpb & IBxx_INUSE)
- return 1;
- else
- /* rmb to prevent wrong speculation for security */
- rmb();
- return 0;
-}
-
-#define ibpb_inuse (__check_ibpb_inuse())
-#define ibpb_supported (use_ibpb & IBxx_SUPPORTED)
-#define ibpb_disabled (use_ibpb & IBxx_DISABLED)
-
-static inline void set_ibpb_supported(void)
-{
- use_ibpb |= IBxx_SUPPORTED;
- if (!ibpb_disabled)
- use_ibpb |= IBxx_INUSE;
-}
-static inline void set_ibpb_disabled(void)
-{
- use_ibpb |= IBxx_DISABLED;
- if (ibpb_inuse)
- use_ibpb &= ~IBxx_INUSE;
-}
-static inline void clear_ibpb_disabled(void)
-{
- use_ibpb &= ~IBxx_DISABLED;
- if (ibpb_supported)
- use_ibpb |= IBxx_INUSE;
-}
-
 #endif /* CONFIG_X86 */
 
 #ifdef CONFIG_SMP
diff --git a/kernel/smp.c b/kernel/smp.c
index 4879de0b392e..139681ddf916 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -509,15 +509,6 @@ EXPORT_SYMBOL(setup_max_cpus);
 
 int use_ibrs;
 EXPORT_SYMBOL(use_ibrs);
-
-/*
- * use IBRS
- * bit 0 = indicate if ibpb is currently in use
- * bit 1 = indicate if system supports ibpb
- * bit 2 = indicate if admin disables ibpb
-*/
-int use_ibpb;
-EXPORT_SYMBOL(use_ibpb);
 #endif
 
 /* mutex to serialize IBRS & IBPB control changes */
@@ -556,15 +547,6 @@ static int __init noibrs(char *str)
 }
 
 early_param("noibrs", noibrs);
-
-static int __init noibpb(char *str)
-{
- set_ibpb_disabled();
-
- return 0;
-}
-
-early_param("noibpb", noibpb);
 #endif
 
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 25501febd7c6..4c9f00d16366 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -204,8 +204,52 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
 #ifdef CONFIG_X86
 int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
                  void __user *buffer, size_t *lenp, loff_t *ppos);
-int proc_dointvec_ibpb_ctrl(struct ctl_table *table, int write,
-                 void __user *buffer, size_t *lenp, loff_t *ppos);
+
+unsigned int ibpb_enabled = 0;
+EXPORT_SYMBOL(ibpb_enabled);   /* Required in some modules */
+
+static unsigned int __ibpb_enabled = 0;   /* procfs shadow variable */
+
+int set_ibpb_enabled(unsigned int val)
+{
+ int error = 0;
+
+ mutex_lock(&spec_ctrl_mutex);
+
+ /* Only enable IBPB if the CPU supports it */
+ if (boot_cpu_has(X86_FEATURE_IBPB)) {
+ ibpb_enabled = val;
+ pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
+ "Branch Prediction Barrier\n",
+ ibpb_enabled ? "Enabling" : "Disabling");
+ } else {
+ ibpb_enabled = 0;
+ if (val) {
+ /* IBPB is not supported but we try to turn it on */
+ error = -EINVAL;
+ }
+ }
+
+ /* Update the shadow variable */
+ __ibpb_enabled = ibpb_enabled;
+
+ mutex_unlock(&spec_ctrl_mutex);
+
+ return error;
+}
+
+static int ibpb_enabled_handler(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ int error;
+
+ error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+ if (error)
+ return error;
+
+ return set_ibpb_enabled(__ibpb_enabled);
+}
 #endif
 
 #ifdef CONFIG_MAGIC_SYSRQ
@@ -246,8 +290,6 @@ int sysctl_legacy_va_layout;
 
 u32 sysctl_ibrs_enabled = 0;
 EXPORT_SYMBOL(sysctl_ibrs_enabled);
-u32 sysctl_ibpb_enabled = 0;
-EXPORT_SYMBOL(sysctl_ibpb_enabled);
 
 /* The default sysctl tables: */
 
@@ -1245,13 +1287,13 @@ static struct ctl_table kern_table[] = {
  .extra2         = &two,
  },
  {
- .procname       = "ibpb_enabled",
- .data           = &sysctl_ibpb_enabled,
- .maxlen         = sizeof(unsigned int),
- .mode           = 0644,
- .proc_handler   = proc_dointvec_ibpb_ctrl,
- .extra1         = &zero,
- .extra2         = &one,
+ .procname = "ibpb_enabled",
+ .data = &__ibpb_enabled,
+ .maxlen = sizeof(unsigned int),
+ .mode = 0644,
+ .proc_handler   = ibpb_enabled_handler,
+ .extra1 = &zero,
+ .extra2 = &one,
  },
 #endif
  { }
@@ -2419,8 +2461,8 @@ int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
  unsigned int cpu;
 
  ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
- pr_debug("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
- pr_debug("before:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
+ pr_debug("sysctl_ibrs_enabled = %u\n", sysctl_ibrs_enabled);
+ pr_debug("before:use_ibrs = %d\n", use_ibrs);
  mutex_lock(&spec_ctrl_mutex);
  if (sysctl_ibrs_enabled == 0) {
  /* always set IBRS off */
@@ -2446,29 +2488,7 @@ int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
  sysctl_ibrs_enabled = 0;
  }
  mutex_unlock(&spec_ctrl_mutex);
- pr_debug("after:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
- return ret;
-}
-
-int proc_dointvec_ibpb_ctrl(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp, loff_t *ppos)
-{
- int ret;
-
- ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
- pr_debug("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
- pr_debug("before:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
- mutex_lock(&spec_ctrl_mutex);
- if (sysctl_ibpb_enabled == 0)
- set_ibpb_disabled();
- else if (sysctl_ibpb_enabled == 1) {
- clear_ibpb_disabled();
- if (!ibpb_inuse)
- /* platform don't support ibpb */
- sysctl_ibpb_enabled = 0;
- }
- mutex_unlock(&spec_ctrl_mutex);
- pr_debug("after:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
+ pr_debug("after:use_ibrs = %d\n", use_ibrs);
  return ret;
 }
 #endif
--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Juerg Haefliger
In reply to this post by Juerg Haefliger
In Ubuntu, we have runtime control for enabling/disabling IBRS via the
commandline ("noibrs") and through the proc interface
/proc/sys/kernel/ibrs_enabled. This commit simplifies the current
(probably broken) implementation by merging it with all the IBRS-related
upstream changes from previous commits.

What we have now is the upstream implementation for detecting the presence
of IBRS support. This commit adds a global state variable 'ibrs_enabled'
which is set to 1 if the CPU supports IBRS but can be overridden via the
commandline "noibrs" switch or by writting 0, 1 or 2 to
/proc/sys/kernel/ibrs_enabled at runtime.

CVE-2017-5715

Signed-off-by: Juerg Haefliger <[hidden email]>
---
 arch/x86/include/asm/mwait.h         |   6 +-
 arch/x86/include/asm/nospec-branch.h |  20 +++++
 arch/x86/include/asm/spec_ctrl.h     |  10 ++-
 arch/x86/kernel/cpu/bugs.c           |  60 +++++++-------
 arch/x86/kernel/cpu/microcode/core.c |  11 ---
 arch/x86/kernel/process.c            |  10 +--
 arch/x86/kernel/smpboot.c            |   6 +-
 arch/x86/lib/delay.c                 |   8 +-
 include/linux/smp.h                  |  46 -----------
 kernel/smp.c                         |  28 -------
 kernel/sysctl.c                      | 115 ++++++++++++++++-----------
 11 files changed, 135 insertions(+), 185 deletions(-)

diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
index 9821763b02cf..9bd760758640 100644
--- a/arch/x86/include/asm/mwait.h
+++ b/arch/x86/include/asm/mwait.h
@@ -106,15 +106,13 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
  mb();
  }
 
- if (ibrs_inuse)
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+ ubuntu_restrict_branch_speculation_end();
 
  __monitor((void *)&current_thread_info()->flags, 0, 0);
  if (!need_resched())
  __mwait(eax, ecx);
 
- if (ibrs_inuse)
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
+ ubuntu_restrict_branch_speculation_start();
  }
  current_clr_polling();
 }
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index dcc7b0348fbc..a6120d43caa7 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -188,6 +188,10 @@
 extern unsigned int ibpb_enabled;
 int set_ibpb_enabled(unsigned int);
 
+/* The IBRS runtime control knob */
+extern unsigned int ibrs_enabled;
+int set_ibrs_enabled(unsigned int);
+
 /* The Spectre V2 mitigation variants */
 enum spectre_v2_mitigation {
  SPECTRE_V2_NONE,
@@ -275,6 +279,22 @@ do { \
  preempt_enable(); \
 } while (0)
 
+#define ubuntu_restrict_branch_speculation_start() \
+do { \
+ u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
+ \
+ if (ibrs_enabled) \
+ native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
+} while (0)
+
+#define ubuntu_restrict_branch_speculation_end() \
+do { \
+ u64 val = x86_spec_ctrl_base; \
+ \
+ if (ibrs_enabled) \
+ native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
+ } while (0)
+
 #endif /* __ASSEMBLY__ */
 
 /*
diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
index 49c3b0a83e9f..a5d93d23390e 100644
--- a/arch/x86/include/asm/spec_ctrl.h
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -8,7 +8,7 @@
 
 #ifdef __ASSEMBLY__
 
-.extern use_ibrs
+.extern ibrs_enabled
 
 #define __ASM_ENABLE_IBRS \
  pushq %rax; \
@@ -21,11 +21,13 @@
  popq %rdx; \
  popq %rcx; \
  popq %rax
+
 #define __ASM_ENABLE_IBRS_CLOBBER \
  movl $MSR_IA32_SPEC_CTRL, %ecx; \
  movl $0, %edx; \
  movl $SPEC_CTRL_IBRS, %eax; \
  wrmsr;
+
 #define __ASM_DISABLE_IBRS \
  pushq %rax; \
  pushq %rcx; \
@@ -39,7 +41,7 @@
  popq %rax
 
 .macro ENABLE_IBRS
- testl $1, use_ibrs
+ testl $1, ibrs_enabled
  jz 10f
  __ASM_ENABLE_IBRS
  jmp 20f
@@ -49,7 +51,7 @@
 .endm
 
 .macro ENABLE_IBRS_CLOBBER
- testl $1, use_ibrs
+ testl $1, ibrs_enabled
  jz 11f
  __ASM_ENABLE_IBRS_CLOBBER
  jmp 21f
@@ -59,7 +61,7 @@
 .endm
 
 .macro DISABLE_IBRS
- testl $1, use_ibrs
+ testl $1, ibrs_enabled
  jz 9f
  __ASM_DISABLE_IBRS
 9:
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 41c57b5c870c..a4565038ab35 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -40,6 +40,16 @@ static int __init noibpb_handler(char *str)
 
 early_param("noibpb", noibpb_handler);
 
+unsigned int noibrs = 0;
+
+static int __init noibrs_handler(char *str)
+{
+ noibrs = 1;
+ return 0;
+}
+
+early_param("noibrs", noibrs_handler);
+
 static void __init spectre_v2_select_mitigation(void);
 static void __init ssb_select_mitigation(void);
 static void __init l1tf_select_mitigation(void);
@@ -414,18 +424,6 @@ specv2_set_mode:
  }
  }
 
- /* Initialize Indirect Branch Restricted Speculation if supported */
- if (boot_cpu_has(X86_FEATURE_IBRS)) {
- pr_info("Spectre v2 mitigation: Enabling Indirect Branch Restricted Speculation\n");
-
- set_ibrs_supported();
- if (ibrs_inuse)
- sysctl_ibrs_enabled = 1;
-        }
-
- pr_info("Spectre v2 mitigation: Speculation control IBRS %s",
-        ibrs_supported ? "supported" : "not-supported");
-
  /*
  * If spectre v2 protection has been enabled, unconditionally fill
  * RSB during a context switch; this protects against two independent
@@ -437,21 +435,6 @@ specv2_set_mode:
  setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
  pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
 
- /*
- * If we have a full retpoline mode and then disable IBPB in kernel mode
- * we do not require both.
- */
- if (mode == SPECTRE_V2_RETPOLINE_AMD ||
-    mode == SPECTRE_V2_RETPOLINE_GENERIC)
- {
- if (ibrs_supported) {
- pr_info("Retpoline compiled kernel.  Defaulting IBRS to disabled");
- set_ibrs_disabled();
- if (!ibrs_inuse)
- sysctl_ibrs_enabled = 0;
- }
- }
-
  /*
  * Retpoline means the kernel is safe because it has no indirect
  * branches. Enhanced IBRS protects firmware too, so, enable restricted
@@ -463,9 +446,23 @@ specv2_set_mode:
  * the CPU supports Enhanced IBRS, kernel might un-intentionally not
  * enable IBRS around firmware calls.
  */
- if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
- setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
- pr_info("Enabling Restricted Speculation for firmware calls\n");
+ if (boot_cpu_has(X86_FEATURE_IBRS)) {
+ if (mode != SPECTRE_V2_IBRS_ENHANCED) {
+ setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
+ pr_info("Enabling Restricted Speculation for firmware calls\n");
+ }
+ if (noibrs ||
+    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
+    mode == SPECTRE_V2_RETPOLINE_AMD ||
+    mode == SPECTRE_V2_IBRS_ENHANCED) {
+ /*
+ * IBRS disabled via commandline or the kernel is
+ * retpoline compiled or the HW supports enhanced IBRS
+ */
+ set_ibrs_enabled(0);
+ } else {
+ set_ibrs_enabled(1);
+ }
  }
 }
 
@@ -875,8 +872,9 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
  break;
 
  case X86_BUG_SPECTRE_V2:
- return sprintf(buf, "%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
+ return sprintf(buf, "%s%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
        ibpb_enabled ? ", IBPB" : "",
+       ibrs_enabled == 2 ? ", IBRS (user space)" : ibrs_enabled ? ", IBRS" : "",
        boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
        spectre_v2_module_string());
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 84bd97f8eeab..4fe475c2f745 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -439,17 +439,6 @@ static ssize_t reload_store(struct device *dev,
  if (!ret)
  perf_check_microcode();
 
- /* Initialize Indirect Branch Restricted Speculation if supported */
- if (boot_cpu_has(X86_FEATURE_IBRS)) {
- pr_info("Enabling Indirect Branch Restricted Speculation\n");
-
- mutex_lock(&spec_ctrl_mutex);
- set_ibrs_supported();
- if (ibrs_inuse)
- sysctl_ibrs_enabled = 1;
- mutex_unlock(&spec_ctrl_mutex);
- }
-
  mutex_unlock(&microcode_mutex);
  put_online_cpus();
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index a5b3404266eb..5587098a5823 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -566,17 +566,15 @@ static void mwait_idle(void)
  smp_mb(); /* quirk */
  }
 
- if (ibrs_inuse)
-                        native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+ ubuntu_restrict_branch_speculation_end();
 
  __monitor((void *)&current_thread_info()->flags, 0, 0);
+
  if (!need_resched()) {
  __sti_mwait(0, 0);
- if (ibrs_inuse)
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
+ ubuntu_restrict_branch_speculation_start();
  } else {
- if (ibrs_inuse)
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
+ ubuntu_restrict_branch_speculation_start();
  local_irq_enable();
  }
  trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 21c3bcbd69a2..6c9bb4db2ed7 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1697,15 +1697,13 @@ void native_play_dead(void)
  play_dead_common();
  tboot_shutdown(TB_SHUTDOWN_WFS);
 
- if (ibrs_inuse)
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+ ubuntu_restrict_branch_speculation_end();
 
  mwait_play_dead(); /* Only returns on failure */
  if (cpuidle_play_dead())
  hlt_play_dead();
 
- if (ibrs_inuse)
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
+ ubuntu_restrict_branch_speculation_start();
 }
 
 #else /* ... !CONFIG_HOTPLUG_CPU */
diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
index 5b66d4417f8d..20e9a5ac91dc 100644
--- a/arch/x86/lib/delay.c
+++ b/arch/x86/lib/delay.c
@@ -107,8 +107,8 @@ static void delay_mwaitx(unsigned long __loops)
  for (;;) {
  delay = min_t(u64, MWAITX_MAX_LOOPS, loops);
 
- if (ibrs_inuse && (delay > IBRS_DISABLE_THRESHOLD))
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+ if (delay > IBRS_DISABLE_THRESHOLD)
+ ubuntu_restrict_branch_speculation_end();
 
  /*
  * Use cpu_tss as a cacheline-aligned, seldomly
@@ -123,8 +123,8 @@ static void delay_mwaitx(unsigned long __loops)
  */
  __mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE);
 
- if (ibrs_inuse && (delay > IBRS_DISABLE_THRESHOLD))
- native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
+ if (delay > IBRS_DISABLE_THRESHOLD)
+ ubuntu_restrict_branch_speculation_start();
 
  end = rdtsc_ordered();
 
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 98ac8ff2afab..c4414074bd88 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -50,52 +50,6 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info),
 
 int smp_call_function_single_async(int cpu, struct call_single_data *csd);
 
-#ifdef CONFIG_X86
-
-#define IBxx_INUSE (1 << 0)
-#define IBxx_SUPPORTED (1 << 1)
-#define IBxx_DISABLED (1 << 2)
-
-/* indicate usage of IBRS to control execution speculation */
-extern int use_ibrs;
-extern u32 sysctl_ibrs_enabled;
-extern struct mutex spec_ctrl_mutex;
-
-static inline int __check_ibrs_inuse(void)
-{
- if (use_ibrs & IBxx_INUSE)
- return 1;
- else
- /* rmb to prevent wrong speculation for security */
- rmb();
- return 0;
-}
-
-#define ibrs_inuse (__check_ibrs_inuse())
-#define ibrs_supported (use_ibrs & IBxx_SUPPORTED)
-#define ibrs_disabled (use_ibrs & IBxx_DISABLED)
-
-static inline void set_ibrs_supported(void)
-{
- use_ibrs |= IBxx_SUPPORTED;
- if (!ibrs_disabled)
- use_ibrs |= IBxx_INUSE;
-}
-static inline void set_ibrs_disabled(void)
-{
- use_ibrs |= IBxx_DISABLED;
- if (ibrs_inuse)
- use_ibrs &= ~IBxx_INUSE;
-}
-static inline void clear_ibrs_disabled(void)
-{
- use_ibrs &= ~IBxx_DISABLED;
- if (ibrs_supported)
- use_ibrs |= IBxx_INUSE;
-}
-
-#endif /* CONFIG_X86 */
-
 #ifdef CONFIG_SMP
 
 #include <linux/preempt.h>
diff --git a/kernel/smp.c b/kernel/smp.c
index 139681ddf916..48eb4fafc356 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -499,22 +499,6 @@ EXPORT_SYMBOL(smp_call_function);
 unsigned int setup_max_cpus = NR_CPUS;
 EXPORT_SYMBOL(setup_max_cpus);
 
-#ifdef CONFIG_X86
-/*
- * use IBRS
- * bit 0 = indicate if ibrs is currently in use
- * bit 1 = indicate if system supports ibrs
- * bit 2 = indicate if admin disables ibrs
-*/
-
-int use_ibrs;
-EXPORT_SYMBOL(use_ibrs);
-#endif
-
-/* mutex to serialize IBRS & IBPB control changes */
-DEFINE_MUTEX(spec_ctrl_mutex);
-EXPORT_SYMBOL(spec_ctrl_mutex);
-
 /*
  * Setup routine for controlling SMP activation
  *
@@ -538,18 +522,6 @@ static int __init nosmp(char *str)
 
 early_param("nosmp", nosmp);
 
-#ifdef CONFIG_X86
-static int __init noibrs(char *str)
-{
- set_ibrs_disabled();
-
- return 0;
-}
-
-early_param("noibrs", noibrs);
-#endif
-
-
 /* this is hard limit */
 static int __init nrcpus(char *str)
 {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4c9f00d16366..11c626dd1b1c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -202,8 +202,8 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
 #endif
 
 #ifdef CONFIG_X86
-int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
-                 void __user *buffer, size_t *lenp, loff_t *ppos);
+/* Mutex to serialize IBPB and IBRS runtime control changes */
+DEFINE_MUTEX(spec_ctrl_mutex);
 
 unsigned int ibpb_enabled = 0;
 EXPORT_SYMBOL(ibpb_enabled);   /* Required in some modules */
@@ -250,6 +250,70 @@ static int ibpb_enabled_handler(struct ctl_table *table, int write,
 
  return set_ibpb_enabled(__ibpb_enabled);
 }
+
+unsigned int ibrs_enabled = 0;
+EXPORT_SYMBOL(ibrs_enabled);   /* Required in some modules */
+
+static unsigned int __ibrs_enabled = 0;   /* procfs shadow variable */
+
+int set_ibrs_enabled(unsigned int val)
+{
+ int error = 0;
+ unsigned int cpu;
+
+ mutex_lock(&spec_ctrl_mutex);
+
+ /* Only enable/disable IBRS if the CPU supports it */
+ if (boot_cpu_has(X86_FEATURE_IBRS)) {
+ ibrs_enabled = val;
+ pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
+ "Branch Restricted Speculation%s\n",
+ ibrs_enabled ? "Enabling" : "Disabling",
+ ibrs_enabled == 2 ? " (user space)" : "");
+
+ if (ibrs_enabled == 0) {
+ /* Always disable IBRS */
+ u64 val = x86_spec_ctrl_base;
+
+ for_each_online_cpu(cpu) {
+ wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
+ }
+ } else if (ibrs_enabled == 2) {
+ /* Always enable IBRS, even in user space */
+ u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
+
+ for_each_online_cpu(cpu) {
+ wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
+ }
+ }
+ } else {
+ ibrs_enabled = 0;
+ if (val) {
+ /* IBRS is not supported but we try to turn it on */
+ error = -EINVAL;
+ }
+ }
+
+ /* Update the shadow variable */
+ __ibrs_enabled = ibrs_enabled;
+
+ mutex_unlock(&spec_ctrl_mutex);
+
+ return error;
+}
+
+static int ibrs_enabled_handler(struct ctl_table *table, int write,
+                               void __user *buffer, size_t *lenp,
+                               loff_t *ppos)
+{
+ int error;
+
+ error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+ if (error)
+ return error;
+
+ return set_ibrs_enabled(__ibrs_enabled);
+}
 #endif
 
 #ifdef CONFIG_MAGIC_SYSRQ
@@ -288,9 +352,6 @@ extern struct ctl_table epoll_table[];
 int sysctl_legacy_va_layout;
 #endif
 
-u32 sysctl_ibrs_enabled = 0;
-EXPORT_SYMBOL(sysctl_ibrs_enabled);
-
 /* The default sysctl tables: */
 
 static struct ctl_table sysctl_base_table[] = {
@@ -1279,10 +1340,10 @@ static struct ctl_table kern_table[] = {
 #ifdef CONFIG_X86
  {
  .procname       = "ibrs_enabled",
- .data           = &sysctl_ibrs_enabled,
+ .data           = &__ibrs_enabled,
  .maxlen         = sizeof(unsigned int),
  .mode           = 0644,
- .proc_handler   = proc_dointvec_ibrs_ctrl,
+ .proc_handler   = ibrs_enabled_handler,
  .extra1         = &zero,
  .extra2         = &two,
  },
@@ -2453,46 +2514,6 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
  do_proc_dointvec_minmax_conv, &param);
 }
 
-#ifdef CONFIG_X86
-int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp, loff_t *ppos)
-{
- int ret;
- unsigned int cpu;
-
- ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
- pr_debug("sysctl_ibrs_enabled = %u\n", sysctl_ibrs_enabled);
- pr_debug("before:use_ibrs = %d\n", use_ibrs);
- mutex_lock(&spec_ctrl_mutex);
- if (sysctl_ibrs_enabled == 0) {
- /* always set IBRS off */
- set_ibrs_disabled();
- if (ibrs_supported) {
- for_each_online_cpu(cpu)
- wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
- }
- } else if (sysctl_ibrs_enabled == 2) {
- /* always set IBRS on, even in user space */
- clear_ibrs_disabled();
- if (ibrs_supported) {
- for_each_online_cpu(cpu)
- wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
- } else {
- sysctl_ibrs_enabled = 0;
- }
- } else if (sysctl_ibrs_enabled == 1) {
- /* use IBRS in kernel */
- clear_ibrs_disabled();
- if (!ibrs_inuse)
- /* platform don't support ibrs */
- sysctl_ibrs_enabled = 0;
- }
- mutex_unlock(&spec_ctrl_mutex);
- pr_debug("after:use_ibrs = %d\n", use_ibrs);
- return ret;
-}
-#endif
-
 static void validate_coredump_safety(void)
 {
 #ifdef CONFIG_COREDUMP
--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH 3/3] UBUNTU: SAUCE: x86/speculation: Move RSB_CTXSW hunk

Juerg Haefliger
In reply to this post by Juerg Haefliger
Move the RSB_CTXSW hunk further up in spectre_v2_select_mitigation() to
match upstream. No functional changes.

CVE-2017-5715

Signed-off-by: Juerg Haefliger <[hidden email]>
---
 arch/x86/kernel/cpu/bugs.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index a4565038ab35..60907abf12f5 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -410,6 +410,17 @@ specv2_set_mode:
  spectre_v2_enabled = mode;
  pr_info("%s\n", spectre_v2_strings[mode]);
 
+ /*
+ * If spectre v2 protection has been enabled, unconditionally fill
+ * RSB during a context switch; this protects against two independent
+ * issues:
+ *
+ * - RSB underflow (and switch to BTB) on Skylake+
+ * - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
+ */
+ 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 and not
  * disabled on the commandline
@@ -424,17 +435,6 @@ specv2_set_mode:
  }
  }
 
- /*
- * If spectre v2 protection has been enabled, unconditionally fill
- * RSB during a context switch; this protects against two independent
- * issues:
- *
- * - RSB underflow (and switch to BTB) on Skylake+
- * - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
- */
- setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
- pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
-
  /*
  * Retpoline means the kernel is safe because it has no indirect
  * branches. Enhanced IBRS protects firmware too, so, enable restricted
--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][PATCH] UBUNTU: SAUCE: x86/speculation: Only report IBPB/IBRS state changes

Juerg Haefliger
In reply to this post by Juerg Haefliger
Only print the IBPB/IBRS state to the log if it actually changes. Otherwise
the log is polluted everytime the procfs file is read from.

Signed-off-by: Juerg Haefliger <[hidden email]>
---
 kernel/sysctl.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 11c626dd1b1c..59fe90f934fb 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -213,15 +213,17 @@ static unsigned int __ibpb_enabled = 0;   /* procfs shadow variable */
 int set_ibpb_enabled(unsigned int val)
 {
  int error = 0;
+ unsigned int prev = ibpb_enabled;
 
  mutex_lock(&spec_ctrl_mutex);
 
  /* Only enable IBPB if the CPU supports it */
  if (boot_cpu_has(X86_FEATURE_IBPB)) {
  ibpb_enabled = val;
- pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
- "Branch Prediction Barrier\n",
- ibpb_enabled ? "Enabling" : "Disabling");
+ if (ibpb_enabled != prev)
+ pr_info("Spectre V2 : Spectre v2 mitigation: %s "
+ "Indirect Branch Prediction Barrier\n",
+ ibpb_enabled ? "Enabling" : "Disabling");
  } else {
  ibpb_enabled = 0;
  if (val) {
@@ -260,16 +262,18 @@ int set_ibrs_enabled(unsigned int val)
 {
  int error = 0;
  unsigned int cpu;
+ unsigned int prev = ibrs_enabled;
 
  mutex_lock(&spec_ctrl_mutex);
 
  /* Only enable/disable IBRS if the CPU supports it */
  if (boot_cpu_has(X86_FEATURE_IBRS)) {
  ibrs_enabled = val;
- pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
- "Branch Restricted Speculation%s\n",
- ibrs_enabled ? "Enabling" : "Disabling",
- ibrs_enabled == 2 ? " (user space)" : "");
+ if (ibrs_enabled != prev)
+ pr_info("Spectre V2 : Spectre v2 mitigation: %s "
+ "Indirect Branch Restricted Speculation%s\n",
+ ibrs_enabled ? "Enabling" : "Disabling",
+ ibrs_enabled == 2 ? " (user space)" : "");
 
  if (ibrs_enabled == 0) {
  /* Always disable IBRS */
--
2.19.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU][Xenial][PATCH] UBUNTU: SAUCE: x86/speculation: Only report IBPB/IBRS state changes

Juerg Haefliger
This should be applied after the series. Sorry, it's missing the CVE
line :-(

...Juerg

On Tue, 27 Nov 2018 10:27:42 +0100
Juerg Haefliger <[hidden email]> wrote:

> Only print the IBPB/IBRS state to the log if it actually changes.
> Otherwise the log is polluted everytime the procfs file is read from.

CVE-2017-5715


> Signed-off-by: Juerg Haefliger <[hidden email]>
> ---
>  kernel/sysctl.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 11c626dd1b1c..59fe90f934fb 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -213,15 +213,17 @@ static unsigned int __ibpb_enabled = 0;   /*
> procfs shadow variable */ int set_ibpb_enabled(unsigned int val)
>  {
>   int error = 0;
> + unsigned int prev = ibpb_enabled;
>  
>   mutex_lock(&spec_ctrl_mutex);
>  
>   /* Only enable IBPB if the CPU supports it */
>   if (boot_cpu_has(X86_FEATURE_IBPB)) {
>   ibpb_enabled = val;
> - pr_info("Spectre V2 : Spectre v2 mitigation: %s
> Indirect "
> - "Branch Prediction Barrier\n",
> - ibpb_enabled ? "Enabling" : "Disabling");
> + if (ibpb_enabled != prev)
> + pr_info("Spectre V2 : Spectre v2 mitigation:
> %s "
> + "Indirect Branch Prediction
> Barrier\n",
> + ibpb_enabled ? "Enabling" :
> "Disabling"); } else {
>   ibpb_enabled = 0;
>   if (val) {
> @@ -260,16 +262,18 @@ int set_ibrs_enabled(unsigned int val)
>  {
>   int error = 0;
>   unsigned int cpu;
> + unsigned int prev = ibrs_enabled;
>  
>   mutex_lock(&spec_ctrl_mutex);
>  
>   /* Only enable/disable IBRS if the CPU supports it */
>   if (boot_cpu_has(X86_FEATURE_IBRS)) {
>   ibrs_enabled = val;
> - pr_info("Spectre V2 : Spectre v2 mitigation: %s
> Indirect "
> - "Branch Restricted Speculation%s\n",
> - ibrs_enabled ? "Enabling" : "Disabling",
> - ibrs_enabled == 2 ? " (user space)" : "");
> + if (ibrs_enabled != prev)
> + pr_info("Spectre V2 : Spectre v2 mitigation:
> %s "
> + "Indirect Branch Restricted
> Speculation%s\n",
> + ibrs_enabled ? "Enabling" :
> "Disabling",
> + ibrs_enabled == 2 ? " (user
> space)" : "");
>   if (ibrs_enabled == 0) {
>   /* Always disable IBRS */

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK/cmnt: [SRU][Xenial][PATCH 0/3] Cleanups for CVE-2017-5715 (Spectre v2)

Kleber Souza
In reply to this post by Juerg Haefliger
On 11/21/18 2:58 PM, Juerg Haefliger wrote:

> This patchset cleans up the Ubuntu-specific IBRS and IBPB runtime controls.
> The runtime controls from the embargoed patches are messy and spread all
> over the place. These patches consolidate the modifications into the
> proper places (commandline options in arch/x86/kernel/cpu/bugs.c instead of
> kernel/smp.c, speculation macros in arch/x86/include/asm/nospec-branch.h
> instead of open-coded) and merge them with the additional spectre-related
> changes that went in recently.
>
> In addtion, the 2nd patch adds an entry to
> /sys/devices/system/cpu/vulnerabilities/spectre_v2 when IBRS is enabled
> via procfs to return the full set of enabled mitigations.
>
> Compile-tested all architectures. Ran release tests to verify no
> regression is introduced. Fiddled with the ibrs_enabled and ibpb_enabled
> procfs controls to verify proper behaviour.
>
> Signed-off-by: Juerg Haefliger <[hidden email]>
>
>
> Juerg Haefliger (3):
>   UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling
>   UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling
>   UBUNTU: SAUCE: x86/speculation: Move RSB_CTXSW hunk
>
>  arch/x86/include/asm/mwait.h         |   6 +-
>  arch/x86/include/asm/nospec-branch.h |  28 +++-
>  arch/x86/include/asm/spec_ctrl.h     |  11 +-
>  arch/x86/kernel/cpu/amd.c            |   5 +-
>  arch/x86/kernel/cpu/bugs.c           |  87 +++++++-----
>  arch/x86/kernel/cpu/microcode/core.c |  23 ---
>  arch/x86/kernel/process.c            |  10 +-
>  arch/x86/kernel/smpboot.c            |   6 +-
>  arch/x86/kvm/svm.c                   |   6 +-
>  arch/x86/kvm/vmx.c                   |   3 +-
>  arch/x86/lib/delay.c                 |   8 +-
>  arch/x86/mm/tlb.c                    |   2 +-
>  include/linux/smp.h                  |  83 -----------
>  kernel/smp.c                         |  46 ------
>  kernel/sysctl.c                      | 201 ++++++++++++++++-----------
>  15 files changed, 223 insertions(+), 302 deletions(-)
>
Giving the missing CVE reference is fixed on the last patch:

Acked-by: Kleber Sacilotto de Souza <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Acked] [SRU][Xenial][PATCH 0/3] Cleanups for CVE-2017-5715 (Spectre v2)

Andy Whitcroft-3
In reply to this post by Juerg Haefliger
On Wed, Nov 21, 2018 at 02:58:28PM +0100, Juerg Haefliger wrote:

> This patchset cleans up the Ubuntu-specific IBRS and IBPB runtime controls.
> The runtime controls from the embargoed patches are messy and spread all
> over the place. These patches consolidate the modifications into the
> proper places (commandline options in arch/x86/kernel/cpu/bugs.c instead of
> kernel/smp.c, speculation macros in arch/x86/include/asm/nospec-branch.h
> instead of open-coded) and merge them with the additional spectre-related
> changes that went in recently.
>
> In addtion, the 2nd patch adds an entry to
> /sys/devices/system/cpu/vulnerabilities/spectre_v2 when IBRS is enabled
> via procfs to return the full set of enabled mitigations.
>
> Compile-tested all architectures. Ran release tests to verify no
> regression is introduced. Fiddled with the ibrs_enabled and ibpb_enabled
> procfs controls to verify proper behaviour.
>
> Signed-off-by: Juerg Haefliger <[hidden email]>
>
>
> Juerg Haefliger (3):
>   UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling
>   UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling
>   UBUNTU: SAUCE: x86/speculation: Move RSB_CTXSW hunk
>
>  arch/x86/include/asm/mwait.h         |   6 +-
>  arch/x86/include/asm/nospec-branch.h |  28 +++-
>  arch/x86/include/asm/spec_ctrl.h     |  11 +-
>  arch/x86/kernel/cpu/amd.c            |   5 +-
>  arch/x86/kernel/cpu/bugs.c           |  87 +++++++-----
>  arch/x86/kernel/cpu/microcode/core.c |  23 ---
>  arch/x86/kernel/process.c            |  10 +-
>  arch/x86/kernel/smpboot.c            |   6 +-
>  arch/x86/kvm/svm.c                   |   6 +-
>  arch/x86/kvm/vmx.c                   |   3 +-
>  arch/x86/lib/delay.c                 |   8 +-
>  arch/x86/mm/tlb.c                    |   2 +-
>  include/linux/smp.h                  |  83 -----------
>  kernel/smp.c                         |  46 ------
>  kernel/sysctl.c                      | 201 ++++++++++++++++-----------
>  15 files changed, 223 insertions(+), 302 deletions(-)
>
> --

I assume we have some testing we can do on top to confirm these work on
the final kernels.  They look ok on face value and the desire is good.

Acked-by: Andy Whitcroft <[hidden email]>

-apw

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Tyler Hicks-2
In reply to this post by Juerg Haefliger
On 2018-11-21 14:58:30, Juerg Haefliger wrote:

> In Ubuntu, we have runtime control for enabling/disabling IBRS via the
> commandline ("noibrs") and through the proc interface
> /proc/sys/kernel/ibrs_enabled. This commit simplifies the current
> (probably broken) implementation by merging it with all the IBRS-related
> upstream changes from previous commits.
>
> What we have now is the upstream implementation for detecting the presence
> of IBRS support. This commit adds a global state variable 'ibrs_enabled'
> which is set to 1 if the CPU supports IBRS but can be overridden via the
> commandline "noibrs" switch or by writting 0, 1 or 2 to
> /proc/sys/kernel/ibrs_enabled at runtime.
>
> CVE-2017-5715
>
> Signed-off-by: Juerg Haefliger <[hidden email]>
Acked-by: Tyler Hicks <[hidden email]>

A few comments below...

> ---
>  arch/x86/include/asm/mwait.h         |   6 +-
>  arch/x86/include/asm/nospec-branch.h |  20 +++++
>  arch/x86/include/asm/spec_ctrl.h     |  10 ++-
>  arch/x86/kernel/cpu/bugs.c           |  60 +++++++-------
>  arch/x86/kernel/cpu/microcode/core.c |  11 ---
>  arch/x86/kernel/process.c            |  10 +--
>  arch/x86/kernel/smpboot.c            |   6 +-
>  arch/x86/lib/delay.c                 |   8 +-
>  include/linux/smp.h                  |  46 -----------
>  kernel/smp.c                         |  28 -------
>  kernel/sysctl.c                      | 115 ++++++++++++++++-----------
>  11 files changed, 135 insertions(+), 185 deletions(-)
>
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index 9821763b02cf..9bd760758640 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -106,15 +106,13 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
>   mb();
>   }
>  
> - if (ibrs_inuse)
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> + ubuntu_restrict_branch_speculation_end();
>  
>   __monitor((void *)&current_thread_info()->flags, 0, 0);
>   if (!need_resched())
>   __mwait(eax, ecx);
>  
> - if (ibrs_inuse)
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
> + ubuntu_restrict_branch_speculation_start();
>   }
>   current_clr_polling();
>  }
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index dcc7b0348fbc..a6120d43caa7 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -188,6 +188,10 @@
>  extern unsigned int ibpb_enabled;
>  int set_ibpb_enabled(unsigned int);
>  
> +/* The IBRS runtime control knob */
> +extern unsigned int ibrs_enabled;
> +int set_ibrs_enabled(unsigned int);
> +
>  /* The Spectre V2 mitigation variants */
>  enum spectre_v2_mitigation {
>   SPECTRE_V2_NONE,
> @@ -275,6 +279,22 @@ do { \
>   preempt_enable(); \
>  } while (0)
>  
> +#define ubuntu_restrict_branch_speculation_start() \
> +do { \
> + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
> + \
> + if (ibrs_enabled) \
> + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> +} while (0)
> +
> +#define ubuntu_restrict_branch_speculation_end() \
> +do { \
> + u64 val = x86_spec_ctrl_base; \
> + \
> + if (ibrs_enabled) \
> + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> + } while (0)
> +
>  #endif /* __ASSEMBLY__ */
>  
>  /*
> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
> index 49c3b0a83e9f..a5d93d23390e 100644
> --- a/arch/x86/include/asm/spec_ctrl.h
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -8,7 +8,7 @@
>  
>  #ifdef __ASSEMBLY__
>  
> -.extern use_ibrs
> +.extern ibrs_enabled
>  
>  #define __ASM_ENABLE_IBRS \
>   pushq %rax; \
> @@ -21,11 +21,13 @@
>   popq %rdx; \
>   popq %rcx; \
>   popq %rax
> +
>  #define __ASM_ENABLE_IBRS_CLOBBER \
>   movl $MSR_IA32_SPEC_CTRL, %ecx; \
>   movl $0, %edx; \
>   movl $SPEC_CTRL_IBRS, %eax; \
>   wrmsr;
> +
>  #define __ASM_DISABLE_IBRS \
>   pushq %rax; \
>   pushq %rcx; \
> @@ -39,7 +41,7 @@
>   popq %rax
>  
>  .macro ENABLE_IBRS
> - testl $1, use_ibrs
> + testl $1, ibrs_enabled
>   jz 10f
>   __ASM_ENABLE_IBRS
>   jmp 20f
> @@ -49,7 +51,7 @@
>  .endm
>  
>  .macro ENABLE_IBRS_CLOBBER
> - testl $1, use_ibrs
> + testl $1, ibrs_enabled
>   jz 11f
>   __ASM_ENABLE_IBRS_CLOBBER
>   jmp 21f
> @@ -59,7 +61,7 @@
>  .endm
>  
>  .macro DISABLE_IBRS
> - testl $1, use_ibrs
> + testl $1, ibrs_enabled
>   jz 9f
>   __ASM_DISABLE_IBRS
>  9:
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 41c57b5c870c..a4565038ab35 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -40,6 +40,16 @@ static int __init noibpb_handler(char *str)
>  
>  early_param("noibpb", noibpb_handler);
>  
> +unsigned int noibrs = 0;
> +
> +static int __init noibrs_handler(char *str)
> +{
> + noibrs = 1;
> + return 0;
> +}
> +
> +early_param("noibrs", noibrs_handler);
> +
>  static void __init spectre_v2_select_mitigation(void);
>  static void __init ssb_select_mitigation(void);
>  static void __init l1tf_select_mitigation(void);
> @@ -414,18 +424,6 @@ specv2_set_mode:
>   }
>   }
>  
> - /* Initialize Indirect Branch Restricted Speculation if supported */
> - if (boot_cpu_has(X86_FEATURE_IBRS)) {
> - pr_info("Spectre v2 mitigation: Enabling Indirect Branch Restricted Speculation\n");
> -
> - set_ibrs_supported();
> - if (ibrs_inuse)
> - sysctl_ibrs_enabled = 1;
> -        }
> -
> - pr_info("Spectre v2 mitigation: Speculation control IBRS %s",
> -        ibrs_supported ? "supported" : "not-supported");
> -
>   /*
>   * If spectre v2 protection has been enabled, unconditionally fill
>   * RSB during a context switch; this protects against two independent
> @@ -437,21 +435,6 @@ specv2_set_mode:
>   setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
>   pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
>  
> - /*
> - * If we have a full retpoline mode and then disable IBPB in kernel mode
> - * we do not require both.
> - */
> - if (mode == SPECTRE_V2_RETPOLINE_AMD ||
> -    mode == SPECTRE_V2_RETPOLINE_GENERIC)
> - {
> - if (ibrs_supported) {
> - pr_info("Retpoline compiled kernel.  Defaulting IBRS to disabled");
> - set_ibrs_disabled();
> - if (!ibrs_inuse)
> - sysctl_ibrs_enabled = 0;
> - }
> - }
> -
>   /*
>   * Retpoline means the kernel is safe because it has no indirect
>   * branches. Enhanced IBRS protects firmware too, so, enable restricted
> @@ -463,9 +446,23 @@ specv2_set_mode:
>   * the CPU supports Enhanced IBRS, kernel might un-intentionally not
>   * enable IBRS around firmware calls.
>   */
> - if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> - setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> - pr_info("Enabling Restricted Speculation for firmware calls\n");
> + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> + if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> + pr_info("Enabling Restricted Speculation for firmware calls\n");
> + }
> + if (noibrs ||
> +    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> +    mode == SPECTRE_V2_RETPOLINE_AMD ||
> +    mode == SPECTRE_V2_IBRS_ENHANCED) {
> + /*
> + * IBRS disabled via commandline or the kernel is
> + * retpoline compiled or the HW supports enhanced IBRS
> + */
> + set_ibrs_enabled(0);
It took me a while to understand that IBRS Enhanced handling here (and
then all the subtle details at runtime...). I now see that the IBRS bit
of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
ubuntu_restrict_branch_speculation_{start,end}() which is the correct
behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
is 0.

> + } else {
> + set_ibrs_enabled(1);
> + }
>   }
>  }
>  
> @@ -875,8 +872,9 @@ static ssize_t cpu_show_common(struct device *dev, struct device_attribute *attr
>   break;
>  
>   case X86_BUG_SPECTRE_V2:
> - return sprintf(buf, "%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> + return sprintf(buf, "%s%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
>         ibpb_enabled ? ", IBPB" : "",
> +       ibrs_enabled == 2 ? ", IBRS (user space)" : ibrs_enabled ? ", IBRS" : "",
>         boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
>         spectre_v2_module_string());
>  
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 84bd97f8eeab..4fe475c2f745 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -439,17 +439,6 @@ static ssize_t reload_store(struct device *dev,
>   if (!ret)
>   perf_check_microcode();
>  
> - /* Initialize Indirect Branch Restricted Speculation if supported */
> - if (boot_cpu_has(X86_FEATURE_IBRS)) {
> - pr_info("Enabling Indirect Branch Restricted Speculation\n");
> -
> - mutex_lock(&spec_ctrl_mutex);
> - set_ibrs_supported();
> - if (ibrs_inuse)
> - sysctl_ibrs_enabled = 1;
> - mutex_unlock(&spec_ctrl_mutex);
> - }
> -
>   mutex_unlock(&microcode_mutex);
>   put_online_cpus();
>  
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index a5b3404266eb..5587098a5823 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -566,17 +566,15 @@ static void mwait_idle(void)
>   smp_mb(); /* quirk */
>   }
>  
> - if (ibrs_inuse)
> -                        native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> + ubuntu_restrict_branch_speculation_end();
>  
>   __monitor((void *)&current_thread_info()->flags, 0, 0);
> +
>   if (!need_resched()) {
>   __sti_mwait(0, 0);
> - if (ibrs_inuse)
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
> + ubuntu_restrict_branch_speculation_start();
>   } else {
> - if (ibrs_inuse)
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
> + ubuntu_restrict_branch_speculation_start();
>   local_irq_enable();
>   }
>   trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 21c3bcbd69a2..6c9bb4db2ed7 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1697,15 +1697,13 @@ void native_play_dead(void)
>   play_dead_common();
>   tboot_shutdown(TB_SHUTDOWN_WFS);
>  
> - if (ibrs_inuse)
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> + ubuntu_restrict_branch_speculation_end();
>  
>   mwait_play_dead(); /* Only returns on failure */
>   if (cpuidle_play_dead())
>   hlt_play_dead();
>  
> - if (ibrs_inuse)
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
> + ubuntu_restrict_branch_speculation_start();
>  }
>  
>  #else /* ... !CONFIG_HOTPLUG_CPU */
> diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
> index 5b66d4417f8d..20e9a5ac91dc 100644
> --- a/arch/x86/lib/delay.c
> +++ b/arch/x86/lib/delay.c
> @@ -107,8 +107,8 @@ static void delay_mwaitx(unsigned long __loops)
>   for (;;) {
>   delay = min_t(u64, MWAITX_MAX_LOOPS, loops);
>  
> - if (ibrs_inuse && (delay > IBRS_DISABLE_THRESHOLD))
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> + if (delay > IBRS_DISABLE_THRESHOLD)
> + ubuntu_restrict_branch_speculation_end();
>  
>   /*
>   * Use cpu_tss as a cacheline-aligned, seldomly
> @@ -123,8 +123,8 @@ static void delay_mwaitx(unsigned long __loops)
>   */
>   __mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE);
>  
> - if (ibrs_inuse && (delay > IBRS_DISABLE_THRESHOLD))
> - native_wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
> + if (delay > IBRS_DISABLE_THRESHOLD)
> + ubuntu_restrict_branch_speculation_start();
>  
>   end = rdtsc_ordered();
>  
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index 98ac8ff2afab..c4414074bd88 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -50,52 +50,6 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info),
>  
>  int smp_call_function_single_async(int cpu, struct call_single_data *csd);
>  
> -#ifdef CONFIG_X86
> -
> -#define IBxx_INUSE (1 << 0)
> -#define IBxx_SUPPORTED (1 << 1)
> -#define IBxx_DISABLED (1 << 2)
> -
> -/* indicate usage of IBRS to control execution speculation */
> -extern int use_ibrs;
> -extern u32 sysctl_ibrs_enabled;
> -extern struct mutex spec_ctrl_mutex;
> -
> -static inline int __check_ibrs_inuse(void)
> -{
> - if (use_ibrs & IBxx_INUSE)
> - return 1;
> - else
> - /* rmb to prevent wrong speculation for security */
> - rmb();
> - return 0;
> -}
> -
> -#define ibrs_inuse (__check_ibrs_inuse())
> -#define ibrs_supported (use_ibrs & IBxx_SUPPORTED)
> -#define ibrs_disabled (use_ibrs & IBxx_DISABLED)
> -
> -static inline void set_ibrs_supported(void)
> -{
> - use_ibrs |= IBxx_SUPPORTED;
> - if (!ibrs_disabled)
> - use_ibrs |= IBxx_INUSE;
> -}
> -static inline void set_ibrs_disabled(void)
> -{
> - use_ibrs |= IBxx_DISABLED;
> - if (ibrs_inuse)
> - use_ibrs &= ~IBxx_INUSE;
> -}
> -static inline void clear_ibrs_disabled(void)
> -{
> - use_ibrs &= ~IBxx_DISABLED;
> - if (ibrs_supported)
> - use_ibrs |= IBxx_INUSE;
> -}
> -
> -#endif /* CONFIG_X86 */
> -
>  #ifdef CONFIG_SMP
>  
>  #include <linux/preempt.h>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 139681ddf916..48eb4fafc356 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -499,22 +499,6 @@ EXPORT_SYMBOL(smp_call_function);
>  unsigned int setup_max_cpus = NR_CPUS;
>  EXPORT_SYMBOL(setup_max_cpus);
>  
> -#ifdef CONFIG_X86
> -/*
> - * use IBRS
> - * bit 0 = indicate if ibrs is currently in use
> - * bit 1 = indicate if system supports ibrs
> - * bit 2 = indicate if admin disables ibrs
> -*/
> -
> -int use_ibrs;
> -EXPORT_SYMBOL(use_ibrs);
> -#endif
> -
> -/* mutex to serialize IBRS & IBPB control changes */
> -DEFINE_MUTEX(spec_ctrl_mutex);
> -EXPORT_SYMBOL(spec_ctrl_mutex);
> -
>  /*
>   * Setup routine for controlling SMP activation
>   *
> @@ -538,18 +522,6 @@ static int __init nosmp(char *str)
>  
>  early_param("nosmp", nosmp);
>  
> -#ifdef CONFIG_X86
> -static int __init noibrs(char *str)
> -{
> - set_ibrs_disabled();
> -
> - return 0;
> -}
> -
> -early_param("noibrs", noibrs);
> -#endif
> -
> -
>  /* this is hard limit */
>  static int __init nrcpus(char *str)
>  {
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4c9f00d16366..11c626dd1b1c 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -202,8 +202,8 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
>  #endif
>  
>  #ifdef CONFIG_X86
> -int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
> -                 void __user *buffer, size_t *lenp, loff_t *ppos);
> +/* Mutex to serialize IBPB and IBRS runtime control changes */
> +DEFINE_MUTEX(spec_ctrl_mutex);
>  
>  unsigned int ibpb_enabled = 0;
>  EXPORT_SYMBOL(ibpb_enabled);   /* Required in some modules */
> @@ -250,6 +250,70 @@ static int ibpb_enabled_handler(struct ctl_table *table, int write,
>  
>   return set_ibpb_enabled(__ibpb_enabled);
>  }
> +
> +unsigned int ibrs_enabled = 0;
> +EXPORT_SYMBOL(ibrs_enabled);   /* Required in some modules */
> +
> +static unsigned int __ibrs_enabled = 0;   /* procfs shadow variable */
> +
> +int set_ibrs_enabled(unsigned int val)
> +{
> + int error = 0;
> + unsigned int cpu;
> +
> + mutex_lock(&spec_ctrl_mutex);
> +
> + /* Only enable/disable IBRS if the CPU supports it */
> + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> + ibrs_enabled = val;
> + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
I think the "Spectre V2 : " portion of this message is redundant and is
also not present in upstream's message which could confuse user or
scripts that are looking at log messages. Can it be removed when
applying the patch?

> + "Branch Restricted Speculation%s\n",
> + ibrs_enabled ? "Enabling" : "Disabling",
> + ibrs_enabled == 2 ? " (user space)" : "");
> +
> + if (ibrs_enabled == 0) {
> + /* Always disable IBRS */
> + u64 val = x86_spec_ctrl_base;
> +
> + for_each_online_cpu(cpu) {
> + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> + }
> + } else if (ibrs_enabled == 2) {
> + /* Always enable IBRS, even in user space */
> + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> +
> + for_each_online_cpu(cpu) {
> + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> + }
> + }
> + } else {
> + ibrs_enabled = 0;
> + if (val) {
> + /* IBRS is not supported but we try to turn it on */
> + error = -EINVAL;
> + }
> + }
> +
> + /* Update the shadow variable */
> + __ibrs_enabled = ibrs_enabled;
> +
> + mutex_unlock(&spec_ctrl_mutex);
> +
> + return error;
> +}
> +
> +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> +                               void __user *buffer, size_t *lenp,
> +                               loff_t *ppos)
> +{
> + int error;
> +
> + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> + if (error)
> + return error;
> +
> + return set_ibrs_enabled(__ibrs_enabled);
> +}
>  #endif
IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
personally don't feel like this is a problem but I wanted to mention it.

Tyler

>  
>  #ifdef CONFIG_MAGIC_SYSRQ
> @@ -288,9 +352,6 @@ extern struct ctl_table epoll_table[];
>  int sysctl_legacy_va_layout;
>  #endif
>  
> -u32 sysctl_ibrs_enabled = 0;
> -EXPORT_SYMBOL(sysctl_ibrs_enabled);
> -
>  /* The default sysctl tables: */
>  
>  static struct ctl_table sysctl_base_table[] = {
> @@ -1279,10 +1340,10 @@ static struct ctl_table kern_table[] = {
>  #ifdef CONFIG_X86
>   {
>   .procname       = "ibrs_enabled",
> - .data           = &sysctl_ibrs_enabled,
> + .data           = &__ibrs_enabled,
>   .maxlen         = sizeof(unsigned int),
>   .mode           = 0644,
> - .proc_handler   = proc_dointvec_ibrs_ctrl,
> + .proc_handler   = ibrs_enabled_handler,
>   .extra1         = &zero,
>   .extra2         = &two,
>   },
> @@ -2453,46 +2514,6 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
>   do_proc_dointvec_minmax_conv, &param);
>  }
>  
> -#ifdef CONFIG_X86
> -int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
> - void __user *buffer, size_t *lenp, loff_t *ppos)
> -{
> - int ret;
> - unsigned int cpu;
> -
> - ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> - pr_debug("sysctl_ibrs_enabled = %u\n", sysctl_ibrs_enabled);
> - pr_debug("before:use_ibrs = %d\n", use_ibrs);
> - mutex_lock(&spec_ctrl_mutex);
> - if (sysctl_ibrs_enabled == 0) {
> - /* always set IBRS off */
> - set_ibrs_disabled();
> - if (ibrs_supported) {
> - for_each_online_cpu(cpu)
> - wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> - }
> - } else if (sysctl_ibrs_enabled == 2) {
> - /* always set IBRS on, even in user space */
> - clear_ibrs_disabled();
> - if (ibrs_supported) {
> - for_each_online_cpu(cpu)
> - wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | SPEC_CTRL_IBRS);
> - } else {
> - sysctl_ibrs_enabled = 0;
> - }
> - } else if (sysctl_ibrs_enabled == 1) {
> - /* use IBRS in kernel */
> - clear_ibrs_disabled();
> - if (!ibrs_inuse)
> - /* platform don't support ibrs */
> - sysctl_ibrs_enabled = 0;
> - }
> - mutex_unlock(&spec_ctrl_mutex);
> - pr_debug("after:use_ibrs = %d\n", use_ibrs);
> - return ret;
> -}
> -#endif
> -
>  static void validate_coredump_safety(void)
>  {
>  #ifdef CONFIG_COREDUMP
> --
> 2.19.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK/CMNT: [SRU][Xenial][PATCH 1/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling

Tyler Hicks-2
In reply to this post by Juerg Haefliger
On 2018-11-21 14:58:29, Juerg Haefliger wrote:

> In Ubuntu, we have runtime control for enabling/disabling IBPB via the
> commandline ("noibpb") and through the proc interface
> /proc/sys/kernel/ibpb_enabled. This commit simplifies the current
> (broken) implementation by merging it with all the IBPB-related upstream
> changes from previous commits.
>
> What we have now is the upstream implementation for detecting the presence
> of IBPB support which is used in the alternative MSR write in
> indirect_branch_prediction_barrier() to enable IBPB. On top of that, this
> commit adds a global state variable 'ibpb_enabled' which is set to 1 if
> the CPU supports IBPB but can be overridden via the commandline "noibpb"
> switch or by writting 0 or 1 to /proc/sys/kernel/ibpb_enabled at runtime.
>
> If ibpb_enabled is 0, indirect_branch_prediction_barrier() is turned into a
> no-op, same as if the platform doesn't support IBPB.
>
> CVE-2017-5715
>
> Signed-off-by: Juerg Haefliger <[hidden email]>
Acked-by: Tyler Hicks <[hidden email]>

A comment below...

> ---
>  arch/x86/include/asm/nospec-branch.h |  8 ++-
>  arch/x86/include/asm/spec_ctrl.h     |  1 -
>  arch/x86/kernel/cpu/amd.c            |  5 +-
>  arch/x86/kernel/cpu/bugs.c           | 31 +++++++---
>  arch/x86/kernel/cpu/microcode/core.c | 12 ----
>  arch/x86/kvm/svm.c                   |  6 +-
>  arch/x86/kvm/vmx.c                   |  3 +-
>  arch/x86/mm/tlb.c                    |  2 +-
>  include/linux/smp.h                  | 37 -----------
>  kernel/smp.c                         | 18 ------
>  kernel/sysctl.c                      | 92 +++++++++++++++++-----------
>  11 files changed, 93 insertions(+), 122 deletions(-)
>
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 8600916c073a..dcc7b0348fbc 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -184,6 +184,10 @@
>  # define THUNK_TARGET(addr) [thunk_target] "rm" (addr)
>  #endif
>  
> +/* The IBPB runtime control knob */
> +extern unsigned int ibpb_enabled;
> +int set_ibpb_enabled(unsigned int);
> +
>  /* The Spectre V2 mitigation variants */
>  enum spectre_v2_mitigation {
>   SPECTRE_V2_NONE,
> @@ -243,7 +247,9 @@ static inline void indirect_branch_prediction_barrier(void)
>  {
>   u64 val = PRED_CMD_IBPB;
>  
> - alternative_msr_write(MSR_IA32_PRED_CMD, val, X86_FEATURE_USE_IBPB);
> + if (ibpb_enabled)
> + alternative_msr_write(MSR_IA32_PRED_CMD, val,
> +      X86_FEATURE_USE_IBPB);
>  }
>  
>  /*
> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
> index c18bc77bb98e..49c3b0a83e9f 100644
> --- a/arch/x86/include/asm/spec_ctrl.h
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -9,7 +9,6 @@
>  #ifdef __ASSEMBLY__
>  
>  .extern use_ibrs
> -.extern use_ibpb
>  
>  #define __ASM_ENABLE_IBRS \
>   pushq %rax; \
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 5d01efd966a9..609b31deeb25 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -21,6 +21,9 @@
>  
>  #include "cpu.h"
>  
> +/* "noibpb" commandline parameter present (1) or not (0) */
> +extern unsigned int noibpb;
> +
>  /*
>   * nodes_per_socket: Stores the number of nodes per socket.
>   * Refer to Fam15h Models 00-0fh BKDG - CPUID Fn8000_001E_ECX
> @@ -842,7 +845,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>   * speculative control features, IBPB type support can be achieved by
>   * disabling indirect branch predictor support.
>   */
> - if (!ibpb_disabled && !cpu_has(c, X86_FEATURE_SPEC_CTRL) &&
> + if (!noibpb && !cpu_has(c, X86_FEATURE_SPEC_CTRL) &&
>      !cpu_has(c, X86_FEATURE_IBPB)) {
>   u64 val;
>  
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index a90514f3514c..41c57b5c870c 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -30,6 +30,16 @@
>  #include <asm/intel-family.h>
>  #include <asm/e820.h>
>  
> +unsigned int noibpb = 0;
> +
> +static int __init noibpb_handler(char *str)
> +{
> + noibpb = 1;
> + return 0;
> +}
> +
> +early_param("noibpb", noibpb_handler);
> +
>  static void __init spectre_v2_select_mitigation(void);
>  static void __init ssb_select_mitigation(void);
>  static void __init l1tf_select_mitigation(void);
> @@ -390,14 +400,18 @@ specv2_set_mode:
>   spectre_v2_enabled = mode;
>   pr_info("%s\n", spectre_v2_strings[mode]);
>  
> - /* Initialize Indirect Branch Prediction Barrier if supported */
> + /*
> + * Initialize Indirect Branch Prediction Barrier if supported and not
> + * disabled on the commandline
> + */
>   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");
> -
> - set_ibpb_supported();
> - if (ibpb_inuse)
> - sysctl_ibpb_enabled = 1;
> + if (noibpb) {
> + /* IBPB disabled via commandline */
> + set_ibpb_enabled(0);
> + } else {
> + set_ibpb_enabled(1);
> + }
>   }
>  
>   /* Initialize Indirect Branch Restricted Speculation if supported */
> @@ -409,8 +423,7 @@ specv2_set_mode:
>   sysctl_ibrs_enabled = 1;
>          }
>  
> - pr_info("Spectre v2 mitigation: Speculation control IBPB %s IBRS %s",
> -        ibpb_supported ? "supported" : "not-supported",
> + pr_info("Spectre v2 mitigation: Speculation control IBRS %s",
>          ibrs_supported ? "supported" : "not-supported");
>  
>   /*
> @@ -863,7 +876,7 @@ 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\n", spectre_v2_strings[spectre_v2_enabled],
> -       ibpb_inuse ? ", IBPB (Intel v4)" : "",
> +       ibpb_enabled ? ", IBPB" : "",
>         boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
>         spectre_v2_module_string());
>  
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 63e3db171945..84bd97f8eeab 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -439,18 +439,6 @@ static ssize_t reload_store(struct device *dev,
>   if (!ret)
>   perf_check_microcode();
>  
> - /* Initialize Indirect Branch Prediction Barrier if supported */
> - if (boot_cpu_has(X86_FEATURE_IBPB)) {
> - setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> - pr_info("Enabling Indirect Branch Prediction Barrier\n");
> -
> - mutex_lock(&spec_ctrl_mutex);
> - set_ibpb_supported();
> - if (ibpb_inuse)
> - sysctl_ibpb_enabled = 1;
> - mutex_unlock(&spec_ctrl_mutex);
> - }
> -
>   /* Initialize Indirect Branch Restricted Speculation if supported */
>   if (boot_cpu_has(X86_FEATURE_IBRS)) {
>   pr_info("Enabling Indirect Branch Restricted Speculation\n");
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 189e197d9193..034e3116415b 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1230,8 +1230,7 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
>   * The VMCB could be recycled, causing a false negative in svm_vcpu_load;
>   * block speculative execution.
>   */
> - if (ibpb_inuse)
> - wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
> + indirect_branch_prediction_barrier();
>  }
>  
>  static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> @@ -1265,8 +1264,7 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  
>   if (sd->current_vmcb != svm->vmcb) {
>   sd->current_vmcb = svm->vmcb;
> - if (ibpb_inuse)
> - wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
> + indirect_branch_prediction_barrier();
>   }
>  }
>  
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index fccd9224e4b3..22012bcc4ef6 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2245,8 +2245,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>   if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) {
>   per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
>   vmcs_load(vmx->loaded_vmcs->vmcs);
> - if (ibpb_inuse)
> - native_wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB);
> + indirect_branch_prediction_barrier();
>   }
>  
>   if (vmx->loaded_vmcs->cpu != cpu) {
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 4762bb43bffa..c229094ad12d 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -127,7 +127,7 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>   */
>   if (tsk && tsk->mm &&
>      tsk->mm->context.ctx_id != last_ctx_id &&
> -    get_dumpable(tsk->mm) != SUID_DUMP_USER && ibpb_inuse)
> +    get_dumpable(tsk->mm) != SUID_DUMP_USER)
>   indirect_branch_prediction_barrier();
>  
>   /*
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index a3b959424808..98ac8ff2afab 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -94,43 +94,6 @@ static inline void clear_ibrs_disabled(void)
>   use_ibrs |= IBxx_INUSE;
>  }
>  
> -/* indicate usage of IBPB to control execution speculation */
> -extern int use_ibpb;
> -extern u32 sysctl_ibpb_enabled;
> -
> -static inline int __check_ibpb_inuse(void)
> -{
> - if (use_ibpb & IBxx_INUSE)
> - return 1;
> - else
> - /* rmb to prevent wrong speculation for security */
> - rmb();
> - return 0;
> -}
> -
> -#define ibpb_inuse (__check_ibpb_inuse())
> -#define ibpb_supported (use_ibpb & IBxx_SUPPORTED)
> -#define ibpb_disabled (use_ibpb & IBxx_DISABLED)
> -
> -static inline void set_ibpb_supported(void)
> -{
> - use_ibpb |= IBxx_SUPPORTED;
> - if (!ibpb_disabled)
> - use_ibpb |= IBxx_INUSE;
> -}
> -static inline void set_ibpb_disabled(void)
> -{
> - use_ibpb |= IBxx_DISABLED;
> - if (ibpb_inuse)
> - use_ibpb &= ~IBxx_INUSE;
> -}
> -static inline void clear_ibpb_disabled(void)
> -{
> - use_ibpb &= ~IBxx_DISABLED;
> - if (ibpb_supported)
> - use_ibpb |= IBxx_INUSE;
> -}
> -
>  #endif /* CONFIG_X86 */
>  
>  #ifdef CONFIG_SMP
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 4879de0b392e..139681ddf916 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -509,15 +509,6 @@ EXPORT_SYMBOL(setup_max_cpus);
>  
>  int use_ibrs;
>  EXPORT_SYMBOL(use_ibrs);
> -
> -/*
> - * use IBRS
> - * bit 0 = indicate if ibpb is currently in use
> - * bit 1 = indicate if system supports ibpb
> - * bit 2 = indicate if admin disables ibpb
> -*/
> -int use_ibpb;
> -EXPORT_SYMBOL(use_ibpb);
>  #endif
>  
>  /* mutex to serialize IBRS & IBPB control changes */
> @@ -556,15 +547,6 @@ static int __init noibrs(char *str)
>  }
>  
>  early_param("noibrs", noibrs);
> -
> -static int __init noibpb(char *str)
> -{
> - set_ibpb_disabled();
> -
> - return 0;
> -}
> -
> -early_param("noibpb", noibpb);
>  #endif
>  
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 25501febd7c6..4c9f00d16366 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -204,8 +204,52 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
>  #ifdef CONFIG_X86
>  int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
>                   void __user *buffer, size_t *lenp, loff_t *ppos);
> -int proc_dointvec_ibpb_ctrl(struct ctl_table *table, int write,
> -                 void __user *buffer, size_t *lenp, loff_t *ppos);
> +
> +unsigned int ibpb_enabled = 0;
> +EXPORT_SYMBOL(ibpb_enabled);   /* Required in some modules */
> +
> +static unsigned int __ibpb_enabled = 0;   /* procfs shadow variable */
> +
> +int set_ibpb_enabled(unsigned int val)
> +{
> + int error = 0;
> +
> + mutex_lock(&spec_ctrl_mutex);
> +
> + /* Only enable IBPB if the CPU supports it */
> + if (boot_cpu_has(X86_FEATURE_IBPB)) {
> + ibpb_enabled = val;
> + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "
I think the "Spectre V2 : " portion of this message is redundant and is
also not present in upstream's message which could confuse user or
scripts that are looking at log messages. Can it be removed when
applying the patch?

Tyler

> + "Branch Prediction Barrier\n",
> + ibpb_enabled ? "Enabling" : "Disabling");
> + } else {
> + ibpb_enabled = 0;
> + if (val) {
> + /* IBPB is not supported but we try to turn it on */
> + error = -EINVAL;
> + }
> + }
> +
> + /* Update the shadow variable */
> + __ibpb_enabled = ibpb_enabled;
> +
> + mutex_unlock(&spec_ctrl_mutex);
> +
> + return error;
> +}
> +
> +static int ibpb_enabled_handler(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int error;
> +
> + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> + if (error)
> + return error;
> +
> + return set_ibpb_enabled(__ibpb_enabled);
> +}
>  #endif
>  
>  #ifdef CONFIG_MAGIC_SYSRQ
> @@ -246,8 +290,6 @@ int sysctl_legacy_va_layout;
>  
>  u32 sysctl_ibrs_enabled = 0;
>  EXPORT_SYMBOL(sysctl_ibrs_enabled);
> -u32 sysctl_ibpb_enabled = 0;
> -EXPORT_SYMBOL(sysctl_ibpb_enabled);
>  
>  /* The default sysctl tables: */
>  
> @@ -1245,13 +1287,13 @@ static struct ctl_table kern_table[] = {
>   .extra2         = &two,
>   },
>   {
> - .procname       = "ibpb_enabled",
> - .data           = &sysctl_ibpb_enabled,
> - .maxlen         = sizeof(unsigned int),
> - .mode           = 0644,
> - .proc_handler   = proc_dointvec_ibpb_ctrl,
> - .extra1         = &zero,
> - .extra2         = &one,
> + .procname = "ibpb_enabled",
> + .data = &__ibpb_enabled,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler   = ibpb_enabled_handler,
> + .extra1 = &zero,
> + .extra2 = &one,
>   },
>  #endif
>   { }
> @@ -2419,8 +2461,8 @@ int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
>   unsigned int cpu;
>  
>   ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> - pr_debug("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
> - pr_debug("before:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
> + pr_debug("sysctl_ibrs_enabled = %u\n", sysctl_ibrs_enabled);
> + pr_debug("before:use_ibrs = %d\n", use_ibrs);
>   mutex_lock(&spec_ctrl_mutex);
>   if (sysctl_ibrs_enabled == 0) {
>   /* always set IBRS off */
> @@ -2446,29 +2488,7 @@ int proc_dointvec_ibrs_ctrl(struct ctl_table *table, int write,
>   sysctl_ibrs_enabled = 0;
>   }
>   mutex_unlock(&spec_ctrl_mutex);
> - pr_debug("after:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
> - return ret;
> -}
> -
> -int proc_dointvec_ibpb_ctrl(struct ctl_table *table, int write,
> - void __user *buffer, size_t *lenp, loff_t *ppos)
> -{
> - int ret;
> -
> - ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> - pr_debug("sysctl_ibrs_enabled = %u, sysctl_ibpb_enabled = %u\n", sysctl_ibrs_enabled, sysctl_ibpb_enabled);
> - pr_debug("before:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
> - mutex_lock(&spec_ctrl_mutex);
> - if (sysctl_ibpb_enabled == 0)
> - set_ibpb_disabled();
> - else if (sysctl_ibpb_enabled == 1) {
> - clear_ibpb_disabled();
> - if (!ibpb_inuse)
> - /* platform don't support ibpb */
> - sysctl_ibpb_enabled = 0;
> - }
> - mutex_unlock(&spec_ctrl_mutex);
> - pr_debug("after:use_ibrs = %d, use_ibpb = %d\n", use_ibrs, use_ibpb);
> + pr_debug("after:use_ibrs = %d\n", use_ibrs);
>   return ret;
>  }
>  #endif
> --
> 2.19.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Xenial][PATCH 3/3] UBUNTU: SAUCE: x86/speculation: Move RSB_CTXSW hunk

Tyler Hicks-2
In reply to this post by Juerg Haefliger
On 2018-11-21 14:58:31, Juerg Haefliger wrote:
> Move the RSB_CTXSW hunk further up in spectre_v2_select_mitigation() to
> match upstream. No functional changes.
>
> CVE-2017-5715
>
> Signed-off-by: Juerg Haefliger <[hidden email]>

Acked-by: Tyler Hicks <[hidden email]>

Tyler

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK/CMNT: [SRU][Xenial][PATCH] UBUNTU: SAUCE: x86/speculation: Only report IBPB/IBRS state changes

Tyler Hicks-2
In reply to this post by Juerg Haefliger
On 2018-11-27 10:27:42, Juerg Haefliger wrote:
> Only print the IBPB/IBRS state to the log if it actually changes. Otherwise
> the log is polluted everytime the procfs file is read from.
>
> Signed-off-by: Juerg Haefliger <[hidden email]>

Acked with the log message adjustment that I asked for in the earlier
two patches.

Acked-by: Tyler Hicks <[hidden email]>

Tyler

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/CMNT: [SRU][Xenial][PATCH 1/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling

Juerg Haefliger
In reply to this post by Tyler Hicks-2
> > +int set_ibpb_enabled(unsigned int val)

> > +{
> > + int error = 0;
> > +
> > + mutex_lock(&spec_ctrl_mutex);
> > +
> > + /* Only enable IBPB if the CPU supports it */
> > + if (boot_cpu_has(X86_FEATURE_IBPB)) {
> > + ibpb_enabled = val;
> > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "  
>
> I think the "Spectre V2 : " portion of this message is redundant and is
> also not present in upstream's message which could confuse user or
> scripts that are looking at log messages. Can it be removed when
> applying the patch?
This is exactly how upstream reported it until 5 days ago (commit
4c71a2b6fd7e changed that message format) :-)

$ uname -r
4.18.0-11-generic
$ dmesg | grep 'Spectre V2'
[    0.028000] Spectre V2 : Mitigation: Full generic retpoline
[    0.028000] Spectre V2 : Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch
[    0.028000] Spectre V2 : Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier
[    0.028000] Spectre V2 : Enabling Restricted Speculation for firmware calls

...Juerg


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/CMNT: [SRU][Xenial][PATCH 1/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling

Tyler Hicks-2
On December 3, 2018 6:53:50 AM CST, Juerg Haefliger <[hidden email]> wrote:

>> > +int set_ibpb_enabled(unsigned int val)
>> > +{
>> > + int error = 0;
>> > +
>> > + mutex_lock(&spec_ctrl_mutex);
>> > +
>> > + /* Only enable IBPB if the CPU supports it */
>> > + if (boot_cpu_has(X86_FEATURE_IBPB)) {
>> > + ibpb_enabled = val;
>> > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "  
>>
>> I think the "Spectre V2 : " portion of this message is redundant and
>is
>> also not present in upstream's message which could confuse user or
>> scripts that are looking at log messages. Can it be removed when
>> applying the patch?
>
>This is exactly how upstream reported it until 5 days ago (commit
>4c71a2b6fd7e changed that message format) :-)
>
>$ uname -r
>4.18.0-11-generic
>$ dmesg | grep 'Spectre V2'
>[    0.028000] Spectre V2 : Mitigation: Full generic retpoline
>[    0.028000] Spectre V2 : Spectre v2 / SpectreRSB mitigation: Filling
>RSB on context switch
>[    0.028000] Spectre V2 : Spectre v2 mitigation: Enabling Indirect
>Branch Prediction Barrier
>[    0.028000] Spectre V2 : Enabling Restricted Speculation for
>firmware calls
>
>...Juerg

Ha! We'll, I don't feel strongly about changing it now. I'll leave that up to you to decide.

Tyler

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Juerg Haefliger
In reply to this post by Tyler Hicks-2
> > +#define ubuntu_restrict_branch_speculation_start() \

> > +do { \
> > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
> > + \
> > + if (ibrs_enabled) \
> > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > +} while (0)
> > +
> > +#define ubuntu_restrict_branch_speculation_end() \
> > +do { \
> > + u64 val = x86_spec_ctrl_base; \
> > + \
> > + if (ibrs_enabled) \
> > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > + } while (0)
I just realized that the above is incorrect. If ibrs_enabled == 2 then we set
the IBRS bit on every CPU and should not fiddle with them here, no?

So it should be:
  if (ibrs_enabled == 1)
      native_wrmsrl(MSR_IA32_SPEC_CTRL, val);


> >   /*
> >   * Retpoline means the kernel is safe because it has no indirect
> >   * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > @@ -463,9 +446,23 @@ specv2_set_mode:
> >   * the CPU supports Enhanced IBRS, kernel might un-intentionally not
> >   * enable IBRS around firmware calls.
> >   */
> > - if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> > - setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > - pr_info("Enabling Restricted Speculation for firmware calls\n");
> > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > + if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> > + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > + pr_info("Enabling Restricted Speculation for firmware calls\n");
> > + }
> > + if (noibrs ||
> > +    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> > +    mode == SPECTRE_V2_RETPOLINE_AMD ||
> > +    mode == SPECTRE_V2_IBRS_ENHANCED) {
> > + /*
> > + * IBRS disabled via commandline or the kernel is
> > + * retpoline compiled or the HW supports enhanced IBRS
> > + */
> > + set_ibrs_enabled(0);  
>
> It took me a while to understand that IBRS Enhanced handling here (and
> then all the subtle details at runtime...). I now see that the IBRS bit
> of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
> Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
> the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
> ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
> the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
> ubuntu_restrict_branch_speculation_{start,end}() which is the correct
> behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
> is 0.
Hmm... The call set_ibrs_enabled(0) will clear the IBRS bit on all CPUs.
On hindsight, that seems wrong in the presence of enhanced IBRS. Maybe we
should disable the runtime control completely if mode ==
SPECTRE_V2_IBRS_ENHANCED?


> > +int set_ibrs_enabled(unsigned int val)
> > +{
> > + int error = 0;
> > + unsigned int cpu;
> > +
> > + mutex_lock(&spec_ctrl_mutex);
> > +
> > + /* Only enable/disable IBRS if the CPU supports it */
> > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > + ibrs_enabled = val;
> > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "  
>
> I think the "Spectre V2 : " portion of this message is redundant and is
> also not present in upstream's message which could confuse user or
> scripts that are looking at log messages. Can it be removed when
> applying the patch?
Again, this is how upstream reported it until just recently.


 

> > + "Branch Restricted Speculation%s\n",
> > + ibrs_enabled ? "Enabling" : "Disabling",
> > + ibrs_enabled == 2 ? " (user space)" : "");
> > +
> > + if (ibrs_enabled == 0) {
> > + /* Always disable IBRS */
> > + u64 val = x86_spec_ctrl_base;
> > +
> > + for_each_online_cpu(cpu) {
> > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > + }
> > + } else if (ibrs_enabled == 2) {
> > + /* Always enable IBRS, even in user space */
> > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> > +
> > + for_each_online_cpu(cpu) {
> > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > + }
> > + }
> > + } else {
> > + ibrs_enabled = 0;
> > + if (val) {
> > + /* IBRS is not supported but we try to turn it on */
> > + error = -EINVAL;
> > + }
> > + }
> > +
> > + /* Update the shadow variable */
> > + __ibrs_enabled = ibrs_enabled;
> > +
> > + mutex_unlock(&spec_ctrl_mutex);
> > +
> > + return error;
> > +}
> > +
> > +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> > +                               void __user *buffer, size_t *lenp,
> > +                               loff_t *ppos)
> > +{
> > + int error;
> > +
> > + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > + if (error)
> > + return error;
> > +
> > + return set_ibrs_enabled(__ibrs_enabled);
> > +}
> >  #endif  
>
> IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
> already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
> personally don't feel like this is a problem but I wanted to mention it.
As mentioned above, with enhanced IBRS we can still fiddle with the IBRS bit
in the MSR via the runtime controls. We probably don't want that.

...Juerg

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

NACK: [SRU][Xenial][PATCH 0/3] Cleanups for CVE-2017-5715 (Spectre v2)

Juerg Haefliger
In reply to this post by Juerg Haefliger
This needs more work.

...Juerg

On Wed, 21 Nov 2018 14:58:28 +0100
Juerg Haefliger <[hidden email]> wrote:

> This patchset cleans up the Ubuntu-specific IBRS and IBPB runtime controls.
> The runtime controls from the embargoed patches are messy and spread all
> over the place. These patches consolidate the modifications into the
> proper places (commandline options in arch/x86/kernel/cpu/bugs.c instead of
> kernel/smp.c, speculation macros in arch/x86/include/asm/nospec-branch.h
> instead of open-coded) and merge them with the additional spectre-related
> changes that went in recently.
>
> In addtion, the 2nd patch adds an entry to
> /sys/devices/system/cpu/vulnerabilities/spectre_v2 when IBRS is enabled
> via procfs to return the full set of enabled mitigations.
>
> Compile-tested all architectures. Ran release tests to verify no
> regression is introduced. Fiddled with the ibrs_enabled and ibpb_enabled
> procfs controls to verify proper behaviour.
>
> Signed-off-by: Juerg Haefliger <[hidden email]>
>
>
> Juerg Haefliger (3):
>   UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling
>   UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling
>   UBUNTU: SAUCE: x86/speculation: Move RSB_CTXSW hunk
>
>  arch/x86/include/asm/mwait.h         |   6 +-
>  arch/x86/include/asm/nospec-branch.h |  28 +++-
>  arch/x86/include/asm/spec_ctrl.h     |  11 +-
>  arch/x86/kernel/cpu/amd.c            |   5 +-
>  arch/x86/kernel/cpu/bugs.c           |  87 +++++++-----
>  arch/x86/kernel/cpu/microcode/core.c |  23 ---
>  arch/x86/kernel/process.c            |  10 +-
>  arch/x86/kernel/smpboot.c            |   6 +-
>  arch/x86/kvm/svm.c                   |   6 +-
>  arch/x86/kvm/vmx.c                   |   3 +-
>  arch/x86/lib/delay.c                 |   8 +-
>  arch/x86/mm/tlb.c                    |   2 +-
>  include/linux/smp.h                  |  83 -----------
>  kernel/smp.c                         |  46 ------
>  kernel/sysctl.c                      | 201 ++++++++++++++++-----------
>  15 files changed, 223 insertions(+), 302 deletions(-)
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Tyler Hicks-2
In reply to this post by Juerg Haefliger
On 2018-12-03 14:17:49, Juerg Haefliger wrote:

> > > +#define ubuntu_restrict_branch_speculation_start() \
> > > +do { \
> > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
> > > + \
> > > + if (ibrs_enabled) \
> > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > +} while (0)
> > > +
> > > +#define ubuntu_restrict_branch_speculation_end() \
> > > +do { \
> > > + u64 val = x86_spec_ctrl_base; \
> > > + \
> > > + if (ibrs_enabled) \
> > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > + } while (0)
>
> I just realized that the above is incorrect. If ibrs_enabled == 2 then we set
> the IBRS bit on every CPU and should not fiddle with them here, no?
>
> So it should be:
>   if (ibrs_enabled == 1)
>       native_wrmsrl(MSR_IA32_SPEC_CTRL, val);
Yes, I think that's correct.

>
>
> > >   /*
> > >   * Retpoline means the kernel is safe because it has no indirect
> > >   * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > > @@ -463,9 +446,23 @@ specv2_set_mode:
> > >   * the CPU supports Enhanced IBRS, kernel might un-intentionally not
> > >   * enable IBRS around firmware calls.
> > >   */
> > > - if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > - setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > - pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > + if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > + pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > + }
> > > + if (noibrs ||
> > > +    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> > > +    mode == SPECTRE_V2_RETPOLINE_AMD ||
> > > +    mode == SPECTRE_V2_IBRS_ENHANCED) {
> > > + /*
> > > + * IBRS disabled via commandline or the kernel is
> > > + * retpoline compiled or the HW supports enhanced IBRS
> > > + */
> > > + set_ibrs_enabled(0);  
> >
> > It took me a while to understand that IBRS Enhanced handling here (and
> > then all the subtle details at runtime...). I now see that the IBRS bit
> > of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
> > Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
> > the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
> > ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
> > the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
> > ubuntu_restrict_branch_speculation_{start,end}() which is the correct
> > behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
> > is 0.
>
> Hmm... The call set_ibrs_enabled(0) will clear the IBRS bit on all CPUs.
> On hindsight, that seems wrong in the presence of enhanced IBRS. Maybe we
> should disable the runtime control completely if mode ==
> SPECTRE_V2_IBRS_ENHANCED?
The call set_ibrs_enabled(0) won't clear the IBRS bit because
x86_spec_ctrl_base has the SPEC_CTRL_IBRS bit set when Enhanced IBRS is
supported. However, one of the main benefits of Enhanced IBRS is that
you write to the MSR once and then don't have to toggle it at all so
set_ibrs_enabled(0) will be inefficient.

>
> > > +int set_ibrs_enabled(unsigned int val)
> > > +{
> > > + int error = 0;
> > > + unsigned int cpu;
> > > +
> > > + mutex_lock(&spec_ctrl_mutex);
> > > +
> > > + /* Only enable/disable IBRS if the CPU supports it */
> > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > + ibrs_enabled = val;
> > > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "  
> >
> > I think the "Spectre V2 : " portion of this message is redundant and is
> > also not present in upstream's message which could confuse user or
> > scripts that are looking at log messages. Can it be removed when
> > applying the patch?
>
> Again, this is how upstream reported it until just recently.
>
>
>  
> > > + "Branch Restricted Speculation%s\n",
> > > + ibrs_enabled ? "Enabling" : "Disabling",
> > > + ibrs_enabled == 2 ? " (user space)" : "");
> > > +
> > > + if (ibrs_enabled == 0) {
> > > + /* Always disable IBRS */
> > > + u64 val = x86_spec_ctrl_base;
> > > +
> > > + for_each_online_cpu(cpu) {
> > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > + }
> > > + } else if (ibrs_enabled == 2) {
> > > + /* Always enable IBRS, even in user space */
> > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> > > +
> > > + for_each_online_cpu(cpu) {
> > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > + }
> > > + }
> > > + } else {
> > > + ibrs_enabled = 0;
> > > + if (val) {
> > > + /* IBRS is not supported but we try to turn it on */
> > > + error = -EINVAL;
> > > + }
> > > + }
> > > +
> > > + /* Update the shadow variable */
> > > + __ibrs_enabled = ibrs_enabled;
> > > +
> > > + mutex_unlock(&spec_ctrl_mutex);
> > > +
> > > + return error;
> > > +}
> > > +
> > > +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> > > +                               void __user *buffer, size_t *lenp,
> > > +                               loff_t *ppos)
> > > +{
> > > + int error;
> > > +
> > > + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > > + if (error)
> > > + return error;
> > > +
> > > + return set_ibrs_enabled(__ibrs_enabled);
> > > +}
> > >  #endif  
> >
> > IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
> > already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
> > personally don't feel like this is a problem but I wanted to mention it.
>
> As mentioned above, with enhanced IBRS we can still fiddle with the IBRS bit
> in the MSR via the runtime controls. We probably don't want that.
Now I'm thinking that it makes sense to be able to disable and enable
Enhanced IBRS with the sysctl. Why wouldn't we want to support that?

Tyler

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Juerg Haefliger
On Tue, 4 Dec 2018 13:48:37 -0600
Tyler Hicks <[hidden email]> wrote:

> On 2018-12-03 14:17:49, Juerg Haefliger wrote:
> > > > +#define ubuntu_restrict_branch_speculation_start() \
> > > > +do { \
> > > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
> > > > + \
> > > > + if (ibrs_enabled) \
> > > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > > +} while (0)
> > > > +
> > > > +#define ubuntu_restrict_branch_speculation_end() \
> > > > +do { \
> > > > + u64 val = x86_spec_ctrl_base; \
> > > > + \
> > > > + if (ibrs_enabled) \
> > > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > > + } while (0)  
> >
> > I just realized that the above is incorrect. If ibrs_enabled == 2 then we set
> > the IBRS bit on every CPU and should not fiddle with them here, no?
> >
> > So it should be:
> >   if (ibrs_enabled == 1)
> >       native_wrmsrl(MSR_IA32_SPEC_CTRL, val);  
>
> Yes, I think that's correct.
After running some more tests I take my earlier statement back. Primarily
because it would change the current behaviour. So I leave it as it is until
somebody reports an issue.

 

> >
> >  
> > > >   /*
> > > >   * Retpoline means the kernel is safe because it has no indirect
> > > >   * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > > > @@ -463,9 +446,23 @@ specv2_set_mode:
> > > >   * the CPU supports Enhanced IBRS, kernel might un-intentionally not
> > > >   * enable IBRS around firmware calls.
> > > >   */
> > > > - if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > - setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > - pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > + if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > + pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > + }
> > > > + if (noibrs ||
> > > > +    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> > > > +    mode == SPECTRE_V2_RETPOLINE_AMD ||
> > > > +    mode == SPECTRE_V2_IBRS_ENHANCED) {
> > > > + /*
> > > > + * IBRS disabled via commandline or the kernel is
> > > > + * retpoline compiled or the HW supports enhanced IBRS
> > > > + */
> > > > + set_ibrs_enabled(0);    
> > >
> > > It took me a while to understand that IBRS Enhanced handling here (and
> > > then all the subtle details at runtime...). I now see that the IBRS bit
> > > of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
> > > Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
> > > the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
> > > ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
> > > the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
> > > ubuntu_restrict_branch_speculation_{start,end}() which is the correct
> > > behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
> > > is 0.  
> >
> > Hmm... The call set_ibrs_enabled(0) will clear the IBRS bit on all CPUs.
> > On hindsight, that seems wrong in the presence of enhanced IBRS. Maybe we
> > should disable the runtime control completely if mode ==
> > SPECTRE_V2_IBRS_ENHANCED?  
>
> The call set_ibrs_enabled(0) won't clear the IBRS bit because
> x86_spec_ctrl_base has the SPEC_CTRL_IBRS bit set when Enhanced IBRS is
> supported.
Oh, right. Convoluted... :-(


> However, one of the main benefits of Enhanced IBRS is that
> you write to the MSR once and then don't have to toggle it at all so
> set_ibrs_enabled(0) will be inefficient.
> >  
> > > > +int set_ibrs_enabled(unsigned int val)
> > > > +{
> > > > + int error = 0;
> > > > + unsigned int cpu;
> > > > +
> > > > + mutex_lock(&spec_ctrl_mutex);
> > > > +
> > > > + /* Only enable/disable IBRS if the CPU supports it */
> > > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > + ibrs_enabled = val;
> > > > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "    
> > >
> > > I think the "Spectre V2 : " portion of this message is redundant and is
> > > also not present in upstream's message which could confuse user or
> > > scripts that are looking at log messages. Can it be removed when
> > > applying the patch?  
> >
> > Again, this is how upstream reported it until just recently.
> >
> >
> >    
> > > > + "Branch Restricted Speculation%s\n",
> > > > + ibrs_enabled ? "Enabling" : "Disabling",
> > > > + ibrs_enabled == 2 ? " (user space)" : "");
> > > > +
> > > > + if (ibrs_enabled == 0) {
> > > > + /* Always disable IBRS */
> > > > + u64 val = x86_spec_ctrl_base;
> > > > +
> > > > + for_each_online_cpu(cpu) {
> > > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > + }
> > > > + } else if (ibrs_enabled == 2) {
> > > > + /* Always enable IBRS, even in user space */
> > > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> > > > +
> > > > + for_each_online_cpu(cpu) {
> > > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > + }
> > > > + }
> > > > + } else {
> > > > + ibrs_enabled = 0;
> > > > + if (val) {
> > > > + /* IBRS is not supported but we try to turn it on */
> > > > + error = -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + /* Update the shadow variable */
> > > > + __ibrs_enabled = ibrs_enabled;
> > > > +
> > > > + mutex_unlock(&spec_ctrl_mutex);
> > > > +
> > > > + return error;
> > > > +}
> > > > +
> > > > +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> > > > +                               void __user *buffer, size_t *lenp,
> > > > +                               loff_t *ppos)
> > > > +{
> > > > + int error;
> > > > +
> > > > + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > > > + if (error)
> > > > + return error;
> > > > +
> > > > + return set_ibrs_enabled(__ibrs_enabled);
> > > > +}
> > > >  #endif    
> > >
> > > IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
> > > already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
> > > personally don't feel like this is a problem but I wanted to mention it.  
> >
> > As mentioned above, with enhanced IBRS we can still fiddle with the IBRS bit
> > in the MSR via the runtime controls. We probably don't want that.  
>
> Now I'm thinking that it makes sense to be able to disable and enable
> Enhanced IBRS with the sysctl. Why wouldn't we want to support that?
Because it introduces another ubuntu-only-impossible-to-test-hard-to-maintain
feature with questionable benefit. Until there's a user request for it
I'm leaning towards disabling the runtime controls in Enhanced IBRS mode.

...Juerg


 
> Tyler


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

attachment0 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/CMNT: [SRU][Xenial][PATCH 2/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBRS runtime control handling

Tyler Hicks-2
On 2018-12-05 11:41:28, Juerg Haefliger wrote:

> On Tue, 4 Dec 2018 13:48:37 -0600
> Tyler Hicks <[hidden email]> wrote:
>
> > On 2018-12-03 14:17:49, Juerg Haefliger wrote:
> > > > > +#define ubuntu_restrict_branch_speculation_start() \
> > > > > +do { \
> > > > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS; \
> > > > > + \
> > > > > + if (ibrs_enabled) \
> > > > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > > > +} while (0)
> > > > > +
> > > > > +#define ubuntu_restrict_branch_speculation_end() \
> > > > > +do { \
> > > > > + u64 val = x86_spec_ctrl_base; \
> > > > > + \
> > > > > + if (ibrs_enabled) \
> > > > > + native_wrmsrl(MSR_IA32_SPEC_CTRL, val); \
> > > > > + } while (0)  
> > >
> > > I just realized that the above is incorrect. If ibrs_enabled == 2 then we set
> > > the IBRS bit on every CPU and should not fiddle with them here, no?
> > >
> > > So it should be:
> > >   if (ibrs_enabled == 1)
> > >       native_wrmsrl(MSR_IA32_SPEC_CTRL, val);  
> >
> > Yes, I think that's correct.
>
> After running some more tests I take my earlier statement back. Primarily
> because it would change the current behaviour. So I leave it as it is until
> somebody reports an issue.
>
>  
> > >
> > >  
> > > > >   /*
> > > > >   * Retpoline means the kernel is safe because it has no indirect
> > > > >   * branches. Enhanced IBRS protects firmware too, so, enable restricted
> > > > > @@ -463,9 +446,23 @@ specv2_set_mode:
> > > > >   * the CPU supports Enhanced IBRS, kernel might un-intentionally not
> > > > >   * enable IBRS around firmware calls.
> > > > >   */
> > > > > - if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > > - setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > > - pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > > + if (mode != SPECTRE_V2_IBRS_ENHANCED) {
> > > > > + setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> > > > > + pr_info("Enabling Restricted Speculation for firmware calls\n");
> > > > > + }
> > > > > + if (noibrs ||
> > > > > +    mode == SPECTRE_V2_RETPOLINE_GENERIC ||
> > > > > +    mode == SPECTRE_V2_RETPOLINE_AMD ||
> > > > > +    mode == SPECTRE_V2_IBRS_ENHANCED) {
> > > > > + /*
> > > > > + * IBRS disabled via commandline or the kernel is
> > > > > + * retpoline compiled or the HW supports enhanced IBRS
> > > > > + */
> > > > > + set_ibrs_enabled(0);    
> > > >
> > > > It took me a while to understand that IBRS Enhanced handling here (and
> > > > then all the subtle details at runtime...). I now see that the IBRS bit
> > > > of the IA32_SPEC_CTRL MSR is set earlier in this function if IBRS
> > > > Enhanced is supported. Additionally, the SPEC_CTRL_IBRS bit is set in
> > > > the x86_spec_ctrl_base bitmask. The above call to set_ibrs_enabled(0)
> > > > ensures that ibrs_enabled is set to 0. That means that the IBRS bit of
> > > > the IA32_SPEC_CTRL MSR will not be twiddled in future calls to
> > > > ubuntu_restrict_branch_speculation_{start,end}() which is the correct
> > > > behavior for IBRS Enhanced but it is a bit confusing since ibrs_enabled
> > > > is 0.  
> > >
> > > Hmm... The call set_ibrs_enabled(0) will clear the IBRS bit on all CPUs.
> > > On hindsight, that seems wrong in the presence of enhanced IBRS. Maybe we
> > > should disable the runtime control completely if mode ==
> > > SPECTRE_V2_IBRS_ENHANCED?  
> >
> > The call set_ibrs_enabled(0) won't clear the IBRS bit because
> > x86_spec_ctrl_base has the SPEC_CTRL_IBRS bit set when Enhanced IBRS is
> > supported.
>
> Oh, right. Convoluted... :-(
>
>
> > However, one of the main benefits of Enhanced IBRS is that
> > you write to the MSR once and then don't have to toggle it at all so
> > set_ibrs_enabled(0) will be inefficient.
> > >  
> > > > > +int set_ibrs_enabled(unsigned int val)
> > > > > +{
> > > > > + int error = 0;
> > > > > + unsigned int cpu;
> > > > > +
> > > > > + mutex_lock(&spec_ctrl_mutex);
> > > > > +
> > > > > + /* Only enable/disable IBRS if the CPU supports it */
> > > > > + if (boot_cpu_has(X86_FEATURE_IBRS)) {
> > > > > + ibrs_enabled = val;
> > > > > + pr_info("Spectre V2 : Spectre v2 mitigation: %s Indirect "    
> > > >
> > > > I think the "Spectre V2 : " portion of this message is redundant and is
> > > > also not present in upstream's message which could confuse user or
> > > > scripts that are looking at log messages. Can it be removed when
> > > > applying the patch?  
> > >
> > > Again, this is how upstream reported it until just recently.
> > >
> > >
> > >    
> > > > > + "Branch Restricted Speculation%s\n",
> > > > > + ibrs_enabled ? "Enabling" : "Disabling",
> > > > > + ibrs_enabled == 2 ? " (user space)" : "");
> > > > > +
> > > > > + if (ibrs_enabled == 0) {
> > > > > + /* Always disable IBRS */
> > > > > + u64 val = x86_spec_ctrl_base;
> > > > > +
> > > > > + for_each_online_cpu(cpu) {
> > > > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > > + }
> > > > > + } else if (ibrs_enabled == 2) {
> > > > > + /* Always enable IBRS, even in user space */
> > > > > + u64 val = x86_spec_ctrl_base | SPEC_CTRL_IBRS;
> > > > > +
> > > > > + for_each_online_cpu(cpu) {
> > > > > + wrmsrl_on_cpu(cpu, MSR_IA32_SPEC_CTRL, val);
> > > > > + }
> > > > > + }
> > > > > + } else {
> > > > > + ibrs_enabled = 0;
> > > > > + if (val) {
> > > > > + /* IBRS is not supported but we try to turn it on */
> > > > > + error = -EINVAL;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + /* Update the shadow variable */
> > > > > + __ibrs_enabled = ibrs_enabled;
> > > > > +
> > > > > + mutex_unlock(&spec_ctrl_mutex);
> > > > > +
> > > > > + return error;
> > > > > +}
> > > > > +
> > > > > +static int ibrs_enabled_handler(struct ctl_table *table, int write,
> > > > > +                               void __user *buffer, size_t *lenp,
> > > > > +                               loff_t *ppos)
> > > > > +{
> > > > > + int error;
> > > > > +
> > > > > + error = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > > > > + if (error)
> > > > > + return error;
> > > > > +
> > > > > + return set_ibrs_enabled(__ibrs_enabled);
> > > > > +}
> > > > >  #endif    
> > > >
> > > > IBRS Enhanced cannot be disabled with this sysctl since ibrs_enabled is
> > > > already 0 and the corresponding bit is set in x86_spec_ctrl_base. I
> > > > personally don't feel like this is a problem but I wanted to mention it.  
> > >
> > > As mentioned above, with enhanced IBRS we can still fiddle with the IBRS bit
> > > in the MSR via the runtime controls. We probably don't want that.  
> >
> > Now I'm thinking that it makes sense to be able to disable and enable
> > Enhanced IBRS with the sysctl. Why wouldn't we want to support that?
>
> Because it introduces another ubuntu-only-impossible-to-test-hard-to-maintain
> feature with questionable benefit. Until there's a user request for it
> I'm leaning towards disabling the runtime controls in Enhanced IBRS mode.

I'm alright with that.

Tyler

>
> ...Juerg
>
>
>  
> > Tyler
>



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team