LinuxLists.cc - [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

[permalink] [raw]

Subject: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

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(-)

--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -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 */
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -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))))
+ goto bad_module;
+
+ text += insn.length;
+ }
+
+ return;
+
+bad_module:
+ pr_warn("disabled due to VMX in module: %s\n", me->name);
+ sld_state = sld_off;
+ on_each_cpu(sld_remote_kill, NULL, 1);
+}
+
static void split_lock_init(void)
{
split_lock_verify_msr(sld_state != sld_off);
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -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);
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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. */
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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;

2020-04-02 16:21:34

by Xiaoyao Li

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On 4/2/2020 11:23 PM, Peter Zijlstra wrote:
>
> 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(-)
>
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -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 */
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -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))))
> + goto bad_module;
> +
> + text += insn.length;
> + }
> +
> + return;
> +
> +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?

> + on_each_cpu(sld_remote_kill, NULL, 1);
> +}
> +
> static void split_lock_init(void)
> {
> split_lock_verify_msr(sld_state != sld_off);
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -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);
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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. */
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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;
>

2020-04-02 17:06:25

by Nadav Amit

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

> On Apr 2, 2020, at 9:25 AM, Peter Zijlstra <[email protected]> wrote:
>
>
> Learn to trim your replies already!
>
> On Fri, Apr 03, 2020 at 12:20:08AM +0800, Xiaoyao Li wrote:
>> On 4/2/2020 11:23 PM, Peter Zijlstra wrote:
>
>>> +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.

Just wondering, since I lack hardware as well: can the performance counter
LOCK_CYCLES.SPLIT_LOCK_UC_LOCK_DURATION be used to detect split-locks
similarly to SLD (although it would be after the split-lock event)?

2020-04-02 17:08:27

by Xiaoyao Li

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On 4/3/2020 12:25 AM, Peter Zijlstra wrote:
>
> Learn to trim your replies already!

Sorry.

> On Fri, Apr 03, 2020 at 12:20:08AM +0800, Xiaoyao Li wrote:
>> On 4/2/2020 11:23 PM, Peter Zijlstra wrote:
>
>>> +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.
>
>> 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.

2020-04-02 17:09:08

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

"Kenneth R. Crudup" <[email protected]> writes:
> On Thu, 2 Apr 2020, Andy Lutomirski wrote:
>> This seems like much more of a fixup than we would usually do ....
>
> TBH, I was kinda surprised to see that patchset myself.
>
> -Kenny, guessing Linus hasn't read this far yet :)

The much we hate it, we simply have users depending on these things for
various reasons.

That said, I'm perfectly fine with changing it to 'refuse module load'.

Thanks,

tglx

2020-04-02 17:35:53

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Xiaoyao Li <[email protected]> writes:
> On 4/3/2020 12:25 AM, Peter Zijlstra wrote:
>> On Fri, Apr 03, 2020 at 12:20:08AM +0800, Xiaoyao Li wrote:
>>> 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.

Obviously I cant.

> 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.

Can we first have a sane solution for the problem at hand?

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.

Thanks,

tglx

2020-04-02 18:05:47

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Learn to trim your replies already!

On Fri, Apr 03, 2020 at 12:20:08AM +0800, Xiaoyao Li wrote:
> On 4/2/2020 11:23 PM, Peter Zijlstra wrote:

> > +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.

> 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.

2020-04-02 18:52:33

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Thu, Apr 02, 2020 at 10:51:28AM -0700, Sean Christopherson wrote:
> On Thu, Apr 02, 2020 at 07:34:35PM +0200, Thomas Gleixner wrote:
> > 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.

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

That's like building a bridge with rotten timber.

2020-04-02 19:17:36

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Thu, Apr 02, 2020 at 07:34:35PM +0200, Thomas Gleixner wrote:
> 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.

