Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758645AbeAIA37 (ORCPT + 1 other); Mon, 8 Jan 2018 19:29:59 -0500 Received: from mail.skyhub.de ([5.9.137.197]:38864 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753703AbeAIA3z (ORCPT ); Mon, 8 Jan 2018 19:29:55 -0500 Date: Tue, 9 Jan 2018 01:29:43 +0100 From: Borislav Petkov To: Tim Chen Cc: Thomas Gleixner , Andy Lutomirski , Linus Torvalds , Greg KH , Dave Hansen , Andrea Arcangeli , Andi Kleen , Arjan Van De Ven , David Woodhouse , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature Message-ID: <20180109002943.vdcjeubkflnccmup@pd.tnic> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote: > From: Tim Chen > From: Andrea Arcangeli There's Co-Developed-by for this: - Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a Signed-off-by: line in the patch as well. > There are 2 ways to control IBRS > > 1. At boot time > noibrs kernel boot parameter will disable IBRS usage > > Otherwise if the above parameters are not specified, the system > will enable ibrs and ibpb usage if the cpu supports it. s/cpu/CPU/ > > 2. At run time > echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS > echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel > echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel > > The implementation was updated with input and suggestions from Andrea Arcangeli. > > Signed-off-by: Tim Chen > --- > arch/x86/entry/calling.h | 42 ++++++++-- > arch/x86/include/asm/spec_ctrl.h | 15 ++++ > arch/x86/kernel/cpu/Makefile | 1 + > arch/x86/kernel/cpu/scattered.c | 2 + > arch/x86/kernel/cpu/spec_ctrl.c | 160 +++++++++++++++++++++++++++++++++++++++ > 5 files changed, 214 insertions(+), 6 deletions(-) > create mode 100644 arch/x86/include/asm/spec_ctrl.h > create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c > > diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h > index 09c870d..6b65d47 100644 > --- a/arch/x86/entry/calling.h > +++ b/arch/x86/entry/calling.h > @@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel is built with > .endm > > .macro ENABLE_IBRS > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > PUSH_MSR_REGS > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS > POP_MSR_REGS > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm > > .macro DISABLE_IBRS > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > PUSH_MSR_REGS > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS > POP_MSR_REGS > + > .Lskip_\@: > .endm > > .macro ENABLE_IBRS_CLOBBER > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm > > .macro DISABLE_IBRS_CLOBBER > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS > + > .Lskip_\@: > .endm > > .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > movl $MSR_IA32_SPEC_CTRL, %ecx > rdmsr > movl %eax, \save_reg > @@ -409,15 +429,25 @@ For 32-bit we have the following conventions - kernel is built with > movl $0, %edx > movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax > wrmsr > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm > > .macro RESTORE_IBRS_CLOBBER save_reg:req > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > /* Set IBRS to the value saved in the save_reg */ > movl $MSR_IA32_SPEC_CTRL, %ecx > movl $0, %edx > movl \save_reg, %eax > wrmsr > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm Why not put all macros in spec_ctrl.h ? > diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h > new file mode 100644 > index 0000000..4fda38b > --- /dev/null > +++ b/arch/x86/include/asm/spec_ctrl.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _ASM_X86_SPEC_CTRL_H > +#define _ASM_X86_SPEC_CTRL_H > + > +#include > +#include > +#include Left over include from a previous version. Instead, you need #include in spec_ctrl.c for get/put_online_cpus(). > +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c); > +bool ibrs_inuse(void); > + > +extern unsigned int dynamic_ibrs; > + > +#endif /* _ASM_X86_SPEC_CTRL_H */ > diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile > index 570e8bb..3ffbd24 100644 > --- a/arch/x86/kernel/cpu/Makefile > +++ b/arch/x86/kernel/cpu/Makefile > @@ -24,6 +24,7 @@ obj-y += match.o > obj-y += bugs.o > obj-y += aperfmperf.o > obj-y += cpuid-deps.o > +obj-y += spec_ctrl.o > > obj-$(CONFIG_PROC_FS) += proc.o > obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o > diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c > index bc50c40..5756d14 100644 > --- a/arch/x86/kernel/cpu/scattered.c > +++ b/arch/x86/kernel/cpu/scattered.c > @@ -8,6 +8,7 @@ > #include > > #include > +#include > > struct cpuid_bit { > u16 feature; > @@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c) > if (regs[cb->reg] & (1 << cb->bit)) > set_cpu_cap(c, cb->feature); > } > + scan_spec_ctrl_feature(c); Hell no! This function is only for setting the feature bits. > u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf, > diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c > new file mode 100644 > index 0000000..1641bec > --- /dev/null > +++ b/arch/x86/kernel/cpu/spec_ctrl.c > @@ -0,0 +1,160 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include > +#include > +#include > + > +#include > +#include > + > +unsigned int dynamic_ibrs __read_mostly; WTH is dynamic_ibrs? > +EXPORT_SYMBOL_GPL(dynamic_ibrs); > + > +enum { > + IBRS_DISABLED, > + /* in host kernel, disabled in guest and userland */ > + IBRS_ENABLED, > + /* in host kernel and host userland, disabled in guest */ > + IBRS_ENABLED_USER, > + IBRS_MAX = IBRS_ENABLED_USER, > +}; > +static unsigned int ibrs_enabled; > +static bool ibrs_admin_disabled; Srsly?! That's *three* variables controlling IBRS. This needs simplification. > + > +/* mutex to serialize IBRS control changes */ > +DEFINE_MUTEX(spec_ctrl_mutex); static > +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c) > +{ > + if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) { What is !c->cpu_index? Checking whether this is the BSP? What for? > + if (!ibrs_admin_disabled) { > + dynamic_ibrs = 1; > + ibrs_enabled = IBRS_ENABLED; > + } > + } > +} > +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature); > + > +/* > + * Used after boot phase to rescan spec_ctrl feature, > + * serialize scan with spec_ctrl_mutex. > + */ > +void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c) > +{ > + mutex_lock(&spec_ctrl_mutex); > + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) { > + if (!ibrs_admin_disabled) { > + dynamic_ibrs = 1; > + ibrs_enabled = IBRS_ENABLED; > + } > + } > + mutex_unlock(&spec_ctrl_mutex); > +} > +EXPORT_SYMBOL_GPL(rescan_spec_ctrl_feature); > + > +bool ibrs_inuse(void) > +{ > + return ibrs_enabled == IBRS_ENABLED; > +} So from looking at this, you can have a single ibrs_state variable and set bits in it and have *all* possible settings you wanna have. > +EXPORT_SYMBOL_GPL(ibrs_inuse); That naming is not really correct: otherwise IBRS_ENABLED_USER means ibrs is *not* in use. > + > +static int __init noibrs(char *str) > +{ > + ibrs_admin_disabled = true; > + ibrs_enabled = IBRS_DISABLED; > + > + return 0; > +} > +early_param("noibrs", noibrs); > + > +static ssize_t __enabled_read(struct file *file, char __user *user_buf, > + size_t count, loff_t *ppos, unsigned int *field) > +{ > + char buf[32]; > + unsigned int len; > + > + len = sprintf(buf, "%d\n", READ_ONCE(*field)); > + return simple_read_from_buffer(user_buf, count, ppos, buf, len); > +} > + > +static ssize_t ibrs_enabled_read(struct file *file, char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + return __enabled_read(file, user_buf, count, ppos, &ibrs_enabled); > +} > + > +static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val) > +{ > + int cpu; > + > + get_online_cpus(); > + for_each_online_cpu(cpu) > + wrmsrl_on_cpu(cpu, msr_nr, val); > + put_online_cpus(); > +} > + > +static ssize_t ibrs_enabled_write(struct file *file, > + const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + char buf[32]; > + ssize_t len; > + unsigned int enable; > + > + len = min(count, sizeof(buf) - 1); > + if (copy_from_user(buf, user_buf, len)) > + return -EFAULT; > + > + buf[len] = '\0'; > + if (kstrtouint(buf, 0, &enable)) > + return -EINVAL; > + > + if (enable > IBRS_MAX) > + return -EINVAL; > + > + if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) { That capability check needs to happen first: no need to do anything if the CPU doesn't have IBRS. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.