x86: Prevent Split-Lock-Detection wreckage on VMX hypervisors

I picked VMXOFF (which also appears in vmmon.ko) instead of VMXON because that latter takes an argument is therefore more difficult to decode.

---
Subject: x86,module: Detect VMX modules and disable Split-Lock-Detect
From: Peter Zijlstra <[email protected]>
Date: Thu, 02 Apr 2020 14:32:59 +0200

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH/VMXOFF instructions. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

Hypervisors, which have been modified and are known to work correctly, can add:

   MODULE_INFO(sld_safe, "Y");

to explicitly tell the module loader they're good.

NOTE: it is unfortunate that struct load_info is not available to the
arch module code, this means CONFIG_CPU_SUP_INTEL gunk is needed in generic code.

NOTE: while we can 'trivially' fix KVM, we're still stuck with stuff
like VMware and VirtualBox doing their own thing.

Reported-by "Kenneth R. Crudup" <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
  arch/x86/include/asm/cpu.h  |    2 ++
  arch/x86/kernel/cpu/intel.c |   41 ++++++++++++++++++++++++++++++++++++++++-
  arch/x86/kernel/module.c    |    6 ++++++
  include/linux/module.h      |    4 ++++
  kernel/module.c             |    5 +++++
  5 files changed, 57 insertions(+), 1 deletion(-)

arch/x86/include/asm/cpu.h +2 -0