2020-04-02 20:37:41

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Thu, Apr 02, 2020 at 08:51:48PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 10:51:28AM -0700, Sean Christopherson wrote:
> > On Thu, Apr 02, 2020 at 07:34:35PM +0200, Thomas Gleixner wrote:
> > > 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.
>
> 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.

2020-04-02 21:10:11

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Sean Christopherson <[email protected]> writes:
> On Thu, Apr 02, 2020 at 08:51:48PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 10:51:28AM -0700, Sean Christopherson wrote:
>> > On Thu, Apr 02, 2020 at 07:34:35PM +0200, Thomas Gleixner wrote:
>> > > 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.
>>
>> 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.

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.

Thanks,

tglx

2020-04-02 21:22:08

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Thu, Apr 02, 2020 at 11:04:05PM +0200, Thomas Gleixner wrote:
> Sean Christopherson <[email protected]> writes:
> > On Thu, Apr 02, 2020 at 08:51:48PM +0200, Peter Zijlstra wrote:
> >> On Thu, Apr 02, 2020 at 10:51:28AM -0700, Sean Christopherson wrote:
> >> > On Thu, Apr 02, 2020 at 07:34:35PM +0200, Thomas Gleixner wrote:
> >> > > 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.
> >>
> >> 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.
>
> 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.

Or more likely, gets fixed, just not in upstream :-)

> 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.

Ya, I see where you're coming from. As above, I agree that having a "KVM
only" mode does more harm than good.

2020-04-03 00:30:44

by Rasmus Villemoes

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On 02/04/2020 14.32, Thomas Gleixner wrote:
> 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.

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

Rasmus

2020-04-03 08:11:10

by David Laight

[permalink] [raw]

Subject: RE: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

From: Peter Zijlstra
> 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.
...
> + 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))))
> + goto bad_module;
> +
> + text += insn.length;
> + }

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

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2020-04-03 12:30:38

by kernel test robot

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Attachments:

(No filename) (1.96 kB)
.config.gz (8.31 kB)
Download all attachments

2020-04-03 14:36:13

by Jessica Yu

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

+++ Rasmus Villemoes [03/04/20 01:42 +0200]:
>On 02/04/2020 14.32, Thomas Gleixner wrote:
>> 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.
>
>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)?

Jessica

2020-04-03 14:36:31

[permalink] [raw]

Subject: Re: [patch v2 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 08:09:03AM +0000, David Laight wrote:
> From: Peter Zijlstra
> > 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.
> ...
> > + 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))))
> > + goto bad_module;
> > +
> > + text += insn.length;
> > + }
>
> How long is that going to take on a module with (say) 400k of text?

It's module load, why would you care? I suspect it's really fast, but
even if it wasn't I couldn't be arsed.

2020-04-03 15:44:24

