Message ID | 20191119031240.7779-5-sean.j.christopherson@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | x86/cpu: Clean up handling of VMX features | expand |
On Mon, 2019-11-18 at 19:12 -0800, Sean Christopherson wrote: > Opportunistically initialize IA32_FEATURE_CONTROL MSR to enable VMX when > the MSR is left unlocked by BIOS. Configuring IA32_FEATURE_CONTROL at > boot time paves the way for similar enabling of other features, e.g. > Software Guard Extensions (SGX). > > Temporarily leave equivalent KVM code in place in order to avoid > introducing a regression on Centaur and Zhaoxin CPUs, e.g. removing > KVM's code would leave the MSR unlocked on those CPUs and would break > existing functionality if people are loading kvm_intel on Centaur and/or > Zhaoxin. Defer enablement of the boot-time configuration on Centaur and > Zhaoxin to future patches to aid bisection. > > Note, Local Machine Check Exceptions (LMCE) are also supported by the > kernel and enabled via IA32_FEATURE_CONTROL, but the kernel currently > uses LMCE if and and only if the feature is explicitly enabled by BIOS. > Keep the current behavior to avoid introducing bugs, future patches can > opt in to opportunistic enabling if it's deemed desirable to do so. > > Always lock IA32_FEATURE_CONTROL if it exists, even if the CPU doesn't > support VMX, so that other existing and future kernel code that queries > IA32_FEATURE_CONTROL can assume it's locked. > > Start from a clean slate when constructing the value to write to > IA32_FEATURE_CONTROL, i.e. ignore whatever value BIOS left in the MSR so > as not to enable random features or fault on the WRMSR. > > Suggested-by: Borislav Petkov <bp@suse.de> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/Kconfig.cpu | 4 +++ > arch/x86/kernel/cpu/Makefile | 1 + > arch/x86/kernel/cpu/cpu.h | 4 +++ > arch/x86/kernel/cpu/feature_control.c | 35 +++++++++++++++++++++++++++ > arch/x86/kernel/cpu/intel.c | 2 ++ > 5 files changed, 46 insertions(+) > create mode 100644 arch/x86/kernel/cpu/feature_control.c > > diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu > index af9c967782f6..aafc14a0abf7 100644 > --- a/arch/x86/Kconfig.cpu > +++ b/arch/x86/Kconfig.cpu > @@ -387,6 +387,10 @@ config X86_DEBUGCTLMSR > def_bool y > depends on !(MK6 || MWINCHIPC6 || MWINCHIP3D || MCYRIXIII || M586MMX || > M586TSC || M586 || M486SX || M486) && !UML > > +config X86_FEATURE_CONTROL_MSR > + def_bool y > + depends on CPU_SUP_INTEL > + > menuconfig PROCESSOR_SELECT > bool "Supported processor vendors" if EXPERT > ---help--- > diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile > index 890f60083eca..84e35e762f76 100644 > --- a/arch/x86/kernel/cpu/Makefile > +++ b/arch/x86/kernel/cpu/Makefile > @@ -29,6 +29,7 @@ obj-y += umwait.o > obj-$(CONFIG_PROC_FS) += proc.o > obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o > > +obj-$(CONFIG_X86_FEATURE_CONTROL_MSR) += feature_control.o > ifdef CONFIG_CPU_SUP_INTEL > obj-y += intel.o intel_pconfig.o tsx.o > obj-$(CONFIG_PM) += intel_epb.o > diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h > index 38ab6e115eac..a58e80866a3f 100644 > --- a/arch/x86/kernel/cpu/cpu.h > +++ b/arch/x86/kernel/cpu/cpu.h > @@ -80,4 +80,8 @@ extern void x86_spec_ctrl_setup_ap(void); > > extern u64 x86_read_arch_cap_msr(void); > > +#ifdef CONFIG_X86_FEATURE_CONTROL_MSR > +void init_feature_control_msr(struct cpuinfo_x86 *c); > +#endif > + > #endif /* ARCH_X86_CPU_H */ > diff --git a/arch/x86/kernel/cpu/feature_control.c > b/arch/x86/kernel/cpu/feature_control.c > new file mode 100644 > index 000000000000..33c9444dda52 > --- /dev/null > +++ b/arch/x86/kernel/cpu/feature_control.c > @@ -0,0 +1,35 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/tboot.h> > + > +#include <asm/cpufeature.h> > +#include <asm/msr-index.h> > +#include <asm/processor.h> > + > +void init_feature_control_msr(struct cpuinfo_x86 *c) > +{ > + u64 msr; > + > + if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) > + return; > + > + if (msr & FEAT_CTL_LOCKED) > + return; > + > + /* > + * Ignore whatever value BIOS left in the MSR to avoid enabling random > + * features or faulting on the WRMSR. > + */ > + msr = FEAT_CTL_LOCKED; > + > + /* > + * Enable VMX if and only if the kernel may do VMXON at some point, > + * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector > + * for the kernel, e.g. using VMX to hide malicious code. > + */ > + if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM)) { > + msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX; > + if (tboot_enabled()) > + msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX; > + } Why not also take this chance to enable SGX? Or it will come with SGX patch series? > + wrmsrl(MSR_IA32_FEATURE_CONTROL, msr); > +} > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index 4a900804a023..b7c6ed0b40b6 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -755,6 +755,8 @@ static void init_intel(struct cpuinfo_x86 *c) > /* Work around errata */ > srat_detect_node(c); > > + init_feature_control_msr(c); Will this compile if you disable CONFIG_X86_FEATURE_CONTROL_MSR? Provide an empty one in cpu.h if the config is not enabled? Thanks, -Kai > + > if (cpu_has(c, X86_FEATURE_VMX)) > detect_vmx_virtcap(c); >
On Tue, Nov 19, 2019 at 05:41:49PM +1300, Kai Huang wrote: > On Mon, 2019-11-18 at 19:12 -0800, Sean Christopherson wrote: > > + /* > > + * Enable VMX if and only if the kernel may do VMXON at some point, > > + * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector > > + * for the kernel, e.g. using VMX to hide malicious code. > > + */ > > + if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM)) { Hmm, this should more specifically be CONFIG_KVM_INTEL. > > + msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX; > > + if (tboot_enabled()) > > + msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX; > > + } > > Why not also take this chance to enable SGX? Or it will come with SGX patch > series? The latter. Similar to the KVM check, this shouldn't opt in to SGX unless the kernel is capable of using SGX. > > + wrmsrl(MSR_IA32_FEATURE_CONTROL, msr); > > +} > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > > index 4a900804a023..b7c6ed0b40b6 100644 > > --- a/arch/x86/kernel/cpu/intel.c > > +++ b/arch/x86/kernel/cpu/intel.c > > @@ -755,6 +755,8 @@ static void init_intel(struct cpuinfo_x86 *c) > > /* Work around errata */ > > srat_detect_node(c); > > > > + init_feature_control_msr(c); > > Will this compile if you disable CONFIG_X86_FEATURE_CONTROL_MSR? > > Provide an empty one in cpu.h if the config is not enabled? CONFIG_X86_FEATURE_CONTROL_MSR can't be disabled manually, it's selected by CPU_SUP_INTEL (and by Zhaoxin/Centaur for their relevant patches).
On Mon, Nov 18, 2019 at 07:12:25PM -0800, Sean Christopherson wrote: > Opportunistically initialize IA32_FEATURE_CONTROL MSR to enable VMX when > the MSR is left unlocked by BIOS. Configuring IA32_FEATURE_CONTROL at > boot time paves the way for similar enabling of other features, e.g. > Software Guard Extensions (SGX). > > Temporarily leave equivalent KVM code in place in order to avoid > introducing a regression on Centaur and Zhaoxin CPUs, e.g. removing > KVM's code would leave the MSR unlocked on those CPUs and would break > existing functionality if people are loading kvm_intel on Centaur and/or > Zhaoxin. Defer enablement of the boot-time configuration on Centaur and > Zhaoxin to future patches to aid bisection. > > Note, Local Machine Check Exceptions (LMCE) are also supported by the > kernel and enabled via IA32_FEATURE_CONTROL, but the kernel currently > uses LMCE if and and only if the feature is explicitly enabled by BIOS. > Keep the current behavior to avoid introducing bugs, future patches can > opt in to opportunistic enabling if it's deemed desirable to do so. > > Always lock IA32_FEATURE_CONTROL if it exists, even if the CPU doesn't > support VMX, so that other existing and future kernel code that queries > IA32_FEATURE_CONTROL can assume it's locked. > > Start from a clean slate when constructing the value to write to > IA32_FEATURE_CONTROL, i.e. ignore whatever value BIOS left in the MSR so > as not to enable random features or fault on the WRMSR. > > Suggested-by: Borislav Petkov <bp@suse.de> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/Kconfig.cpu | 4 +++ > arch/x86/kernel/cpu/Makefile | 1 + > arch/x86/kernel/cpu/cpu.h | 4 +++ > arch/x86/kernel/cpu/feature_control.c | 35 +++++++++++++++++++++++++++ > arch/x86/kernel/cpu/intel.c | 2 ++ > 5 files changed, 46 insertions(+) > create mode 100644 arch/x86/kernel/cpu/feature_control.c > > diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu > index af9c967782f6..aafc14a0abf7 100644 > --- a/arch/x86/Kconfig.cpu > +++ b/arch/x86/Kconfig.cpu > @@ -387,6 +387,10 @@ config X86_DEBUGCTLMSR > def_bool y > depends on !(MK6 || MWINCHIPC6 || MWINCHIP3D || MCYRIXIII || M586MMX || M586TSC || M586 || M486SX || M486) && !UML > > +config X86_FEATURE_CONTROL_MSR > + def_bool y > + depends on CPU_SUP_INTEL > + > menuconfig PROCESSOR_SELECT > bool "Supported processor vendors" if EXPERT > ---help--- > diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile > index 890f60083eca..84e35e762f76 100644 > --- a/arch/x86/kernel/cpu/Makefile > +++ b/arch/x86/kernel/cpu/Makefile > @@ -29,6 +29,7 @@ obj-y += umwait.o > obj-$(CONFIG_PROC_FS) += proc.o > obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o > > +obj-$(CONFIG_X86_FEATURE_CONTROL_MSR) += feature_control.o > ifdef CONFIG_CPU_SUP_INTEL > obj-y += intel.o intel_pconfig.o tsx.o > obj-$(CONFIG_PM) += intel_epb.o > diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h > index 38ab6e115eac..a58e80866a3f 100644 > --- a/arch/x86/kernel/cpu/cpu.h > +++ b/arch/x86/kernel/cpu/cpu.h > @@ -80,4 +80,8 @@ extern void x86_spec_ctrl_setup_ap(void); > > extern u64 x86_read_arch_cap_msr(void); > > +#ifdef CONFIG_X86_FEATURE_CONTROL_MSR > +void init_feature_control_msr(struct cpuinfo_x86 *c); > +#endif > + > #endif /* ARCH_X86_CPU_H */ > diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c > new file mode 100644 > index 000000000000..33c9444dda52 > --- /dev/null > +++ b/arch/x86/kernel/cpu/feature_control.c > @@ -0,0 +1,35 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/tboot.h> > + > +#include <asm/cpufeature.h> > +#include <asm/msr-index.h> > +#include <asm/processor.h> > + > +void init_feature_control_msr(struct cpuinfo_x86 *c) > +{ > + u64 msr; > + > + if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) > + return; > + > + if (msr & FEAT_CTL_LOCKED) > + return; > + > + /* > + * Ignore whatever value BIOS left in the MSR to avoid enabling random > + * features or faulting on the WRMSR. > + */ > + msr = FEAT_CTL_LOCKED; > + > + /* > + * Enable VMX if and only if the kernel may do VMXON at some point, > + * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector > + * for the kernel, e.g. using VMX to hide malicious code. > + */ > + if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM)) { > + msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX; Add empty line here. > + if (tboot_enabled()) > + msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX; > + } Add empty line here. > + wrmsrl(MSR_IA32_FEATURE_CONTROL, msr); > +} > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index 4a900804a023..b7c6ed0b40b6 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -755,6 +755,8 @@ static void init_intel(struct cpuinfo_x86 *c) > /* Work around errata */ > srat_detect_node(c); > > + init_feature_control_msr(c); > + ... similarly as you do here and earlier when first initializing 'msr'. > if (cpu_has(c, X86_FEATURE_VMX)) > detect_vmx_virtcap(c); > > -- > 2.24.0 > /Jarkko
On Thu, Nov 21, 2019 at 12:39:25PM +0200, Jarkko Sakkinen wrote:
> > +void init_feature_control_msr(struct cpuinfo_x86 *c)
I'd also use here shorter init_feat_ctl_msr(). It has one call site
but shorter name is more convenient when playing with tracing tools.
/Jarkko
On Thu, Nov 21, 2019 at 12:41:45PM +0200, Jarkko Sakkinen wrote: > On Thu, Nov 21, 2019 at 12:39:25PM +0200, Jarkko Sakkinen wrote: > > > +void init_feature_control_msr(struct cpuinfo_x86 *c) > > I'd also use here shorter init_feat_ctl_msr(). It has one call site > but shorter name is more convenient when playing with tracing tools. Yeah, and since we're shortening all to feat_ctl, let's do: mv arch/x86/kernel/cpu/feature_control.c arch/x86/kernel/cpu/feat_ctl.c too. Thx.
On Thu, Nov 21, 2019 at 12:05:33PM +0100, Borislav Petkov wrote: > On Thu, Nov 21, 2019 at 12:41:45PM +0200, Jarkko Sakkinen wrote: > > On Thu, Nov 21, 2019 at 12:39:25PM +0200, Jarkko Sakkinen wrote: > > > > +void init_feature_control_msr(struct cpuinfo_x86 *c) > > > > I'd also use here shorter init_feat_ctl_msr(). It has one call site > > but shorter name is more convenient when playing with tracing tools. > > Yeah, and since we're shortening all to feat_ctl, let's do: > > mv arch/x86/kernel/cpu/feature_control.c arch/x86/kernel/cpu/feat_ctl.c Any objection to keeping the MSR name as MSR_IA32_FEATURE_CONTOL? I'd like to have some anchor back to the name used in the SDM. Any opinions/thoughts on the name of the Kconfig? Currently it's X86_FEATURE_CONTROL_MSR, which gets a bit long with CONFIG_ on the front. I also overlooked that we have MSR_MISC_FEATURE_CONTROL, so having IA32 in the Kconfig would probably be a good idea. X86_IA32 is rather redundant, so maybe IA32_FEAT_CTL or IA32_FEATURE_CONTROL?
On Thu, Nov 21, 2019 at 02:12:34PM -0800, Sean Christopherson wrote: > Any objection to keeping the MSR name as MSR_IA32_FEATURE_CONTOL? I'd like > to have some anchor back to the name used in the SDM. > > Any opinions/thoughts on the name of the Kconfig? Currently it's > X86_FEATURE_CONTROL_MSR, which gets a bit long with CONFIG_ on the front. > I also overlooked that we have MSR_MISC_FEATURE_CONTROL, so having IA32 in > the Kconfig would probably be a good idea. X86_IA32 is rather redundant, > so maybe IA32_FEAT_CTL or IA32_FEATURE_CONTROL? Well, what I'd do is since we have MSR_MISC_FEATURE_CONTROL too, I'd call all code and defines pertaining to the 0x3a MSR <bla>_IA32_FEAT_CTL I.e., CONFIG_IA32_FEAT_CTL, MSR_IA32_FEAT_CTL, ... and leave a comment over the MSR definition containing the SDM name. This way, you have a clear distinction between the IA32 and the MISC feature control. But this is just me and I realize we're pretty much deep inside the bike shed. :)
diff --git a/arch/x86/Kconfig.cpu b/arch/x86/Kconfig.cpu index af9c967782f6..aafc14a0abf7 100644 --- a/arch/x86/Kconfig.cpu +++ b/arch/x86/Kconfig.cpu @@ -387,6 +387,10 @@ config X86_DEBUGCTLMSR def_bool y depends on !(MK6 || MWINCHIPC6 || MWINCHIP3D || MCYRIXIII || M586MMX || M586TSC || M586 || M486SX || M486) && !UML +config X86_FEATURE_CONTROL_MSR + def_bool y + depends on CPU_SUP_INTEL + menuconfig PROCESSOR_SELECT bool "Supported processor vendors" if EXPERT ---help--- diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile index 890f60083eca..84e35e762f76 100644 --- a/arch/x86/kernel/cpu/Makefile +++ b/arch/x86/kernel/cpu/Makefile @@ -29,6 +29,7 @@ obj-y += umwait.o obj-$(CONFIG_PROC_FS) += proc.o obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o +obj-$(CONFIG_X86_FEATURE_CONTROL_MSR) += feature_control.o ifdef CONFIG_CPU_SUP_INTEL obj-y += intel.o intel_pconfig.o tsx.o obj-$(CONFIG_PM) += intel_epb.o diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h index 38ab6e115eac..a58e80866a3f 100644 --- a/arch/x86/kernel/cpu/cpu.h +++ b/arch/x86/kernel/cpu/cpu.h @@ -80,4 +80,8 @@ extern void x86_spec_ctrl_setup_ap(void); extern u64 x86_read_arch_cap_msr(void); +#ifdef CONFIG_X86_FEATURE_CONTROL_MSR +void init_feature_control_msr(struct cpuinfo_x86 *c); +#endif + #endif /* ARCH_X86_CPU_H */ diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c new file mode 100644 index 000000000000..33c9444dda52 --- /dev/null +++ b/arch/x86/kernel/cpu/feature_control.c @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/tboot.h> + +#include <asm/cpufeature.h> +#include <asm/msr-index.h> +#include <asm/processor.h> + +void init_feature_control_msr(struct cpuinfo_x86 *c) +{ + u64 msr; + + if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) + return; + + if (msr & FEAT_CTL_LOCKED) + return; + + /* + * Ignore whatever value BIOS left in the MSR to avoid enabling random + * features or faulting on the WRMSR. + */ + msr = FEAT_CTL_LOCKED; + + /* + * Enable VMX if and only if the kernel may do VMXON at some point, + * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector + * for the kernel, e.g. using VMX to hide malicious code. + */ + if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM)) { + msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX; + if (tboot_enabled()) + msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX; + } + wrmsrl(MSR_IA32_FEATURE_CONTROL, msr); +} diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 4a900804a023..b7c6ed0b40b6 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -755,6 +755,8 @@ static void init_intel(struct cpuinfo_x86 *c) /* Work around errata */ srat_detect_node(c); + init_feature_control_msr(c); + if (cpu_has(c, X86_FEATURE_VMX)) detect_vmx_virtcap(c);
Opportunistically initialize IA32_FEATURE_CONTROL MSR to enable VMX when the MSR is left unlocked by BIOS. Configuring IA32_FEATURE_CONTROL at boot time paves the way for similar enabling of other features, e.g. Software Guard Extensions (SGX). Temporarily leave equivalent KVM code in place in order to avoid introducing a regression on Centaur and Zhaoxin CPUs, e.g. removing KVM's code would leave the MSR unlocked on those CPUs and would break existing functionality if people are loading kvm_intel on Centaur and/or Zhaoxin. Defer enablement of the boot-time configuration on Centaur and Zhaoxin to future patches to aid bisection. Note, Local Machine Check Exceptions (LMCE) are also supported by the kernel and enabled via IA32_FEATURE_CONTROL, but the kernel currently uses LMCE if and and only if the feature is explicitly enabled by BIOS. Keep the current behavior to avoid introducing bugs, future patches can opt in to opportunistic enabling if it's deemed desirable to do so. Always lock IA32_FEATURE_CONTROL if it exists, even if the CPU doesn't support VMX, so that other existing and future kernel code that queries IA32_FEATURE_CONTROL can assume it's locked. Start from a clean slate when constructing the value to write to IA32_FEATURE_CONTROL, i.e. ignore whatever value BIOS left in the MSR so as not to enable random features or fault on the WRMSR. Suggested-by: Borislav Petkov <bp@suse.de> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/Kconfig.cpu | 4 +++ arch/x86/kernel/cpu/Makefile | 1 + arch/x86/kernel/cpu/cpu.h | 4 +++ arch/x86/kernel/cpu/feature_control.c | 35 +++++++++++++++++++++++++++ arch/x86/kernel/cpu/intel.c | 2 ++ 5 files changed, 46 insertions(+) create mode 100644 arch/x86/kernel/cpu/feature_control.c