@@ -44,6 +44,7 @@ unsigned int x86_stepping(unsigned int s
  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_validate_module_text(struct module *me, void *text, void *text_end);
  #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 +52,6 @@ static inline bool handle_user_split_loc
  {
  	return false;
  }
+static inline void split_lock_validate_module_text(struct module *me, void *text, void *text_end) {}
  #endif
  #endif /* _ASM_X86_CPU_H */

arch/x86/kernel/cpu/intel.c +27 -1

@@ -9,6 +9,7 @@
  #include <linux/thread_info.h>
  #include <linux/init.h>
  #include <linux/uaccess.h>
+#include <linux/module.h>
 
  #include <asm/cpufeature.h>
  #include <asm/pgtable.h>

@@ -21,6 +22,7 @@
  #include <asm/elf.h>
  #include <asm/cpu_device_id.h>
  #include <asm/cmdline.h>
+#include <asm/insn.h>
 
  #ifdef CONFIG_X86_64
  #include <linux/topology.h>

@@ -1055,12 +1057,49 @@ static void sld_update_msr(bool on)
  {
  	u64 test_ctrl_val = msr_test_ctrl_cache;
 
-	if (on)
+	if (on && (sld_state != sld_off))
  		test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
 
  	wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
  }
 
+static void sld_remote_kill(void *arg)
+{
+	sld_update_msr(false);
+}
+
+void split_lock_validate_module_text(struct module *me, void *text, void *text_end)
+{
+	u8 vmxoff[] = { 0x0f, 0x01, 0xc4 };
+	u8 vmlaunch[] = { 0x0f, 0x01, 0xc2 };
+	struct insn insn;
+
+	if (sld_state == sld_off)
+		return;
+
+	while (text < text_end) {
+		kernel_insn_init(&insn, text, text_end - text);
+		insn_get_length(&insn);
+
+		if (WARN_ON_ONCE(!insn_complete(&insn)))
+			break;
+
+		if (insn.length == 3 &&
+		    (!memcmp(text, vmlaunch, sizeof(vmlaunch)) ||
+		     !memcmp(text, vmxoff, sizeof(vmxoff))))

arch/x86/kernel/cpu/intel.c +3 -0

+	on_each_cpu(sld_remote_kill, NULL, 1);
+}
+
  static void split_lock_init(void)
  {
  	split_lock_verify_msr(sld_state != sld_off);

arch/x86/kernel/module.c +6 -0

@@ -24,6 +24,7 @@
  #include <asm/pgtable.h>
  #include <asm/setup.h>
  #include <asm/unwind.h>
+#include <asm/cpu.h>
 
  #if 0
  #define DEBUGP(fmt, ...)				\

@@ -253,6 +254,11 @@ int module_finalize(const Elf_Ehdr *hdr,
  					    tseg, tseg + text->sh_size);
  	}
 
+	if (text && !me->sld_safe) {
+		void *tseg = (void *)text->sh_addr;
+		split_lock_validate_module_text(me, tseg, tseg + text->sh_size);
+	}
+
  	if (para) {
  		void *pseg = (void *)para->sh_addr;
  		apply_paravirt(pseg, pseg + para->sh_size);

include/linux/module.h +4 -0

@@ -407,6 +407,10 @@ struct module {
  	bool sig_ok;
  #endif
 
+#ifdef CONFIG_CPU_SUP_INTEL
+	bool sld_safe;
+#endif
+
  	bool async_probe_requested;
 
  	/* symbols that will be GPL-only in the near future. */

kernel/module.c +5 -0

@@ -3096,6 +3096,11 @@ static int check_modinfo(struct module *
  			"is unknown, you have been warned.\n", mod->name);
  	}
 
+#ifdef CONFIG_CPU_SUP_INTEL
+	if (get_modinfo(info, "sld_safe"))
+		mod->sld_safe = true;
+#endif
+
  	err = check_modinfo_livepatch(mod, info);
  	if (err)
  		return err;

arch/x86/kernel/cpu/intel.c static void sld_update_msr(bool on) · line 1099

Quoted 3 lines · Peter Zijlstra

arch/x86/kernel/cpu/intel.c +3 -0

+bad_module:
+	pr_warn("disabled due to VMX in module: %s\n", me->name);
+	sld_state = sld_off;

shouldn't we remove the __ro_after_init of sld_state?

And, shouldn't we clear X86_FEATURE_SPLIT_LOCK_DETECT flag?

Learn to trim your replies already!

Quoted 1 line · Xiaoyao Li

On 4/2/2020 11:23 PM, Peter Zijlstra wrote:

Quoted 5 lines

arch/x86/kernel/cpu/intel.c static void sld_update_msr(bool on) · line 1099

Quoted 3 lines

arch/x86/kernel/cpu/intel.c +3 -0

+bad_module:
+	pr_warn("disabled due to VMX in module: %s\n", me->name);
+	sld_state = sld_off;

shouldn't we remove the __ro_after_init of sld_state?

Oh, that's probably a good idea. I can't actually test this due to no hardware.

Learn to trim your replies already!

Quoted 7 lines · Peter Zijlstra

arch/x86/kernel/cpu/intel.c static void sld_update_msr(bool on) · line 1099

Quoted 4 lines

arch/x86/kernel/cpu/intel.c +3 -0

+bad_module:
+	pr_warn("disabled due to VMX in module: %s\n", me->name);
+	sld_state = sld_off;

shouldn't we remove the __ro_after_init of sld_state?

Oh, that's probably a good idea. I can't actually test this due to no hardware.

Quoted 1 line

And, shouldn't we clear X86_FEATURE_SPLIT_LOCK_DETECT flag?

Don't think you can do that this late. Also, the hardware has the MSR and it works, it's just that we should not.

Quoted 6 lines · Peter Zijlstra

Quoted 1 line · Xiaoyao Li

And, shouldn't we clear X86_FEATURE_SPLIT_LOCK_DETECT flag?

Don't think you can do that this late. Also, the hardware has the MSR and it works, it's just that we should not.

Actually, I agree to keep this flag.

But, during the previous patch review, tglx wants to make

sld_off = no X86_FEATURE_SPLIT_LOCK_DETECT

I'm not sure whether he still insists on it now.

I really want to decouple sld_off and X86_FEATURE_SPLIT_LOCK_DETECT. So if X86_FEATURE_SPLIT_LOCK_DETECT is set, we can virtualize and expose it to guest even when host is sld_off.

Aside of that I'm still against the attempt of proliferating crap, i.e. disabling it because the host is triggering it and then exposing it to guests. The above does not change my mind in any way. This proposal is still wrong.

Quoted 4 lines · Thomas Gleixner

Aside of that I'm still against the attempt of proliferating crap, i.e. disabling it because the host is triggering it and then exposing it to guests. The above does not change my mind in any way. This proposal is still wrong.

Eh, I still think the "off in host, on in guest" is a legit scenario for debug/development/testing, but I agree that the added complexity doesn't justify the minimal benefits versus sld_warn.

Quoted 6 lines · Sean Christopherson

Quoted 4 lines · Thomas Gleixner

Aside of that I'm still against the attempt of proliferating crap, i.e. disabling it because the host is triggering it and then exposing it to guests. The above does not change my mind in any way. This proposal is still wrong.

Quoted 3 lines

Eh, I still think the "off in host, on in guest" is a legit scenario for debug/development/testing, but I agree that the added complexity doesn't justify the minimal benefits versus sld_warn.

Off in host on in guest seems utterly insane to me. Why do you care about that?

Quoted 7 lines · Peter Zijlstra

Quoted 6 lines · Sean Christopherson

Quoted 4 lines · Thomas Gleixner

Aside of that I'm still against the attempt of proliferating crap, i.e. disabling it because the host is triggering it and then exposing it to guests. The above does not change my mind in any way. This proposal is still wrong.

Quoted 6 lines

Quoted 3 lines

Eh, I still think the "off in host, on in guest" is a legit scenario for debug/development/testing, but I agree that the added complexity doesn't justify the minimal benefits versus sld_warn.

Off in host on in guest seems utterly insane to me. Why do you care about that?

For development/debug/testing. Ignoring the core-scope stupidity of split lock, the _functional_ behavior of the host kernel and guest kernel are completely separate. The host can generate split locks all it wants, but other than performance, its bad behavior has no impact on the guest.

For example, all of the debug that was done to eliminate split locks in the kernel could have been done in a KVM guest, even though the host kernel would not have yet been split-lock free.

It's somewhat of a moot point now that the kernel is split-lock free. But, if I encountered a split lock panic on my system, the first thing I would do (after rebooting) would be to fire up a VM to try and reproduce and debug the issue.

Oftentimes it's significantly easier to "enable" a feature in KVM, i.e. expose a feature to the guest, than it is to actually enable it in the kernel. Enabling KVM first doesn't work if there are hard dependencies on kernel enabling, e.g. most things that have an XSAVE component, but for a lot of features it's a viable strategy to enable KVM first, and then do all testing and debug inside a KVM guest.

Quoted 29 lines · Sean Christopherson

Quoted 7 lines · Peter Zijlstra

Quoted 6 lines · Sean Christopherson

Quoted 4 lines · Thomas Gleixner

Aside of that I'm still against the attempt of proliferating crap, i.e. disabling it because the host is triggering it and then exposing it to guests. The above does not change my mind in any way. This proposal is still wrong.

Quoted 6 lines

Quoted 3 lines

Eh, I still think the "off in host, on in guest" is a legit scenario for debug/development/testing, but I agree that the added complexity doesn't justify the minimal benefits versus sld_warn.

Off in host on in guest seems utterly insane to me. Why do you care about that?

For development/debug/testing. Ignoring the core-scope stupidity of split lock, the _functional_ behavior of the host kernel and guest kernel are completely separate. The host can generate split locks all it wants, but other than performance, its bad behavior has no impact on the guest.

For example, all of the debug that was done to eliminate split locks in the kernel could have been done in a KVM guest, even though the host kernel would not have yet been split-lock free.

It's somewhat of a moot point now that the kernel is split-lock free. But, if I encountered a split lock panic on my system, the first thing I would do (after rebooting) would be to fire up a VM to try and reproduce and debug the issue.

Quoted 6 lines

Oftentimes it's significantly easier to "enable" a feature in KVM, i.e. expose a feature to the guest, than it is to actually enable it in the kernel. Enabling KVM first doesn't work if there are hard dependencies on kernel enabling, e.g. most things that have an XSAVE component, but for a lot of features it's a viable strategy to enable KVM first, and then do all testing and debug inside a KVM guest.

I can see that aspect, but there were pretty clear messages in one of the other threads:

"It's not about whether or not host is clean. It's for the cases that
users just don't want it enabled on host, to not break the applications or drivers that do have split lock issue."

"My thought is for CSPs that they might not turn on SLD on their
product environment. Any split lock in kernel or drivers may break their service for tenants."

which I back then called out as proliferating crap and ensuring that this stuff never gets fixed.

I still call it out as exactly that and you know as well as I do that this is the reality.

For people like you who actually want to debug stuff in a guest, the extra 10 lines of hack on top of the other 1000 lines of hacks you already have are not really something which justifies to give hardware and OS/application vendors the easy way out to avoid fixing their broken crap.

Sent: 02 April 2020 16:24

I picked VMXOFF (which also appears in vmmon.ko) instead of VMXON because that latter takes an argument is therefore more difficult to decode.

arch/x86/kernel/cpu/intel.c +4 -0

+	while (text < text_end) {
+		kernel_insn_init(&insn, text, text_end - text);
+		insn_get_length(&insn);
+

From: Peter Zijlstra

Quoted 5 lines

Sent: 02 April 2020 16:24

I picked VMXOFF (which also appears in vmmon.ko) instead of VMXON because that latter takes an argument is therefore more difficult to decode.

...

Quoted 4 lines

arch/x86/kernel/cpu/intel.c +4 -0

+	while (text < text_end) {
+		kernel_insn_init(&insn, text, text_end - text);
+		insn_get_length(&insn);
+

arch/x86/kernel/cpu/intel.c static void sld_update_msr(bool on) · line 1093

Quoted 10 lines

arch/x86/kernel/cpu/intel.c +10 -0

+		if (WARN_ON_ONCE(!insn_complete(&insn)))
+			break;
+
+		if (insn.length == 3 &&
+		    (!memcmp(text, vmlaunch, sizeof(vmlaunch)) ||
+		     !memcmp(text, vmxoff, sizeof(vmxoff))))
+				goto bad_module;
+
+		text += insn.length;
+	}

How long is that going to take on a module with (say) 400k of text?

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

Quoted 6 lines

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Quoted 3 lines

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

How long does that scan take/add to module load time? Would it make sense to exempt in-tree modules?

Rasmus

+++ Rasmus Villemoes [03/04/20 01:42 +0200]:

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Quoted 8 lines

Quoted 3 lines

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

How long does that scan take/add to module load time? Would it make sense to exempt in-tree modules?

Rasmus

I second Rasmus's question. It seems rather unfortunate that we have to do this text scan for every module load on x86, when it doesn't apply to the majority of them, and only to a handful of out-of-tree hypervisor modules (assuming kvm is taken care of already).

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

Quoted 22 lines · Jessica Yu

+++ Rasmus Villemoes [03/04/20 01:42 +0200]:

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Quoted 8 lines

Quoted 3 lines

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

How long does that scan take/add to module load time? Would it make sense to exempt in-tree modules?

Rasmus

I second Rasmus's question. It seems rather unfortunate that we have to do this text scan for every module load on x86, when it doesn't apply to the majority of them, and only to a handful of out-of-tree hypervisor modules (assuming kvm is taken care of already).

Quoted 2 lines

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

Sent: 03 April 2020 17:12

Quoted 11 lines · Sean Christopherson

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

And then you're hoping that the module uses that and not:

asm volatile ("mov %0, cr4" :: "r" (val));

I think I feel safer with the scanning to be fair. Also with the intree hint on, we can extend the scanning for out-of-tree modules for more dodgy crap we really don't want modules to do, like for example the above.

From: Peter Zijlstra

Quoted 18 lines

Sent: 03 April 2020 17:12

Quoted 11 lines · Sean Christopherson

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

And then you're hoping that the module uses that and not:

asm volatile ("mov %0, cr4" :: "r" (val));

Quoted 4 lines

I think I feel safer with the scanning to be fair. Also with the intree hint on, we can extend the scanning for out-of-tree modules for more dodgy crap we really don't want modules to do, like for example the above.

Could you do the scanning in the last phase of the module build that has to be done against the target kernel headers and with the target kernel build infrastructure?

Quoted 11 lines · Sean Christopherson

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

And then you're hoping that the module uses that and not:

asm volatile ("mov %0, cr4" :: "r" (val));

I think I feel safer with the scanning to be fair. Also with the intree hint on, we can extend the scanning for out-of-tree modules for more dodgy crap we really don't want modules to do, like for example the above.

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Sean Christopherson

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

Quoted 8 lines

And then you're hoping that the module uses that and not:

asm volatile ("mov %0, cr4" :: "r" (val));

I think I feel safer with the scanning to be fair. Also with the intree hint on, we can extend the scanning for out-of-tree modules for more dodgy crap we really don't want modules to do, like for example the above.

Ya, that's the big uknown. But wouldn't they'd already be broken in the sense that they'd corrupt the CR4 shadow? E.g. setting VMXE without updating cpu_tlbstate.cr4 would result in future in-kernel writes to CR4 attempting to clear CR4.VMXE post-VMXON, which would #GP.

Quoted 23 lines · Sean Christopherson

Quoted 21 lines · Peter Zijlstra

Quoted 11 lines · Sean Christopherson

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

And then you're hoping that the module uses that and not:

asm volatile ("mov %0, cr4" :: "r" (val));

I think I feel safer with the scanning to be fair. Also with the intree hint on, we can extend the scanning for out-of-tree modules for more dodgy crap we really don't want modules to do, like for example the above.

Quoted 4 lines

Ya, that's the big uknown. But wouldn't they'd already be broken in the sense that they'd corrupt the CR4 shadow? E.g. setting VMXE without updating cpu_tlbstate.cr4 would result in future in-kernel writes to CR4 attempting to clear CR4.VMXE post-VMXON, which would #GP.

Sadly the CR4 shadow is exported, so they can actually fix that up :/

Quoted 26 lines · Peter Zijlstra

Quoted 23 lines · Sean Christopherson

Quoted 21 lines · Peter Zijlstra

Quoted 11 lines · Sean Christopherson

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Jessica Yu

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Rather than scanning modules at all, what about hooking native_write_cr4() to kill SLD if CR4.VMXE is toggled on and the caller didn't increment a "sld safe" counter?

And then you're hoping that the module uses that and not:

asm volatile ("mov %0, cr4" :: "r" (val));

I think I feel safer with the scanning to be fair. Also with the intree hint on, we can extend the scanning for out-of-tree modules for more dodgy crap we really don't want modules to do, like for example the above.

Quoted 6 lines

Quoted 4 lines

Ya, that's the big uknown. But wouldn't they'd already be broken in the sense that they'd corrupt the CR4 shadow? E.g. setting VMXE without updating cpu_tlbstate.cr4 would result in future in-kernel writes to CR4 attempting to clear CR4.VMXE post-VMXON, which would #GP.

Sadly the CR4 shadow is exported, so they can actually fix that up :/

I do not think that Sean’s idea would work for VMware.

Quoted 22 lines · Jessica Yu

+++ Rasmus Villemoes [03/04/20 01:42 +0200]:

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Quoted 8 lines

Quoted 3 lines

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

How long does that scan take/add to module load time? Would it make sense to exempt in-tree modules?

Rasmus

I second Rasmus's question. It seems rather unfortunate that we have to do this text scan for every module load on x86, when it doesn't apply to the majority of them, and only to a handful of out-of-tree hypervisor modules (assuming kvm is taken care of already).

Quoted 2 lines

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

Quoted 23 lines · Peter Zijlstra

Quoted 23 lines · Jessica Yu

+++ Rasmus Villemoes [03/04/20 01:42 +0200]:

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Quoted 8 lines

Quoted 3 lines

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

How long does that scan take/add to module load time? Would it make sense to exempt in-tree modules?

Rasmus

I second Rasmus's question. It seems rather unfortunate that we have to do this text scan for every module load on x86, when it doesn't apply to the majority of them, and only to a handful of out-of-tree hypervisor modules (assuming kvm is taken care of already).

Quoted 4 lines

Quoted 2 lines

I wonder if it would make sense then to limit the text scans to just out-of-tree modules (i.e., missing the intree modinfo flag)?

It would; didn't know there was one.

But that still would not make it complete.

I was staring at virtualbox today after Jann pointed out that this sucker does complete backwards things.

The kernel driver does not contain any VM* instructions at all.

The actual hypervisor code is built as a separate binary and somehow loaded into the kernel with their own magic fixup of relocations and function linking. This "design" probably comes from the original virtualbox implementation which circumvented GPL that way.

TBH, I don't care if we wreckage virtualbox simply because that thing is already a complete and utter trainwreck violating taste and common sense in any possible way. Just for illustration:

- It installs preempt notifiers and the first thing in the callback function is to issue 'stac()'!

- There is quite some other horrible code in there which fiddles in the guts of the kernel just because it can.

- Conditionals in release code which check stuff like VBOX_WITH_TEXT_MODMEM_HACK, VBOX_WITH_EFLAGS_AC_SET_IN_VBOXDRV, VBOX_WITH_NON_PROD_HACK_FOR_PERF_STACKS along with the most absurd hacks ever.

If you feel the need to look yourself, please use your eyecancer protection gear.

Can someone at Oracle please make sure, that this monstrosity gets shred in pieces?

Enough vented, but that still does not solve the SLD problem in any sensible way.

Thanks,

tglx

Quoted 4 lines · Thomas Gleixner

Peter Zijlstra <[email protected]> writes:
Enough vented, but that still does not solve the SLD problem in any sensible way.

Could we unexport set_memory_x perhaps? And maybe try to make virtualbox break in as many ways as possible?

arch/x86/kernel/module.c +1 -0

@@ -24,6 +24,7 @@
 #include <asm/pgtable.h>
 #include <asm/setup.h>
 #include <asm/unwind.h>
+#include <asm/cpu.h>
 
 #if 0
 #define DEBUGP(fmt, ...)				\

arch/x86/kernel/module.c +4 -0

+		void *tseg = (void *)text->sh_addr;
+		split_lock_validate_module_text(me, tseg, tseg + text->sh_size);
+	}
+
 	if (para) {
 		void *pseg = (void *)para->sh_addr;
 		apply_paravirt(pseg, pseg + para->sh_size);

include/linux/module.h +4 -0

@@ -407,6 +407,10 @@ struct module {
 	bool sig_ok;
 #endif
 
+#ifdef CONFIG_CPU_SUP_INTEL
+	bool sld_safe;
+#endif
+
 	bool async_probe_requested;
 
 	/* symbols that will be GPL-only in the near future. */

kernel/module.c +5 -0

@@ -3096,6 +3096,11 @@ static int check_modinfo(struct module *
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
+#ifdef CONFIG_CPU_SUP_INTEL
+	if (get_modinfo(info, "sld_safe"))
+		mod->sld_safe = true;
+#endif
+
 	err = check_modinfo_livepatch(mod, info);
 	if (err)
 		return err;

Quoted 11 lines · Thomas Gleixner

arch/x86/kernel/module.c +1 -0

@@ -24,6 +24,7 @@
 #include <asm/pgtable.h>
 #include <asm/setup.h>
 #include <asm/unwind.h>
+#include <asm/cpu.h>
 
 #if 0
 #define DEBUGP(fmt, ...)				\

arch/x86/kernel/module.c int module_finalize(const Elf_Ehdr *hdr, · line 257

Quoted 4 lines

arch/x86/kernel/module.c +1 -0

@@ -253,6 +254,11 @@ int module_finalize(const Elf_Ehdr *hdr,
 					    tseg, tseg + text->sh_size);
 	}
 
+	if (text && !me->sld_safe) {

As also reported by the test bot, sld_safe only exist if CPU_SUP_INTEL=y.

This can also be conditioned on boot_cpu_has(X86_FEATURE_VMX), or the static variant. If CPU_SUP_INTEL=y, X86_FEATURE_VMX will be set if and only if VMX is fully enabled, i.e. supported by the CPU and enabled in MSR_IA32_FEATURE_CONTROl.

arch/x86/kernel/module.c int module_finalize(const Elf_Ehdr *hdr, · line 260

Quoted 3 lines

arch/x86/kernel/module.c +3 -0

+		void *tseg = (void *)text->sh_addr;
+		split_lock_validate_module_text(me, tseg, tseg + text->sh_size);
+	}

Quoted 24 lines · Thomas Gleixner

Quoted 11 lines

arch/x86/kernel/module.c +1 -0

@@ -24,6 +24,7 @@
 #include <asm/pgtable.h>
 #include <asm/setup.h>
 #include <asm/unwind.h>
+#include <asm/cpu.h>
 
 #if 0
 #define DEBUGP(fmt, ...)				\

arch/x86/kernel/module.c int module_finalize(const Elf_Ehdr *hdr, · line 257

Quoted 4 lines

arch/x86/kernel/module.c +1 -0

@@ -253,6 +254,11 @@ int module_finalize(const Elf_Ehdr *hdr,
 					    tseg, tseg + text->sh_size);
 	}
 
+	if (text && !me->sld_safe) {

As also reported by the test bot, sld_safe only exist if CPU_SUP_INTEL=y.

This can also be conditioned on boot_cpu_has(X86_FEATURE_VMX), or the static variant. If CPU_SUP_INTEL=y, X86_FEATURE_VMX will be set if and only if VMX is fully enabled, i.e. supported by the CPU and enabled in MSR_IA32_FEATURE_CONTROl.

Quoted 3 lines

arch/x86/kernel/module.c int module_finalize(const Elf_Ehdr *hdr, · line 260

Quoted 3 lines

arch/x86/kernel/module.c +3 -0

+		void *tseg = (void *)text->sh_addr;
+		split_lock_validate_module_text(me, tseg, tseg + text->sh_size);
+	}

Ideally we push it all into arch code, but load_info isn't exposed to arch module code :/.

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

Hypervisors, which have been modified and are known to work correctly, can add:

  MODULE_INFO(sld_safe, "Y");

to explicitly tell the module loader they're good.

NOTE: it is unfortunate that struct load_info is not available to the
arch module code, this means CONFIG_CPU_SUP_INTEL gunk is needed in generic code.

NOTE: while we can 'trivially' fix KVM, we're still stuck with stuff
like VMware and VirtualBox doing their own thing.

Quoted 21 lines · Thomas Gleixner

From: Peter Zijlstra <[email protected]>

It turns out that with Split-Lock-Detect enabled (default) any VMX hypervisor needs at least a little modification in order to not blindly inject the #AC into the guest without the guest being ready for it.

Since there is no telling which module implements a hypervisor, scan the module text and look for the VMLAUNCH instruction. If found, the module is assumed to be a hypervisor of some sort and SLD is disabled.

Hypervisors, which have been modified and are known to work correctly, can add:

  MODULE_INFO(sld_safe, "Y");

to explicitly tell the module loader they're good.

NOTE: it is unfortunate that struct load_info is not available to the
arch module code, this means CONFIG_CPU_SUP_INTEL gunk is needed in generic code.

Quoted 2 lines

NOTE: while we can 'trivially' fix KVM, we're still stuck with stuff
like VMware and VirtualBox doing their own thing.

This is just crazy. We have never cared about any out tree module, why would we care here where it creates a real complexity. Just fix KVM and ignore anything else.

It is absolutely bonkers, but at the same time we can extend this infrastructure to scan for dubious code patterns we don't want to support. Like for instance direct manipulation of CR4.

Quoted 3 lines · Peter Zijlstra

It is absolutely bonkers, but at the same time we can extend this infrastructure to scan for dubious code patterns we don't want to support. Like for instance direct manipulation of CR4.

But that is not what this code does - it disables split lock detection. If it failed to load the module the whole thing would make a little more sense.

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Peter Zijlstra

It is absolutely bonkers, but at the same time we can extend this infrastructure to scan for dubious code patterns we don't want to support. Like for instance direct manipulation of CR4.

Quoted 3 lines

But that is not what this code does - it disables split lock detection. If it failed to load the module the whole thing would make a little more sense.

If this lives, it'll be to just to fail module loading. IIRC the same was suggested elsewhere in the thread.

Without at least minimal handling for split lock detection induced #AC, VMX will just run into the same problem as the VMWare hypervisor, which was reported by Kenneth.

It will inject the #AC blindly into the guest whether the guest is prepared or not.

Add the minimal required handling for it:

- Check guest state whether CR0.AM is enabled and EFLAGS.AC is set. If so, then the #AC originated from CPL3 and the guest has is prepared to handle it. In this case it does not matter whether the #AC is due to a split lock or a regular unaligned check.

- Invoke a minimal split lock detection handler. If the host SLD mode is sld_warn, then handle it in the same way as user space handling works:
Emit a warning, disable SLD and mark the current task with TIF_SLD. With that resume the guest without injecting #AC.

If the host mode is sld_fatal or sld_off, emit a warning and deliver the exception to user space which can crash and burn itself.

Mark the module with MOD_INFO(sld_safe, "Y") so the module loader does not force SLD off.

Signed-off-by Thomas Gleixner <[email protected]>

Cc "Kenneth R. Crudup" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Fenghua Yu <[email protected]>

Cc Xiaoyao Li <[email protected]>

Cc Nadav Amit <[email protected]>

Cc Thomas Hellstrom <[email protected]>

Cc Sean Christopherson <[email protected]>

Cc Tony Luck <[email protected]>

---
 arch/x86/include/asm/cpu.h  |    1 +
 arch/x86/kernel/cpu/intel.c |   28 +++++++++++++++++++++++-----
 arch/x86/kvm/vmx/vmx.c      |   40 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 61 insertions(+), 8 deletions(-)

arch/x86/include/asm/cpu.h +1 -0

@@ -44,6 +44,7 @@ unsigned int x86_stepping(unsigned int s
 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 bool handle_guest_split_lock(unsigned long ip);
 extern void split_lock_validate_module_text(struct module *me, void *text, void *text_end);
 #else
 static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}

arch/x86/kernel/cpu/intel.c +23 -5

@@ -1102,13 +1102,10 @@ static void split_lock_init(void)
 	split_lock_verify_msr(sld_state != sld_off);
 }
 
-bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+static void split_lock_warn(unsigned long ip)
 {
-	if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
-		return false;
-
 	pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n",
-			    current->comm, current->pid, regs->ip);
+			    current->comm, current->pid, ip);
 
 	/*
 	 * Disable the split lock detection for this task so it can make

@@ -1117,6 +1114,27 @@ bool handle_user_split_lock(struct pt_re
 	 */
 	sld_update_msr(false);
 	set_tsk_thread_flag(current, TIF_SLD);
+}
+
+bool handle_guest_split_lock(unsigned long ip)
+{
+	if (sld_state == sld_warn) {
+		split_lock_warn(ip);
+		return true;
+	}
+
+	pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n",
+		     current->comm, current->pid,
+		     sld_state == sld_fatal ? "fatal" : "bogus", ip);
+	return false;
+}
+EXPORT_SYMBOL_GPL(handle_guest_split_lock);
+
+bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+{
+	if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
+		return false;
+	split_lock_warn(regs->ip);
 	return true;
 }
 

arch/x86/kvm/vmx/vmx.c +1 -0

@@ -65,6 +65,7 @@
 
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
+MODULE_INFO(sld_safe, "Y");
 
 #ifdef MODULE
 static const struct x86_cpu_id vmx_cpu_id[] = {

@@ -4623,6 +4624,22 @@ static int handle_machine_check(struct k
 	return 1;
 }

arch/x86/kvm/vmx/vmx.c +26 -3

+		return true;
+
+	/* Add guest SLD handling checks here once it's supported */
+	return false;
+}
+
 static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

@@ -4688,9 +4705,6 @@ static int handle_exception_nmi(struct k
 		return handle_rmode_exception(vcpu, ex_no, error_code);
 
 	switch (ex_no) {
-	case AC_VECTOR:
-		kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
-		return 1;
 	case DB_VECTOR:
 		dr6 = vmcs_readl(EXIT_QUALIFICATION);
 		if (!(vcpu->guest_debug &

@@ -4719,6 +4733,26 @@ static int handle_exception_nmi(struct k
 		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
 		kvm_run->debug.arch.exception = ex_no;
 		break;
+	case AC_VECTOR:
+		if (guest_handles_ac(vcpu)) {
+			kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
+			return 1;
+		}
+		/*
+		 * Handle #AC caused by split lock detection. If the host
+		 * mode is sld_warn, then it warns, marks current with
+		 * TIF_SLD and disables split lock detection. So the guest
+		 * can just continue.
+		 *
+		 * If the host mode is fatal, the handling code warned. Let
+		 * qemu kill itself.
+		 *
+		 * If the host mode is off, then this #AC is bonkers and
+		 * something is badly wrong. Let it fail as well.
+		 */
+		if (handle_guest_split_lock(kvm_rip_read(vcpu)))
+			return 1;
+		/* fall through */
 	default:
 		kvm_run->exit_reason = KVM_EXIT_EXCEPTION;
 		kvm_run->ex.exception = ex_no;

Quoted 22 lines · Thomas Gleixner

Without at least minimal handling for split lock detection induced #AC, VMX will just run into the same problem as the VMWare hypervisor, which was reported by Kenneth.

It will inject the #AC blindly into the guest whether the guest is prepared or not.

Add the minimal required handling for it:

- Check guest state whether CR0.AM is enabled and EFLAGS.AC is set. If so, then the #AC originated from CPL3 and the guest has is prepared to handle it. In this case it does not matter whether the #AC is due to a split lock or a regular unaligned check.

- Invoke a minimal split lock detection handler. If the host SLD mode is sld_warn, then handle it in the same way as user space handling works:
Emit a warning, disable SLD and mark the current task with TIF_SLD. With that resume the guest without injecting #AC.

If the host mode is sld_fatal or sld_off, emit a warning and deliver the exception to user space which can crash and burn itself.

Quoted 2 lines

Mark the module with MOD_INFO(sld_safe, "Y") so the module loader does not force SLD off.

Some comments below. But, any objection to taking Xiaoyao's patches that do effectively the same things, minus the MOD_INFO()? I'll repost them in reply to this thread.

Quoted 28 lines · Sean Christopherson

Quoted 24 lines · Thomas Gleixner

Without at least minimal handling for split lock detection induced #AC, VMX will just run into the same problem as the VMWare hypervisor, which was reported by Kenneth.

It will inject the #AC blindly into the guest whether the guest is prepared or not.

Add the minimal required handling for it:

- Check guest state whether CR0.AM is enabled and EFLAGS.AC is set. If so, then the #AC originated from CPL3 and the guest has is prepared to handle it. In this case it does not matter whether the #AC is due to a split lock or a regular unaligned check.

- Invoke a minimal split lock detection handler. If the host SLD mode is sld_warn, then handle it in the same way as user space handling works:
Emit a warning, disable SLD and mark the current task with TIF_SLD. With that resume the guest without injecting #AC.

If the host mode is sld_fatal or sld_off, emit a warning and deliver the exception to user space which can crash and burn itself.

Mark the module with MOD_INFO(sld_safe, "Y") so the module loader does not force SLD off.

Quoted 3 lines

Some comments below. But, any objection to taking Xiaoyao's patches that do effectively the same things, minus the MOD_INFO()? I'll repost them in reply to this thread.

IIUC they also deal with emulated split-lock accesses in the host, during instruction emulation [1]. This seems also to be required, although I am not sure the approach that he took once emulation encounters a split-lock is robust.

[1] https://lore.kernel.org/lkml/[email protected]/

Quoted 2 lines · Thomas Gleixner

Mark the module with MOD_INFO(sld_safe, "Y") so the module loader does not force SLD off.

Some comments below. But, any objection to taking Xiaoyao's patches that do effectively the same things, minus the MOD_INFO()? I'll repost them in reply to this thread.

Quoted 4 lines

arch/x86/kvm/vmx/vmx.c +4 -0

+static bool guest_handles_ac(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * If guest has alignment checking enabled in CR0 and activated in

arch/x86/kvm/vmx/vmx.c static int handle_machine_check(struct k · line 4635

Quoted 5 lines

arch/x86/kvm/vmx/vmx.c +5 -0

+	 * eflags, then the #AC originated from CPL3 and the guest is able
+	 * to handle it. It does not matter whether this is a regular or
+	 * a split lock operation induced #AC.
+	 */
+	if (vcpu->arch.cr0 & X86_CR0_AM &&

Technically not required since KVM doesn't let the gets toggle CR0.AM at will, but going through kvm_read_cr0{_bits}() is preferred.

arch/x86/kvm/vmx/vmx.c static int handle_machine_check(struct k · line 4636

Quoted 1 line

arch/x86/kvm/vmx/vmx.c +1 -0

+	    vmx_get_rflags(vcpu) & X86_EFLAGS_AC)

I don't think this is correct. A guest could trigger a split-lock #AC at CPL0 with EFLAGS.AC=1 and CR0.AM=1, and then panic because it didn't expect #AC at CPL0.

From: Xiaoyao Li <[email protected]>

In the future, KVM will use handle_user_split_lock() to handle #AC caused by split lock in guest. Due to the fact that KVM doesn't have a @regs context and will pre-check EFLAGS.AC, move the EFLAGS.AC check to do_alignment_check().

Suggested-by Sean Christopherson <[email protected]>

Signed-off-by Xiaoyao Li <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/include/asm/cpu.h  | 4 ++--
 arch/x86/kernel/cpu/intel.c | 7 ++++---
 arch/x86/kernel/traps.c     | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

arch/x86/include/asm/cpu.h +0 -0

index ff6f3ca649b3..ff567afa6ee1 100644

arch/x86/include/asm/cpu.h +0 -0

 {
 	return false;
 }

arch/x86/kernel/cpu/intel.c +0 -0

index 9a26e972cdea..7688f51aabdb 100644
@@ -1066,13 +1066,13 @@ static void split_lock_init(void)
 	split_lock_verify_msr(sld_state != sld_off);
 }
 

Quoted 26 lines · Sean Christopherson

From: Xiaoyao Li <[email protected]>

In the future, KVM will use handle_user_split_lock() to handle #AC caused by split lock in guest. Due to the fact that KVM doesn't have a @regs context and will pre-check EFLAGS.AC, move the EFLAGS.AC check to do_alignment_check().

Suggested-by Sean Christopherson <[email protected]>

Signed-off-by Xiaoyao Li <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/include/asm/cpu.h  | 4 ++--
 arch/x86/kernel/cpu/intel.c | 7 ++++---
 arch/x86/kernel/traps.c     | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

arch/x86/include/asm/cpu.h +0 -1

index ff6f3ca649b3..ff567afa6ee1 100644
@@ -43,11 +43,11 @@ unsigned int x86_stepping(unsigned int sig);
 #ifdef CONFIG_CPU_SUP_INTEL
 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);

arch/x86/include/asm/cpu.h unsigned int x86_stepping(unsigned int sig); · line 50

Quoted 6 lines

arch/x86/include/asm/cpu.h +2 -1

+extern bool handle_user_split_lock(unsigned long ip);
 #else
 static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
 static inline void switch_to_sld(unsigned long tifn) {}
-static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+static inline bool handle_user_split_lock(unsigned long ip)

This is necessary because VMX can be compiled without CPU_SUP_INTEL?

Quoted 6 lines · Thomas Gleixner

Review

Quoted 4 lines

static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {} static inline void switch_to_sld(unsigned long tifn) {}

(quoted diff) +1 -1

-static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+static inline bool handle_user_split_lock(unsigned long ip)

This is necessary because VMX can be compiled without CPU_SUP_INTEL?

Ya, it came about when cleaning up the IA32_FEATURE_CONTROL MSR handling to consolidate duplicate code.

config KVM_INTEL
tristate "KVM for Intel (and compatible) processors support" depends on KVM && IA32_FEAT_CTL

config IA32_FEAT_CTL

        def_bool y
        depends on CPU_SUP_INTEL || CPU_SUP_CENTAUR || CPU_SUP_ZHAOXIN

Quoted 15 lines · Sean Christopherson

Quoted 6 lines · Thomas Gleixner

Review

Quoted 4 lines

static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {} static inline void switch_to_sld(unsigned long tifn) {}

(quoted diff) +1 -1

-static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+static inline bool handle_user_split_lock(unsigned long ip)

This is necessary because VMX can be compiled without CPU_SUP_INTEL?

Ya, it came about when cleaning up the IA32_FEATURE_CONTROL MSR handling to consolidate duplicate code.

config KVM_INTEL
tristate "KVM for Intel (and compatible) processors support" depends on KVM && IA32_FEAT_CTL

Quoted 3 lines

config IA32_FEAT_CTL

         def_bool y
         depends on CPU_SUP_INTEL || CPU_SUP_CENTAUR || CPU_SUP_ZHAOXIN

Ah, indeed. So something like the below would make sense. Hmm?

Of course that can be mangled into Xiaoyao's patches, I'm not worried about my patch count :)

8<----------------

arch/x86/include/asm/cpu.h +3 -3

@@ -43,14 +43,14 @@ unsigned int x86_stepping(unsigned int s
  #ifdef CONFIG_CPU_SUP_INTEL
  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 int handle_ac_split_lock(unsigned long ip);
  extern void split_lock_validate_module_text(struct module *me, void *text, void *text_end);
  #else
  static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
  static inline void switch_to_sld(unsigned long tifn) {}
-static inline bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+static int handle_ac_split_lock(unsigned long ip)
  {
-	return false;
+	return -ENOSYS;
  }
  static inline void split_lock_validate_module_text(struct module *me, void *text, void *text_end) {}
  #endif

arch/x86/kernel/cpu/intel.c +13 -5

@@ -1102,13 +1102,20 @@ static void split_lock_init(void)
  	split_lock_verify_msr(sld_state != sld_off);
  }
 
-bool handle_user_split_lock(struct pt_regs *regs, long error_code)
+int handle_ac_split_lock(unsigned long ip)
  {
-	if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
-		return false;
+	switch (sld_state) {
+	case sld_warn:
+		break;
+	case sld_off:
+		pr_warn_once("#AC: Spurious trap at address: 0x%lx\n", ip);
+		return -ENOSYS;
+	case sld_fatal:
+		return -EFAULT;
+	}
 
  	pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n",
-			    current->comm, current->pid, regs->ip);
+			    current->comm, current->pid, ip);
 
  	/*
  	 * Disable the split lock detection for this task so it can make

@@ -1117,8 +1124,9 @@ bool handle_user_split_lock(struct pt_re
  	 */
  	sld_update_msr(false);
  	set_tsk_thread_flag(current, TIF_SLD);
-	return true;
+	return 0;
  }
+EXPORT_SYMBOL_GPL(handle_ac_split_lock);
 
  /*
   * This function is called only when switching between tasks with

arch/x86/kernel/traps.c +1 -1

@@ -304,7 +304,7 @@ dotraplinkage void do_alignment_check(st
 
  	local_irq_enable();
 
-	if (handle_user_split_lock(regs, error_code))
+	if (!(regs->flags & X86_EFLAGS_AC) && !handle_ac_split_lock(regs->ip))
  		return;
 
  	do_trap(X86_TRAP_AC, SIGBUS, "alignment check", regs,

arch/x86/kvm/vmx/vmx.c +41 -3

@@ -65,6 +65,7 @@
 
  MODULE_AUTHOR("Qumranet");
  MODULE_LICENSE("GPL");
+MODULE_INFO(sld_safe, "Y");
 
  #ifdef MODULE
  static const struct x86_cpu_id vmx_cpu_id[] = {

@@ -4623,6 +4624,22 @@ static int handle_machine_check(struct k
  	return 1;
  }
 
+static bool guest_handles_ac(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * If guest has alignment checking enabled in CR0 and activated in
+	 * eflags, then the #AC originated from CPL3 and the guest is able
+	 * to handle it. It does not matter whether this is a regular or
+	 * a split lock operation induced #AC.
+	 */
+	if (vmx_get_cpl(vcpu) == 3 && kvm_read_cr0_bits(vcpu, X86_CR0_AM) &&
+	    kvm_get_rflags(vcpu) & X86_EFLAGS_AC)
+		return true;
+
+	/* Add guest SLD handling checks here once it's supported */
+	return false;
+}
+
  static int handle_exception_nmi(struct kvm_vcpu *vcpu)
  {
  	struct vcpu_vmx *vmx = to_vmx(vcpu);

@@ -4630,6 +4647,7 @@ static int handle_exception_nmi(struct k
  	u32 intr_info, ex_no, error_code;
  	unsigned long cr2, rip, dr6;
  	u32 vect_info;
+	int err;
 
  	vect_info = vmx->idt_vectoring_info;
  	intr_info = vmx->exit_intr_info;

@@ -4688,9 +4706,6 @@ static int handle_exception_nmi(struct k
  		return handle_rmode_exception(vcpu, ex_no, error_code);
 
  	switch (ex_no) {
-	case AC_VECTOR:
-		kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
-		return 1;
  	case DB_VECTOR:
  		dr6 = vmcs_readl(EXIT_QUALIFICATION);
  		if (!(vcpu->guest_debug &

@@ -4719,6 +4734,29 @@ static int handle_exception_nmi(struct k
  		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
  		kvm_run->debug.arch.exception = ex_no;
  		break;
+	case AC_VECTOR:
+		if (guest_handles_ac(vcpu)) {
+			kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
+			return 1;
+		}
+		/*
+		 * Handle #AC caused by split lock detection. If the host
+		 * mode is sld_warn, then it warns, marks current with
+		 * TIF_SLD and disables split lock detection. So the guest
+		 * can just continue.
+		 *
+		 * If the host mode is fatal, the handling code warned. Let
+		 * qemu kill itself.
+		 *
+		 * If the host mode is off, then this #AC is bonkers and
+		 * something is badly wrong. Let it fail as well.
+		 */
+		err = handle_ac_split_lock(kvm_rip_read(vcpu));
+		if (!err)
+			return 1;
+		/* Propagate the error type to user space */
+		error_code = err == -EFAULT ? 0x100 : 0x200;
+		fallthrough;
  	default:
  		kvm_run->exit_reason = KVM_EXIT_EXCEPTION;
  		kvm_run->ex.exception = ex_no;

  +static bool guest_handles_ac(struct kvm_vcpu *vcpu)

arch/x86/kvm/vmx/vmx.c +15 -0

+{
+    /*
+     * If guest has alignment checking enabled in CR0 and activated in
+     * eflags, then the #AC originated from CPL3 and the guest is able
+     * to handle it. It does not matter whether this is a regular or
+     * a split lock operation induced #AC.
+     */
+    if (vmx_get_cpl(vcpu) == 3 && kvm_read_cr0_bits(vcpu, X86_CR0_AM) &&
+        kvm_get_rflags(vcpu) & X86_EFLAGS_AC)
+        return true;
+
+    /* Add guest SLD handling checks here once it's supported */
+    return false;
+}
+

  static int handle_exception_nmi(struct kvm_vcpu *vcpu) {
      struct vcpu_vmx *vmx = to_vmx(vcpu);

arch/x86/kvm/vmx/vmx.c +1 -3

@@ -4630,6 +4647,7 @@ static int handle_exception_nmi(struct k
       u32 intr_info, ex_no, error_code;
       unsigned long cr2, rip, dr6;
       u32 vect_info;
+    int err;
         vect_info = vmx->idt_vectoring_info;
       intr_info = vmx->exit_intr_info;

@@ -4688,9 +4706,6 @@ static int handle_exception_nmi(struct k
           return handle_rmode_exception(vcpu, ex_no, error_code);
         switch (ex_no) {
-    case AC_VECTOR:
-        kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
-        return 1;

      case DB_VECTOR:
          dr6 = vmcs_readl(EXIT_QUALIFICATION);
          if (!(vcpu->guest_debug &

arch/x86/kvm/vmx/vmx.c +13 -0

@@ -4719,6 +4734,29 @@ static int handle_exception_nmi(struct k
           kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
           kvm_run->debug.arch.exception = ex_no;
           break;
+    case AC_VECTOR:
+        if (guest_handles_ac(vcpu)) {
+            kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
+            return 1;
+        }
+        /*
+         * Handle #AC caused by split lock detection. If the host
+         * mode is sld_warn, then it warns, marks current with
+         * TIF_SLD and disables split lock detection. So the guest
+         * can just continue.
+         *
+         * If the host mode is fatal, the handling code warned. Let
+         * qemu kill itself.

arch/x86/kvm/vmx/vmx.c +5 -3

+{
+	return vmx_get_cpl(vcpu) == 3 && kvm_read_cr0_bits(vcpu, X86_CR0_AM) &&
+	       (kvm_get_rflags(vcpu) & X86_EFLAGS_AC);
+}
+
 static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

@@ -4688,9 +4694,6 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 		return handle_rmode_exception(vcpu, ex_no, error_code);
 
 	switch (ex_no) {
-	case AC_VECTOR:
-		kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
-		return 1;
 	case DB_VECTOR:
 		dr6 = vmcs_readl(EXIT_QUALIFICATION);
 		if (!(vcpu->guest_debug &

@@ -4719,6 +4722,27 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
 		kvm_run->debug.arch.exception = ex_no;

arch/x86/kvm/vmx/vmx.c static int handle_machine_check(struct kvm_vcpu *vcpu) · line 4626

Quoted 5 lines · Sean Christopherson

arch/x86/kvm/vmx/vmx.c +1 -0

@@ -4623,6 +4623,12 @@ static int handle_machine_check(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static inline bool guest_cpu_alignment_check_enabled(struct kvm_vcpu *vcpu)

I used a different function name intentionally so the check for 'guest want's split lock #AC' can go there as well once it's sorted.

Quoted 21 lines

arch/x86/kvm/vmx/vmx.c +5 -3

+{
+	return vmx_get_cpl(vcpu) == 3 && kvm_read_cr0_bits(vcpu, X86_CR0_AM) &&
+	       (kvm_get_rflags(vcpu) & X86_EFLAGS_AC);
+}
+
 static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);

@@ -4688,9 +4694,6 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 		return handle_rmode_exception(vcpu, ex_no, error_code);
 
 	switch (ex_no) {
-	case AC_VECTOR:
-		kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
-		return 1;
 	case DB_VECTOR:
 		dr6 = vmcs_readl(EXIT_QUALIFICATION);
 		if (!(vcpu->guest_debug &

@@ -4719,6 +4722,27 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 		kvm_run->debug.arch.pc = vmcs_readl(GUEST_CS_BASE) + rip;
 		kvm_run->debug.arch.exception = ex_no;

arch/x86/kvm/vmx/vmx.c static int handle_exception_nmi(struct kvm_vcpu *vcpu) · line 4733

Quoted 10 lines

arch/x86/kvm/vmx/vmx.c +9 -0

 		break;
+	case AC_VECTOR:
+		/*
+		 * Reflect #AC to the guest if it's expecting the #AC, i.e. has
+		 * legacy alignment check enabled.  Pre-check host split lock
+		 * turned on to avoid the VMREADs needed to check legacy #AC,
+		 * i.e. reflect the #AC if the only possible source is legacy
+		 * alignment checks.
+		 */
+		if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) ||

I think the right thing to do here is to make this really independent of that feature, i.e. inject the exception if

(CPL==3 && CR0.AM && EFLAGS.AC) || (FUTURE && (GUEST_TEST_CTRL & SLD))

iow. when its really clear that the guest asked for it. If there is an actual #AC with SLD disabled and !(CPL==3 && CR0.AM && EFLAGS.AC) then something is badly wrong and the thing should just die. That's why I separated handle_guest_split_lock() and tell about that case.

Quoted 14 lines · Thomas Gleixner

arch/x86/kvm/vmx/vmx.c static int handle_exception_nmi(struct kvm_vcpu *vcpu) · line 4733

Quoted 9 lines · Sean Christopherson

arch/x86/kvm/vmx/vmx.c +9 -0

+	case AC_VECTOR:
+		/*
+		 * Reflect #AC to the guest if it's expecting the #AC, i.e. has
+		 * legacy alignment check enabled.  Pre-check host split lock
+		 * turned on to avoid the VMREADs needed to check legacy #AC,
+		 * i.e. reflect the #AC if the only possible source is legacy
+		 * alignment checks.
+		 */
+		if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) ||

I think the right thing to do here is to make this really independent of that feature, i.e. inject the exception if

Quoted 6 lines

(CPL==3 && CR0.AM && EFLAGS.AC) || (FUTURE && (GUEST_TEST_CTRL & SLD))

iow. when its really clear that the guest asked for it. If there is an actual #AC with SLD disabled and !(CPL==3 && CR0.AM && EFLAGS.AC) then something is badly wrong and the thing should just die. That's why I separated handle_guest_split_lock() and tell about that case.

That puts KVM in a weird spot if/when intercepting #AC is no longer necessary, e.g. "if" future CPUs happen to gain a feature that traps into the hypervisor (KVM) if a potential near-infinite ucode loop is detected.

The only reason KVM intercepts #AC (before split-lock) is to prevent a malicious guest from executing a DoS attack on the host by putting the #AC handler in ring 3. Current CPUs will get stuck in ucode vectoring #AC faults more or less indefinitely, e.g. long enough to trigger watchdogs in the host.

Injecting #AC if and only if KVM is 100% certain the guest wants the #AC would lead to divergent behavior if KVM chose to not intercept #AC, e.g.

some theoretical unknown #AC source would conditionally result in exits to userspace depending on whether or not KVM wanted to intercept #AC for other reasons.

TBH, the more I learn about this, the more I tend to just give up on this whole split lock stuff in its current form and wait until HW folks provide something which is actually usable:

- Per thread
- Properly distinguishable from a regular #AC via error code

OTOH, that means I won't be able to use it before retirement. Oh well.

Thanks,

tglx

TBH, the more I learn about this, the more I tend to just give up on this whole split lock stuff in its current form and wait until HW folks provide something which is actually usable:

- Per thread
- Properly distinguishable from a regular #AC via error code

Quoted 11 lines · Sean Christopherson

Quoted 1 line · Thomas Gleixner

Sean Christopherson <[email protected]> writes:

That puts KVM in a weird spot if/when intercepting #AC is no longer necessary, e.g. "if" future CPUs happen to gain a feature that traps into the hypervisor (KVM) if a potential near-infinite ucode loop is detected.

The only reason KVM intercepts #AC (before split-lock) is to prevent a malicious guest from executing a DoS attack on the host by putting the #AC handler in ring 3. Current CPUs will get stuck in ucode vectoring #AC faults more or less indefinitely, e.g. long enough to trigger watchdogs in the host.

Which is thankfully well documented in the VMX code and the corresponding chapter in the SDM.

Quoted 2 lines

Injecting #AC if and only if KVM is 100% certain the guest wants the #AC would lead to divergent behavior if KVM chose to not intercept #AC, e.g.

AFAICT, #AC is not really something which is performance relevant, but I might obviously be uninformed on that.

Assumed it is not, then there is neither a hard requirement nor a real incentive to give up on intercepting #AC even when future CPUs have a fix for the above wreckage.

Quoted 3 lines

some theoretical unknown #AC source would conditionally result in exits to userspace depending on whether or not KVM wanted to intercept #AC for other reasons.

I'd rather like to know when there is an unknown #AC source instead of letting the guest silently swallow it.

TBH, the more I learn about this, the more I tend to just give up on this whole split lock stuff in its current form and wait until HW folks provide something which is actually usable:

- Per thread
- Properly distinguishable from a regular #AC via error code

OTOH, that means I won't be able to use it before retirement. Oh well.

Thanks,

tglx

Quoted 7 lines · Thomas Gleixner

Sean Christopherson <[email protected]> writes:
AFAICT, #AC is not really something which is performance relevant, but I might obviously be uninformed on that.

Assumed it is not, then there is neither a hard requirement nor a real incentive to give up on intercepting #AC even when future CPUs have a fix for the above wreckage.

Agreed that there's no hard requirement, but general speaking, the less KVM needs to poke into the guest the better.

Quoted 6 lines

Quoted 3 lines

some theoretical unknown #AC source would conditionally result in exits to userspace depending on whether or not KVM wanted to intercept #AC for other reasons.

I'd rather like to know when there is an unknown #AC source instead of letting the guest silently swallow it.

Trying to prevent the guest from squashing a spurious fault is a fools
errand. For example, with nested virtualization, the correct behavior
from an architectural perspective is to forward exceptions from L2 (the nested VM) to L1 (the direct VM) that L1 wants to intercept. E.g. if L1 wants to intercept #AC faults that happen in L2, then KVM reflects all #AC faults into L1 as VM-Exits without ever reaching this code.

Anyways, this particular case isn't a sticking point, i.e. I'd be ok with exiting to userspace on a spurious #AC, I just don't see the value in doing so. Returning KVM_EXIT_EXCEPTION doesn't necessarily equate to throwing up a red flag, e.g. from a kernel perspective you'd still be relying on the userspace VMM to report the error in a sane manner. I think at one point Xiaoyao had a WARN_ON for a spurious #AC, but it was removed because the odds of a false positive due to some funky corner case seemed higher than detecting a CPU bug.

As I just verified, it's possible to load the vmware module parallel to the KVM/VMX one.

So either we deal with it in some way or just decide that SLD and HV modules which do not have the MOD_INFO(sld_safe) magic cannot be loaded when SLD is enabled on the host. I'm fine with the latter :)

What a mess.

Just to communicate the information that was given to me: we do intend to fix the SLD issue in VMware and if needed to release a minor version that addresses it. Having said that, there are other hypervisors, such as virtualbox or jailhouse, which would have a similar issue.

Quoted 9 lines · Thomas Gleixner

As I just verified, it's possible to load the vmware module parallel to the KVM/VMX one.

So either we deal with it in some way or just decide that SLD and HV modules which do not have the MOD_INFO(sld_safe) magic cannot be loaded when SLD is enabled on the host. I'm fine with the latter :)

Quoted 1 line

What a mess.

[ +Doug ]

Just to communicate the information that was given to me: we do intend to fix the SLD issue in VMware and if needed to release a minor version that addresses it. Having said that, there are other hypervisors, such as virtualbox or jailhouse, which would have a similar issue.

If we go the approach of not letting VM modules load if it doesn't have the sld_safe flag set, how is this different than a VM module not loading due to kabi breakage?

On Thu, 2 Apr 2020, Steven Rostedt wrote:

Quoted 3 lines

If we go the approach of not letting VM modules load if it doesn't have the sld_safe flag set, how is this different than a VM module not loading due to kabi breakage?

Why not a compromise: if such a module is attempted to be loaded, print up a message saying something akin to "turn the parameter 'split_lock_detect' off" as we reject loading it- and if we see that we've booted with it off just splat a WARN_ON() if someone tries to load such modules?

Quoted 6 lines · Kenneth R. Crudup

Quoted 3 lines · Steven Rostedt

If we go the approach of not letting VM modules load if it doesn't have the sld_safe flag set, how is this different than a VM module not loading due to kabi breakage?

Quoted 4 lines

Why not a compromise: if such a module is attempted to be loaded, print up a message saying something akin to "turn the parameter 'split_lock_detect' off" as we reject loading it- and if we see that we've booted with it off just splat a WARN_ON() if someone tries to load such modules?

What modules are we talking about? I thought we were discussing L1 hypervisors, which are just binary blobs. The only modules at the L0 level are kvm and kvm_intel.

First three patches from Xiaoyao's series to add split-lock #AC support in KVM.

Xiaoyao Li (3):
KVM: x86: Emulate split-lock access as a write in emulator x86/split_lock: Refactor and export handle_user_split_lock() for KVM
KVM: VMX: Extend VMX's #AC interceptor to handle split lock #AC in
guest

Quoted 8 lines · Sean Christopherson

First three patches from Xiaoyao's series to add split-lock #AC support in KVM.

Xiaoyao Li (3):
KVM: x86: Emulate split-lock access as a write in emulator x86/split_lock: Refactor and export handle_user_split_lock() for KVM
KVM: VMX: Extend VMX's #AC interceptor to handle split lock #AC in
guest

Sorry I was out of the loop on this (I'm working part time and it's a mess). Sean, can you send the patches as a top-level message? I'll queue them and get them to Linus over the weekend.

As Peter and myself don't have access to a SLD enabled machine, the KVM/VMX part is untested. The module scan part works.

Quoted 2 lines · Thomas Gleixner

As Peter and myself don't have access to a SLD enabled machine, the KVM/VMX part is untested. The module scan part works.

I just applied both of these patches to my (Linus' tip) tree, and unfortunately VMWare still hangs if split_lock_detect= is set to anything but "off".

Was there anything else I'd needed to do?

Hmm, you're not seeing this:
+ pr_warn("disabled due to VMLAUNCH in module: %s\n", me->name);
fire when you load the vmware kernel module?

Could you slip me a copy of that thing by private mail?

Quoted 3 lines · Peter Zijlstra

Hmm, you're not seeing this:
+ pr_warn("disabled due to VMLAUNCH in module: %s\n", me->name);
fire when you load the vmware kernel module?

I just looked back at the syslog's copy of the kernel messages at the time I'd tried it, and no, I don't see that message.

Quoted 1 line

Could you slip me a copy of that thing by private mail?

OK, gimme a couple of days though, I've gotta get a little work done. (Also, what "thing" exactly did you want?)

Quoted 2 lines

(Also, what "thing" exactly did you want?)

All the .ko files that come with vmware.

Quoted 2 lines · Thomas Gleixner

As Peter and myself don't have access to a SLD enabled machine, the KVM/VMX part is untested. The module scan part works.

I just applied both of these patches to my (Linus' tip) tree, and unfortunately VMWare still hangs if split_lock_detect= is set to anything but "off".

Was there anything else I'd needed to do?

Quoted 8 lines · Kenneth R. Crudup

Quoted 2 lines · Thomas Gleixner

As Peter and myself don't have access to a SLD enabled machine, the KVM/VMX part is untested. The module scan part works.

I just applied both of these patches to my (Linus' tip) tree, and unfortunately VMWare still hangs if split_lock_detect= is set to anything but "off".

Quoted 1 line

Was there anything else I'd needed to do?

Hmm. Not really. Does dmesg show the warning when the VMWare module loads? Something like:

x86/split lock detection: disabled due to VMLAUNCH in module: ….

Quoted 14 lines · Thomas Gleixner

Quoted 9 lines · Kenneth R. Crudup

Quoted 2 lines · Thomas Gleixner

As Peter and myself don't have access to a SLD enabled machine, the KVM/VMX part is untested. The module scan part works.

I just applied both of these patches to my (Linus' tip) tree, and unfortunately VMWare still hangs if split_lock_detect= is set to anything but "off".

Was there anything else I'd needed to do?

Quoted 4 lines

Hmm. Not really. Does dmesg show the warning when the VMWare module loads? Something like:

x86/split lock detection: disabled due to VMLAUNCH in module: ….

I ran an objdump on VMware workstation and indeed I do not see a VMLAUNCH/VMRESUME. I do see however VMXON which should also be good for detecting hypervisors. I will try to understand why VMLAUNCH is not there.