by kernel test robot

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Hi Thomas,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/auto-latest]
[also build test ERROR on next-20200403]
[cannot apply to jeyu/modules-next tip/x86/core v5.6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Thomas-Gleixner/x86-Prevent-Split-Lock-Detection-wreckage-on-VMX-hypervisors/20200403-171430
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git ee8bac724cc7767dcf9480afb656318994f22c3d
config: x86_64-randconfig-s1-20200403 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>

All errors (new ones prefixed by >>):

arch/x86/kernel/module.c: In function 'module_finalize':
>> arch/x86/kernel/module.c:257:17: error: 'struct module' has no member named 'sld_safe'
if (text && !me->sld_safe) {
^~

vim +257 arch/x86/kernel/module.c

220
221 int module_finalize(const Elf_Ehdr *hdr,
222 const Elf_Shdr *sechdrs,
223 struct module *me)
224 {
225 const Elf_Shdr *s, *text = NULL, *alt = NULL, *locks = NULL,
226 *para = NULL, *orc = NULL, *orc_ip = NULL;
227 char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
228
229 for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
230 if (!strcmp(".text", secstrings + s->sh_name))
231 text = s;
232 if (!strcmp(".altinstructions", secstrings + s->sh_name))
233 alt = s;
234 if (!strcmp(".smp_locks", secstrings + s->sh_name))
235 locks = s;
236 if (!strcmp(".parainstructions", secstrings + s->sh_name))
237 para = s;
238 if (!strcmp(".orc_unwind", secstrings + s->sh_name))
239 orc = s;
240 if (!strcmp(".orc_unwind_ip", secstrings + s->sh_name))
241 orc_ip = s;
242 }
243
244 if (alt) {
245 /* patch .altinstructions */
246 void *aseg = (void *)alt->sh_addr;
247 apply_alternatives(aseg, aseg + alt->sh_size);
248 }
249 if (locks && text) {
250 void *lseg = (void *)locks->sh_addr;
251 void *tseg = (void *)text->sh_addr;
252 alternatives_smp_module_add(me, me->name,
253 lseg, lseg + locks->sh_size,
254 tseg, tseg + text->sh_size);
255 }
256
> 257 if (text && !me->sld_safe) {
258 void *tseg = (void *)text->sh_addr;
259 split_lock_validate_module_text(me, tseg, tseg + text->sh_size);
260 }
261
262 if (para) {
263 void *pseg = (void *)para->sh_addr;
264 apply_paravirt(pseg, pseg + para->sh_size);
265 }
266
267 /* make jump label nops */
268 jump_label_apply_nops(me);
269
270 if (orc && orc_ip)
271 unwind_module_init(me, (void *)orc_ip->sh_addr, orc_ip->sh_size,
272 (void *)orc->sh_addr, orc->sh_size);
273
274 return 0;
275 }
276

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (3.34 kB)
.config.gz (35.57 kB)
Download all attachments

2020-04-03 15:47:15

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
> +++ Rasmus Villemoes [03/04/20 01:42 +0200]:
> > On 02/04/2020 14.32, Thomas Gleixner wrote:
> > > 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.
> >
> > 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)?

It would; didn't know there was one.

2020-04-03 16:13:05

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
> On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:

> > > 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.

2020-04-03 16:36:44

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Thu, Apr 02, 2020 at 02:32:59PM +0200, Thomas Gleixner wrote:
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -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) {

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.

> + 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);
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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. */
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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;
>

2020-04-03 16:42:25

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 04:16:38PM +0000, David Laight wrote:
> From: Peter Zijlstra
> > Sent: 03 April 2020 17:12
> > On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
> > > On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> > > > On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
> >
> > > > > 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.
>
> 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?

You think the binary module people care to respect our module build
warnings?

2020-04-03 16:49:08

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
> > +++ Rasmus Villemoes [03/04/20 01:42 +0200]:
> > > On 02/04/2020 14.32, Thomas Gleixner wrote:
> > > > 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.
> > >
> > > 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)?
>
> 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?

Partially tested patch incoming...

2020-04-03 16:51:57

by David Laight

[permalink] [raw]

Subject: RE: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

From: Peter Zijlstra
> Sent: 03 April 2020 17:12
> On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
> > On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> > > On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
>
> > > > 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.

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?

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2020-04-03 16:52:28

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 06:12:05PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
> > On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> > > On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
>
> > > > 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.

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.

2020-04-03 16:52:45

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 09:25:55AM -0700, Sean Christopherson wrote:
> On Fri, Apr 03, 2020 at 06:12:05PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
> > > On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> > > > On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
> >
> > > > > 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.
>
> 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 :/

