diff mbox series

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

Message ID 20181121135831.25405-3-juergh@canonical.com
State New
Headers show
Series [SRU,Xenial,1/3] UBUNTU: SAUCE: x86/speculation: Cleanup IBPB runtime control handling | expand

Commit Message

Juerg Haefliger Nov. 21, 2018, 1:58 p.m. UTC
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 <juergh@canonical.com>
---
 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(-)

Comments

Tyler Hicks Nov. 30, 2018, 7:58 p.m. UTC | #1
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 <juergh@canonical.com>

Acked-by: Tyler Hicks <tyhicks@canonical.com>

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
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Juerg Haefliger Dec. 3, 2018, 1:17 p.m. UTC | #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
Tyler Hicks Dec. 4, 2018, 7:48 p.m. UTC | #3
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
Juerg Haefliger Dec. 5, 2018, 10:41 a.m. UTC | #4
On Tue, 4 Dec 2018 13:48:37 -0600
Tyler Hicks <tyhicks@canonical.com> 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
Tyler Hicks Dec. 5, 2018, 3:06 p.m. UTC | #5
On 2018-12-05 11:41:28, Juerg Haefliger wrote:
> On Tue, 4 Dec 2018 13:48:37 -0600
> Tyler Hicks <tyhicks@canonical.com> 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
>
diff mbox series

Patch

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