[RFC PATCH] x86/split_lock: Disable SLD if an unaware (out-of-tree) module enables VMX

Sean Christopherson
2020-04-03T16:30:16Z

Hook into native CR4 writes to disable split-lock detection if CR4.VMXE
is toggled on by an SDL-unaware entity, e.g. an out-of-tree hypervisor
module. Most/all VMX-based hypervisors blindly reflect #AC exceptions
into the guest, or don't intercept #AC in the first place. With SLD
enabled, this results in unexpected #AC faults in the guest, leading to
crashes in the guest and other undesirable behavior.

Reported-by: "Kenneth R. Crudup" <kenny@panix.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Kenneth R. Crudup <kenny@panix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Nadav Amit <namit@vmware.com>
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Doug Covelli <dcovelli@vmware.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

A bit ugly, but on the plus side the code is largely contained to intel.c.
I think forgoing the on_all_cpus() remote kill is safe? The reverse is
definitely not safe as native_cr4_write() is called with IRQs disabled.
There's the weird discrepancy where SLD is still reported as supported and
enabled, but I don't think anything in the kernel will notice/care?

If we wanted, this could also make the "kill" temporary, e.g. undo it if
VMX is disabled. But, that would require per-CPU tracking of VMXE, and
going that far out of our way to support out-of-tree hypervisors seems a
bit much.

The big unknown is whether or not existing out-of-tree modules play nice
and actually route through native_cr4_write().

arch/x86/include/asm/cpu.h | 18 ++++++++++++++++++
arch/x86/kernel/cpu/common.c | 1 +
arch/x86/kernel/cpu/intel.c | 31 ++++++++++++++++++++++++++++++-
3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index ff6f3ca649b3..73c5f6478fb5 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -8,6 +8,8 @@
#include <linux/nodemask.h>
#include <linux/percpu.h>

+#include <asm/tlbflush.h>
+
#ifdef CONFIG_SMP

extern void prefill_possible_map(void);
@@ -44,6 +46,16 @@ unsigned int x86_stepping(unsigned int sig);
extern void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
extern void switch_to_sld(unsigned long tifn);
extern bool handle_user_split_lock(struct pt_regs *regs, long error_code);
+extern void split_lock_kill(void);
+
+extern atomic_t cr4_vmxe_split_lock_safe;
+static inline void cr4_set_vmxe(void)
+{
+ atomic_inc(&cr4_vmxe_split_lock_safe);
+ cr4_set_bits(X86_CR4_VMXE);
+ atomic_dec(&cr4_vmxe_split_lock_safe);
+}
+extern void split_lock_cr4_write(unsigned long val);
#else
static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
static inline void switch_to_sld(unsigned long tifn) {}
@@ -51,5 +63,11 @@ static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
{
return false;
}
+static inline void split_lock_kill(void) {}
+static inline void cr4_set_vmxe(void)
+{
+ cr4_set_bits(X86_CR4_VMXE);
+}
+static inline void split_lock_cr4_write(unsigned long val) {}
#endif
#endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index bed0cb83fe24..a50dda8d4409 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -373,6 +373,7 @@ void native_write_cr4(unsigned long val)
{
unsigned long bits_missing = 0;

+ split_lock_cr4_write(val);
set_register:
asm volatile("mov %0,%%cr4": "+r" (val), "+m" (cr4_pinned_bits));

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 9a26e972cdea..9cfe29d68457 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -46,6 +46,11 @@ enum split_lock_detect_state {
*/
static enum split_lock_detect_state sld_state __ro_after_init = sld_off;
static u64 msr_test_ctrl_cache __ro_after_init;
+static bool sld_killed __read_mostly;
+
+/* Cookie that states CR4.VMXE can be enabled without killing SLD. */
+atomic_t cr4_vmxe_split_lock_safe = ATOMIC_INIT(0);
+EXPORT_SYMBOL_GPL(cr4_vmxe_split_lock_safe);

/*
* Processors which have self-snooping capability can handle conflicting
@@ -1055,7 +1060,7 @@ static void sld_update_msr(bool on)
{
u64 test_ctrl_val = msr_test_ctrl_cache;

- if (on)
+ if (on && !sld_killed)
test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;

wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
@@ -1096,6 +1101,30 @@ void switch_to_sld(unsigned long tifn)
sld_update_msr(!(tifn & _TIF_SLD));
}

+void split_lock_cr4_write(unsigned long val)
+{
+ /*
+ * Out-of-tree hypervisors that aren't aware of split-lock will blindly
+ * reflect split-lock #AC into their guests. Kill split-lock detection
+ * if an unaware entity enables VMX.
+ */
+ if (!static_cpu_has(X86_FEATURE_VMX) ||
+ !static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) ||
+ !(val & X86_CR4_VMXE) || atomic_read(&cr4_vmxe_split_lock_safe) ||
+ (native_read_cr4() & X86_CR4_VMXE))
+ return;
+
+ WARN_ON_ONCE(1);
+
+ /*
+ * No need to forcefully disable SLD on other CPUs, they'll come here
+ * if/when they set CR4.VMXE. Set the global kill bit to prevent
+ * re-enabling SLD, e.g. via switch_to_sld().
+ */
+ sld_killed = true;
+ sld_update_msr(false);
+}
+
#define SPLIT_LOCK_CPU(model) {X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY}

/*
--
2.24.1