2020-04-03 16:52:48

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 09:36:05AM -0700, Sean Christopherson wrote:
> On Thu, Apr 02, 2020 at 02:32:59PM +0200, Thomas Gleixner wrote:
> > --- a/arch/x86/kernel/module.c
> > +++ b/arch/x86/kernel/module.c
> > @@ -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) {
>
> 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.
>
> > + 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 :/.

2020-04-03 16:53:09

by Nadav Amit

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

> On Apr 3, 2020, at 9:40 AM, Peter Zijlstra <[email protected]> wrote:
>
> On Fri, Apr 03, 2020 at 09:25:55AM -0700, Sean Christopherson wrote:
>> On Fri, Apr 03, 2020 at 06:12:05PM +0200, Peter Zijlstra wrote:
>>> On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
>>>> On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
>>>>> On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
>>>
>>>>>> 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.
>>
>> 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.

2020-04-03 17:47:45

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Fri, Apr 03, 2020 at 04:48:35PM +0000, Nadav Amit wrote:
> > On Apr 3, 2020, at 9:40 AM, Peter Zijlstra <[email protected]> wrote:
> >
> > On Fri, Apr 03, 2020 at 09:25:55AM -0700, Sean Christopherson wrote:
> >> On Fri, Apr 03, 2020 at 06:12:05PM +0200, Peter Zijlstra wrote:
> >>> On Fri, Apr 03, 2020 at 09:01:56AM -0700, Sean Christopherson wrote:
> >>>> On Fri, Apr 03, 2020 at 05:21:58PM +0200, Peter Zijlstra wrote:
> >>>>> On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
> >>>
> >>>>>> 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.
> >>
> >> 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.

Well phooey.

2020-04-03 18:37:07

by Jessica Yu

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

+++ Peter Zijlstra [03/04/20 18:41 +0200]:
>On Fri, Apr 03, 2020 at 09:36:05AM -0700, Sean Christopherson wrote:
>> On Thu, Apr 02, 2020 at 02:32:59PM +0200, Thomas Gleixner wrote:
>> > --- a/arch/x86/kernel/module.c
>> > +++ b/arch/x86/kernel/module.c
>> > @@ -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) {
>>
>> 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.
>>
>> > + 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 :/.

Hm, I can look into exposing load_info to arch module code and will
post a patch on Monday.

Jessica

2020-04-03 18:56:35

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Peter Zijlstra <[email protected]> writes:
> On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
>> +++ Rasmus Villemoes [03/04/20 01:42 +0200]:
>> > On 02/04/2020 14.32, Thomas Gleixner wrote:
>> > > 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.
>> >
>> > 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)?
>
> 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

2020-04-03 20:58:57

by Andy Lutomirski

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

> On Apr 3, 2020, at 11:54 AM, Thomas Gleixner <[email protected]> wrote:
>
> Peter Zijlstra <[email protected]> writes:
>>> On Fri, Apr 03, 2020 at 04:35:00PM +0200, Jessica Yu wrote:
>>> +++ Rasmus Villemoes [03/04/20 01:42 +0200]:
>>>> On 02/04/2020 14.32, Thomas Gleixner wrote:
>>>>> 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.
>>>>
>>>> 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)?
>>
>> 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.

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

>
> Thanks,
>
> tglx

2020-04-03 21:50:33

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

Andy Lutomirski <[email protected]> writes:
>> On Apr 3, 2020, at 11:54 AM, Thomas Gleixner <[email protected]> wrote:
>> 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?

set_memory_x() is not exported anymore, but they found new ways of
circumvention. set_memory_x() is only used on really old kernels.

Thanks,

tglx

2020-04-06 12:25:28

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Thu, Apr 02, 2020 at 02:32:59PM +0200, Thomas Gleixner wrote:
> 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.

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.

2020-04-06 14:42:12

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Mon, Apr 06, 2020 at 05:23:43AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 02, 2020 at 02:32:59PM +0200, Thomas Gleixner wrote:
> > 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.
>
> 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.

Look at is as another layer to enforce sanity on binary only modules.

Do we want to go that way, and do you know of other code patterns you'd
want to fail module loading for?

2020-04-06 15:19:43

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [patch 1/2] x86,module: Detect VMX modules and disable Split-Lock-Detect

On Mon, Apr 06, 2020 at 04:40:20PM +0200, Peter Zijlstra wrote:
> 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.

2020-04-06 15:23:40