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
all out-of-tree modules' text and look for VMX instructions and refuse
to load it when SLD is enabled (default) and the module isn't marked
'sld_safe'.
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.
Reported-by: "Kenneth R. Crudup" <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/cpu.h | 5 ++
arch/x86/kernel/cpu/intel.c | 5 ++
arch/x86/kernel/module.c | 87 ++++++++++++++++++++++++++++++++++++++++++-
include/linux/moduleloader.h | 2
kernel/module.c | 3 -
5 files changed, 99 insertions(+), 3 deletions(-)
--- 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 bool split_lock_enabled(void);
#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,9 @@ static inline bool handle_user_split_loc
{
return false;
}
+static inline bool split_lock_enabled(void)
+{
+ return false;
+}
#endif
#endif /* _ASM_X86_CPU_H */
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1061,6 +1061,11 @@ static void sld_update_msr(bool on)
wrmsrl(MSR_TEST_CTRL, test_ctrl_val);
}
+bool split_lock_enabled(void)
+{
+ return sld_state != sld_off;
+}
+
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,8 @@
#include <asm/pgtable.h>
#include <asm/setup.h>
#include <asm/unwind.h>
+#include <asm/cpu.h>
+#include <asm/insn.h>
#if 0
#define DEBUGP(fmt, ...) \
@@ -217,6 +219,78 @@ int apply_relocate_add(Elf64_Shdr *sechd
}
#endif
+static bool insn_is_vmx(struct insn *insn)
+{
+ u8 modrm = insn->modrm.bytes[0];
+ u8 modrm_mod = X86_MODRM_MOD(modrm);
+ u8 modrm_reg = X86_MODRM_REG(modrm);
+
+ u8 prefix = insn->prefixes.bytes[0];
+
+ if (insn->opcode.bytes[0] != 0x0f)
+ return false;
+
+ switch (insn->opcode.bytes[1]) {
+ case 0x01:
+ switch (insn->opcode.bytes[2]) {
+ case 0xc1: /* VMCALL */
+ case 0xc2: /* VMLAUNCH */
+ case 0xc3: /* VMRESUME */
+ case 0xc4: /* VMXOFF */
+ return true;
+
+ default:
+ break;
+ }
+ break;
+
+ case 0x78: /* VMREAD */
+ case 0x79: /* VMWRITE */
+ return true;
+
+ case 0xc7:
+ /* VMPTRLD, VMPTRST, VMCLEAR, VMXON */
+ if (modrm_mod == 0x03)
+ break;
+
+ if ((modrm_reg == 6 && (!prefix || prefix == 0x66 || prefix == 0xf3)) ||
+ (modrm_reg == 7 && (!prefix || prefix == 0xf3)))
+ return true;
+
+ break;
+
+ default:
+ break;
+ }
+
+ return false;
+}
+
+static int decode_module(struct module *mod, void *text, void *text_end, bool sld_safe)
+{
+ bool allow_vmx = sld_safe || !split_lock_enabled();
+ struct insn insn;
+
+ while (text < text_end) {
+ kernel_insn_init(&insn, text, text_end - text);
+ insn_get_length(&insn);
+
+ if (WARN_ON_ONCE(!insn_complete(&insn))) {
+ pr_err("Module text malformed: %s\n", mod->name);
+ return -ENOEXEC;
+ }
+
+ if (!allow_vmx && insn_is_vmx(&insn)) {
+ pr_err("Module has VMX instructions and is not marked 'sld_safe', boot with: 'split_lock_detect=off': %s\n", mod->name);
+ return -ENOEXEC;
+ }
+
+ text += insn.length;
+ }
+
+ return 0;
+}
+
int module_finalize(const struct load_info *info, struct module *me)
{
const Elf_Ehdr *hdr = info->hdr;
@@ -253,6 +327,16 @@ int module_finalize(const struct load_in
tseg, tseg + text->sh_size);
}
+ if (text && !get_modinfo(info, "intree")) {
+ bool sld_safe = get_modinfo(info, "sld_safe");
+ void *tseg = (void *)text->sh_addr;
+ int ret;
+
+ ret = decode_module(me, tseg, tseg + text->sh_size, sld_safe);
+ if (ret)
+ return ret;
+ }
+
if (para) {
void *pseg = (void *)para->sh_addr;
apply_paravirt(pseg, pseg + para->sh_size);
@@ -261,9 +345,10 @@ int module_finalize(const struct load_in
/* make jump label nops */
jump_label_apply_nops(me);
- if (orc && orc_ip)
+ if (orc && orc_ip) {
unwind_module_init(me, (void *)orc_ip->sh_addr, orc_ip->sh_size,
(void *)orc->sh_addr, orc->sh_size);
+ }
return 0;
}
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -26,6 +26,8 @@ struct load_info {
} index;
};
+extern char *get_modinfo(const struct load_info *info, const char *tag);
+
/* These may be implemented by architectures that need to hook into the
* module loader code. Architectures that don't need to do anything special
* can just rely on the 'weak' default hooks defined in kernel/module.c.
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1393,7 +1393,6 @@ static inline int same_magic(const char
}
#endif /* CONFIG_MODVERSIONS */
-static char *get_modinfo(const struct load_info *info, const char *tag);
static char *get_next_modinfo(const struct load_info *info, const char *tag,
char *prev);
@@ -2521,7 +2520,7 @@ static char *get_next_modinfo(const stru
return NULL;
}
-static char *get_modinfo(const struct load_info *info, const char *tag)
+char *get_modinfo(const struct load_info *info, const char *tag)
{
return get_next_modinfo(info, tag, NULL);
}
On Tue, Apr 07, 2020 at 01:02:39PM +0200, Peter Zijlstra wrote:
> 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
> all out-of-tree modules' text and look for VMX instructions and refuse
> to load it when SLD is enabled (default) and the module isn't marked
> 'sld_safe'.
>
> 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.
What's to keep any out-of-tree module from adding this same module info
"flag" and just lie about it? Isn't that what you are trying to catch
here, or is it a case of, "if you lie, your code will break" as well?
thanks,
greg k-h
On 07/04/20 16:35, Greg KH wrote:
> On Tue, Apr 07, 2020 at 01:02:39PM +0200, Peter Zijlstra wrote:
>> 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
>> all out-of-tree modules' text and look for VMX instructions and refuse
>> to load it when SLD is enabled (default) and the module isn't marked
>> 'sld_safe'.
>>
>> 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.
>
> What's to keep any out-of-tree module from adding this same module info
> "flag" and just lie about it? Isn't that what you are trying to catch
> here, or is it a case of, "if you lie, your code will break" as well?
It's the latter. Basically it's doing _the users_ of out-of-tree
modules a favor by avoiding crashes of their virtual machines;
developers need to fix them anyway.
Paolo
On Tue, 7 Apr 2020 16:35:43 +0200
Greg KH <[email protected]> wrote:
> > 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.
>
> What's to keep any out-of-tree module from adding this same module info
> "flag" and just lie about it? Isn't that what you are trying to catch
> here, or is it a case of, "if you lie, your code will break" as well?
Keeping with the analogy to module kabi breakage, that would basically be
the same as an out of tree module fixing the api but not using it properly.
It will break.
All this is doing is to make sure VM modules that haven't been updated to
handle split lock detection, wont be loaded if split lock detection is
enabled. Saying you can handle SLD and not handling it is just broken code
and we can't really protect against that.
-- Steve
On Tue, Apr 07, 2020 at 04:44:57PM +0200, Paolo Bonzini wrote:
> On 07/04/20 16:35, Greg KH wrote:
> > On Tue, Apr 07, 2020 at 01:02:39PM +0200, Peter Zijlstra wrote:
> >> 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
> >> all out-of-tree modules' text and look for VMX instructions and refuse
> >> to load it when SLD is enabled (default) and the module isn't marked
> >> 'sld_safe'.
> >>
> >> 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.
> >
> > What's to keep any out-of-tree module from adding this same module info
> > "flag" and just lie about it? Isn't that what you are trying to catch
> > here, or is it a case of, "if you lie, your code will break" as well?
>
> It's the latter. Basically it's doing _the users_ of out-of-tree
> modules a favor by avoiding crashes of their virtual machines;
> developers need to fix them anyway.
Ok, seems kind of a heavy hammer, but oh well...
thanks for the explanation.
greg k-h
On Tue, Apr 07, 2020 at 04:35:43PM +0200, Greg KH wrote:
> On Tue, Apr 07, 2020 at 01:02:39PM +0200, Peter Zijlstra wrote:
> > 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
> > all out-of-tree modules' text and look for VMX instructions and refuse
> > to load it when SLD is enabled (default) and the module isn't marked
> > 'sld_safe'.
> >
> > 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.
>
> What's to keep any out-of-tree module from adding this same module info
> "flag" and just lie about it? Isn't that what you are trying to catch
> here, or is it a case of, "if you lie, your code will break" as well?
If they lie they get to keep both pieces.
The thing I worry about is them lying about "intree", is there anything
that avoids that?
On 07/04/20 17:24, Peter Zijlstra wrote:
> The thing I worry about is them lying about "intree", is there anything
> that avoids that?
In QEMU we generate a random-ish number on every build (actually an SHA
of the version and a bunch of other things) and include that in the
modules as the "in tree" marker.
Paolo
On Tue, Apr 07, 2020 at 05:24:12PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 07, 2020 at 04:35:43PM +0200, Greg KH wrote:
> > On Tue, Apr 07, 2020 at 01:02:39PM +0200, Peter Zijlstra wrote:
> > > 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
> > > all out-of-tree modules' text and look for VMX instructions and refuse
> > > to load it when SLD is enabled (default) and the module isn't marked
> > > 'sld_safe'.
> > >
> > > 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.
> >
> > What's to keep any out-of-tree module from adding this same module info
> > "flag" and just lie about it? Isn't that what you are trying to catch
> > here, or is it a case of, "if you lie, your code will break" as well?
>
> If they lie they get to keep both pieces.
>
> The thing I worry about is them lying about "intree", is there anything
> that avoids that?
Yeah, the build system should be enforcing that. I haven't looked in a
while if that really is able to be "faked", but no distro would do that
so it shouldn't be an issue except for custom systems.
greg k-h
Hi Peter,
On Tue, 07 Apr 2020 13:02:39 +0200
Peter Zijlstra <[email protected]> wrote:
> +static bool insn_is_vmx(struct insn *insn)
> +{
> + u8 modrm = insn->modrm.bytes[0];
> + u8 modrm_mod = X86_MODRM_MOD(modrm);
> + u8 modrm_reg = X86_MODRM_REG(modrm);
> +
> + u8 prefix = insn->prefixes.bytes[0];
> +
> + if (insn->opcode.bytes[0] != 0x0f)
> + return false;
> +
> + switch (insn->opcode.bytes[1]) {
> + case 0x01:
> + switch (insn->opcode.bytes[2]) {
> + case 0xc1: /* VMCALL */
> + case 0xc2: /* VMLAUNCH */
> + case 0xc3: /* VMRESUME */
> + case 0xc4: /* VMXOFF */
> + return true;
> +
> + default:
> + break;
> + }
> + break;
> +
> + case 0x78: /* VMREAD */
> + case 0x79: /* VMWRITE */
> + return true;
> +
> + case 0xc7:
> + /* VMPTRLD, VMPTRST, VMCLEAR, VMXON */
> + if (modrm_mod == 0x03)
> + break;
> +
> + if ((modrm_reg == 6 && (!prefix || prefix == 0x66 || prefix == 0xf3)) ||
> + (modrm_reg == 7 && (!prefix || prefix == 0xf3)))
> + return true;
> +
> + break;
> +
> + default:
> + break;
> + }
> +
> + return false;
> +}
OK, so here is what you need ;)
From 36f4f6aec623b0190fde95c8630a6a1d8c23ffc9 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <[email protected]>
Date: Wed, 8 Apr 2020 01:04:41 +0900
Subject: [PATCH] x86: insn: Add insn_is_vmx()
Add insn_is_vmx() to identify the given instruction is
for VMX or not. This is simply identifying those instructions
by mnemonic pattern.
Signed-off-by: Masami Hiramatsu <[email protected]>
---
arch/x86/include/asm/inat.h | 1 +
arch/x86/include/asm/insn.h | 7 +++++++
arch/x86/tools/gen-insn-attr-x86.awk | 6 ++++++
tools/arch/x86/include/asm/inat.h | 1 +
tools/arch/x86/include/asm/insn.h | 7 +++++++
tools/arch/x86/tools/gen-insn-attr-x86.awk | 6 ++++++
6 files changed, 28 insertions(+)
diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index ffce45178c08..599876801ae8 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -79,6 +79,7 @@
#define INAT_EVEXONLY (1 << (INAT_FLAG_OFFS + 7))
#define INAT_FPU (1 << (INAT_FLAG_OFFS + 8))
#define INAT_FPUIFVEX (1 << (INAT_FLAG_OFFS + 9))
+#define INAT_VMX (1 << (INAT_FLAG_OFFS + 10))
/* Attribute making macros for attribute tables */
#define INAT_MAKE_PREFIX(pfx) (pfx << INAT_PFX_OFFS)
#define INAT_MAKE_ESCAPE(esc) (esc << INAT_ESC_OFFS)
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 1752c54d2103..57e81013836d 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -141,6 +141,13 @@ static inline int insn_is_fpu(struct insn *insn)
return 0;
}
+static inline int insn_is_vmx(struct insn *insn)
+{
+ if (!insn->opcode.got)
+ insn_get_opcode(insn);
+ return (insn->attr & INAT_VMX) && !insn_is_fpu(insn);
+}
+
static inline int insn_has_emulate_prefix(struct insn *insn)
{
return !!insn->emulate_prefix_size;
diff --git a/arch/x86/tools/gen-insn-attr-x86.awk b/arch/x86/tools/gen-insn-attr-x86.awk
index d74d9e605723..ade80796453c 100644
--- a/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/arch/x86/tools/gen-insn-attr-x86.awk
@@ -70,6 +70,8 @@ BEGIN {
mmx_expr = "^(emms|fxsave|fxrstor|ldmxcsr|stmxcsr)" # MMX/SSE nmemonics lacking operands
fpu_expr = "^x87"
+ vmx_expr = "^VM.*" # All mnemonic start with "VM" are VMX instructions
+
lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
lprefix3_expr = "\\((F2|!F3|66&F2)\\)"
@@ -328,6 +330,10 @@ function convert_operands(count,opnd, i,j,imm,mod,mmx)
if (match(ext, force64_expr))
flags = add_flags(flags, "INAT_FORCE64")
+ # check VMX related opcode
+ if (match(opcode, vmx_expr))
+ flags = add_flags(flags, "INAT_VMX")
+
# check REX prefix
if (match(opcode, rex_expr))
flags = add_flags(flags, "INAT_MAKE_PREFIX(INAT_PFX_REX)")
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 2e6a05290efd..af393952916c 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -79,6 +79,7 @@
#define INAT_EVEXONLY (1 << (INAT_FLAG_OFFS + 7))
#define INAT_FPU (1 << (INAT_FLAG_OFFS + 8))
#define INAT_FPUIFVEX (1 << (INAT_FLAG_OFFS + 9))
+#define INAT_VMX (1 << (INAT_FLAG_OFFS + 10))
/* Attribute making macros for attribute tables */
#define INAT_MAKE_PREFIX(pfx) (pfx << INAT_PFX_OFFS)
#define INAT_MAKE_ESCAPE(esc) (esc << INAT_ESC_OFFS)
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index d9f6bd9059c1..d18ce4683d8e 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -141,6 +141,13 @@ static inline int insn_is_fpu(struct insn *insn)
return 0;
}
+static inline int insn_is_vmx(struct insn *insn)
+{
+ if (!insn->opcode.got)
+ insn_get_opcode(insn);
+ return (insn->attr & INAT_VMX) && !insn_is_fpu(insn);
+}
+
static inline int insn_has_emulate_prefix(struct insn *insn)
{
return !!insn->emulate_prefix_size;
diff --git a/tools/arch/x86/tools/gen-insn-attr-x86.awk b/tools/arch/x86/tools/gen-insn-attr-x86.awk
index d74d9e605723..ade80796453c 100644
--- a/tools/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/tools/arch/x86/tools/gen-insn-attr-x86.awk
@@ -70,6 +70,8 @@ BEGIN {
mmx_expr = "^(emms|fxsave|fxrstor|ldmxcsr|stmxcsr)" # MMX/SSE nmemonics lacking operands
fpu_expr = "^x87"
+ vmx_expr = "^VM.*" # All mnemonic start with "VM" are VMX instructions
+
lprefix1_expr = "\\((66|!F3)\\)"
lprefix2_expr = "\\(F3\\)"
lprefix3_expr = "\\((F2|!F3|66&F2)\\)"
@@ -328,6 +330,10 @@ function convert_operands(count,opnd, i,j,imm,mod,mmx)
if (match(ext, force64_expr))
flags = add_flags(flags, "INAT_FORCE64")
+ # check VMX related opcode
+ if (match(opcode, vmx_expr))
+ flags = add_flags(flags, "INAT_VMX")
+
# check REX prefix
if (match(opcode, rex_expr))
flags = add_flags(flags, "INAT_MAKE_PREFIX(INAT_PFX_REX)")
--
2.20.1
--
Masami Hiramatsu <[email protected]>
On 07/04/2020 17:51, Masami Hiramatsu wrote:
> diff --git a/tools/arch/x86/tools/gen-insn-attr-x86.awk b/tools/arch/x86/tools/gen-insn-attr-x86.awk
> index d74d9e605723..ade80796453c 100644
> --- a/tools/arch/x86/tools/gen-insn-attr-x86.awk
> +++ b/tools/arch/x86/tools/gen-insn-attr-x86.awk
> @@ -70,6 +70,8 @@ BEGIN {
> mmx_expr = "^(emms|fxsave|fxrstor|ldmxcsr|stmxcsr)" # MMX/SSE nmemonics lacking operands
> fpu_expr = "^x87"
>
> + vmx_expr = "^VM.*" # All mnemonic start with "VM" are VMX instructions
Not really.
VMMCALL, VMLOAD, VMSAVE and VMRUN are SVM instructions.
VMASKMOV is a AVX instruction.
~Andrew
Hi Peter,
I love your patch! Yet something to improve:
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on linus/master next-20200407]
[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/Peter-Zijlstra/x86-module-Out-of-tree-module-decode-and-sanitize/20200408-011239
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git ee8bac724cc7767dcf9480afb656318994f22c3d
config: um-x86_64_defconfig (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=um SUBARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
/usr/bin/ld: arch/x86/um/../kernel/module.o: in function `decode_module':
>> module.c:(.text+0x26): undefined reference to `split_lock_enabled'
>> /usr/bin/ld: module.c:(.text+0x39): undefined reference to `insn_init'
>> /usr/bin/ld: module.c:(.text+0x6b): undefined reference to `insn_get_length'
collect2: error: ld returned 1 exit status
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
From: Peter Zijlstra
> Sent: 07 April 2020 12:03
>
> 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
> all out-of-tree modules' text and look for VMX instructions and refuse
> to load it when SLD is enabled (default) and the module isn't marked
> 'sld_safe'.
...
> + while (text < text_end) {
> + kernel_insn_init(&insn, text, text_end - text);
> + insn_get_length(&insn);
> +
> + if (WARN_ON_ONCE(!insn_complete(&insn))) {
> + pr_err("Module text malformed: %s\n", mod->name);
> + return -ENOEXEC;
> + }
> +
> + if (!allow_vmx && insn_is_vmx(&insn)) {
> + pr_err("Module has VMX instructions and is not marked 'sld_safe', boot with:
> 'split_lock_detect=off': %s\n", mod->name);
> + return -ENOEXEC;
> + }
> +
> + text += insn.length;
> + }
There is a slight flaw in the above.
A malicious module can hide the required instruction by jumping into the
middle of a long instruction.
Even checking branch targets hit instruction barriers isn't enough,
an indirect jump could be used.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Apr 07, 2020 at 09:25:56PM +0000, David Laight wrote:
> From: Peter Zijlstra
> > Sent: 07 April 2020 12:03
> >
> > 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
> > all out-of-tree modules' text and look for VMX instructions and refuse
> > to load it when SLD is enabled (default) and the module isn't marked
> > 'sld_safe'.
> ...
> > + while (text < text_end) {
> > + kernel_insn_init(&insn, text, text_end - text);
> > + insn_get_length(&insn);
> > +
> > + if (WARN_ON_ONCE(!insn_complete(&insn))) {
> > + pr_err("Module text malformed: %s\n", mod->name);
> > + return -ENOEXEC;
> > + }
> > +
> > + if (!allow_vmx && insn_is_vmx(&insn)) {
> > + pr_err("Module has VMX instructions and is not marked 'sld_safe', boot with:
> > 'split_lock_detect=off': %s\n", mod->name);
> > + return -ENOEXEC;
> > + }
> > +
> > + text += insn.length;
> > + }
>
> There is a slight flaw in the above.
> A malicious module can hide the required instruction by jumping into the
> middle of a long instruction.
>
> Even checking branch targets hit instruction barriers isn't enough,
> an indirect jump could be used.
If I understand the goals here, it's to provide feedback for good actors
doing things that they don't realize aren't safe. Trying to stop a
malicious module from doing malicious things is basically impossible:
it can just load a data blob and self-modify, etc. :)
Though, Peter, this does get me thinking: if this is meant to be helpful
for module authors tripping over things they shouldn't be touching,
perhaps every test needs to include explicit recommendations? It's
_kind_ of doing this already. Maybe the above should be:
pr_err("%s: contains VMX instructions but is not marked 'sld_safe'. Please see <url> or boot with: 'split_lock_detect=off' to ignore.\n", mod->name);
?
--
Kees Cook
Hi Andrew,
On Tue, 7 Apr 2020 18:16:58 +0100
Andrew Cooper <[email protected]> wrote:
> On 07/04/2020 17:51, Masami Hiramatsu wrote:
> > diff --git a/tools/arch/x86/tools/gen-insn-attr-x86.awk b/tools/arch/x86/tools/gen-insn-attr-x86.awk
> > index d74d9e605723..ade80796453c 100644
> > --- a/tools/arch/x86/tools/gen-insn-attr-x86.awk
> > +++ b/tools/arch/x86/tools/gen-insn-attr-x86.awk
> > @@ -70,6 +70,8 @@ BEGIN {
> > mmx_expr = "^(emms|fxsave|fxrstor|ldmxcsr|stmxcsr)" # MMX/SSE nmemonics lacking operands
> > fpu_expr = "^x87"
> >
> > + vmx_expr = "^VM.*" # All mnemonic start with "VM" are VMX instructions
>
> Not really.
>
> VMMCALL, VMLOAD, VMSAVE and VMRUN are SVM instructions.
Here VMX will include SVM instructions. Would we need to distinguish them in this context?
(Or INAT_VIRT might be politically correct :) )
> VMASKMOV is a AVX instruction.
Good point. That instruction is written in lowercase "vmaskmov" in x86-opcode-map.txt.
(Maybe it is better to note it in x86-opcode-map.txt)
Thank you,
--
Masami Hiramatsu <[email protected]>
On 4/7/2020 7:02 PM, Peter Zijlstra wrote:
> 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
> all out-of-tree modules' text and look for VMX instructions and refuse
> to load it when SLD is enabled (default) and the module isn't marked
> 'sld_safe'.
>
> 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.
>
I'm thinking that it helps nothing other than telling the possible
hypervisors "we have a new feature SLD enabled in kernel, but you seem
not aware of it. To avoid something wrong with you and your VMs, you are
not allowed to be loaded. Please tell me sld_safe as your assurance to
get approval"
It's actually the bug/issue of hypervisor and it does no harm to (host)
kernel. We can just leave it to hypervisor developer that they need to
fix the bug in their hypervisor.
If we go the way like this patch, then whenever someone reports a
similar bug due to new feature introduced and enabled in the future, we
add a new xxx_safe module info?
On Wed, 8 Apr 2020 01:51:24 +0900
Masami Hiramatsu <[email protected]> wrote:
> Hi Peter,
>
> On Tue, 07 Apr 2020 13:02:39 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > +static bool insn_is_vmx(struct insn *insn)
> > +{
> > + u8 modrm = insn->modrm.bytes[0];
> > + u8 modrm_mod = X86_MODRM_MOD(modrm);
> > + u8 modrm_reg = X86_MODRM_REG(modrm);
> > +
> > + u8 prefix = insn->prefixes.bytes[0];
> > +
> > + if (insn->opcode.bytes[0] != 0x0f)
> > + return false;
> > +
> > + switch (insn->opcode.bytes[1]) {
> > + case 0x01:
> > + switch (insn->opcode.bytes[2]) {
> > + case 0xc1: /* VMCALL */
> > + case 0xc2: /* VMLAUNCH */
> > + case 0xc3: /* VMRESUME */
> > + case 0xc4: /* VMXOFF */
> > + return true;
> > +
> > + default:
> > + break;
> > + }
> > + break;
> > +
> > + case 0x78: /* VMREAD */
> > + case 0x79: /* VMWRITE */
> > + return true;
> > +
> > + case 0xc7:
> > + /* VMPTRLD, VMPTRST, VMCLEAR, VMXON */
> > + if (modrm_mod == 0x03)
> > + break;
> > +
> > + if ((modrm_reg == 6 && (!prefix || prefix == 0x66 || prefix == 0xf3)) ||
> > + (modrm_reg == 7 && (!prefix || prefix == 0xf3)))
> > + return true;
> > +
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + return false;
> > +}
>
> OK, so here is what you need ;)
>
> From 36f4f6aec623b0190fde95c8630a6a1d8c23ffc9 Mon Sep 17 00:00:00 2001
> From: Masami Hiramatsu <[email protected]>
> Date: Wed, 8 Apr 2020 01:04:41 +0900
> Subject: [PATCH] x86: insn: Add insn_is_vmx()
>
> Add insn_is_vmx() to identify the given instruction is
> for VMX or not. This is simply identifying those instructions
> by mnemonic pattern.
>
Hmm, I found that this is still not enough... since the inat_tables
mixes the instruction attributes on same entry in group tables.
It distinguishes opcodes by last-prefix variants, but not by
MOD & R/M bits, since it is designed only for decoding instructions.
Thank you,
--
Masami Hiramatsu <[email protected]>
On Tue, 07 Apr 2020 13:02:39 +0200
Peter Zijlstra <[email protected]> wrote:
> +static bool insn_is_vmx(struct insn *insn)
> +{
> + u8 modrm = insn->modrm.bytes[0];
> + u8 modrm_mod = X86_MODRM_MOD(modrm);
> + u8 modrm_reg = X86_MODRM_REG(modrm);
> +
> + u8 prefix = insn->prefixes.bytes[0];
This should be the last prefix,
u8 prefix = insn->prefixes.bytes[3];
(The last prefix always copied on the bytes[3])
> +
> + if (insn->opcode.bytes[0] != 0x0f)
> + return false;
> +
> + switch (insn->opcode.bytes[1]) {
> + case 0x01:
> + switch (insn->opcode.bytes[2]) {
Sorry, VMCALL etc. is in Grp7 (0f 01), the 3rd code is embedded
in modrm instead of opcode. Thus it should be,
switch (insn->modrm.value) {
> + case 0xc1: /* VMCALL */
> + case 0xc2: /* VMLAUNCH */
> + case 0xc3: /* VMRESUME */
> + case 0xc4: /* VMXOFF */
case 0xd4: /* VMFUNC */
> + return true;
> +
> + default:
> + break;
> + }
> + break;
> +
> + case 0x78: /* VMREAD */
> + case 0x79: /* VMWRITE */
return !insn_is_evex(insn);
With EVEX prefix, these becomes vcvt* instructions.
> + case 0xc7:
> + /* VMPTRLD, VMPTRST, VMCLEAR, VMXON */
> + if (modrm_mod == 0x03)
> + break;
> +
> + if ((modrm_reg == 6 && (!prefix || prefix == 0x66 || prefix == 0xf3)) ||
> + (modrm_reg == 7 && (!prefix || prefix == 0xf3)))
> + return true;
> +
> + break;
> +
> + default:
> + break;
> + }
> +
> + return false;
> +}
Thank you,
--
Masami Hiramatsu <[email protected]>
On 08/04/2020 10:56, Peter Zijlstra wrote:
>>> + case 0xc1: /* VMCALL */
>>> + case 0xc2: /* VMLAUNCH */
>>> + case 0xc3: /* VMRESUME */
>>> + case 0xc4: /* VMXOFF */
>> case 0xd4: /* VMFUNC */
> As per Andrew, VMCALL and VMFUNC are SMV, and I really only need VMX in
> this case. Including SMV is probably harmless, but I'm thinking a
> smaller function is better.
VMCALL and VMFUNC are both VMX instructions. VMMCALL (count the M's -
yes it is a different instruction) is SVM, and I forgot VMGEXIT from the
list yesterday which is also SVM.
However, searching for them is probably not helpful. They are all
guest-only instructions and you wouldn't expect to see them in
hypervisor code.
~Andrew
On Wed, Apr 08, 2020 at 05:09:34PM +0900, Masami Hiramatsu wrote:
> On Tue, 07 Apr 2020 13:02:39 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > +static bool insn_is_vmx(struct insn *insn)
> > +{
> > + u8 modrm = insn->modrm.bytes[0];
> > + u8 modrm_mod = X86_MODRM_MOD(modrm);
> > + u8 modrm_reg = X86_MODRM_REG(modrm);
> > +
> > + u8 prefix = insn->prefixes.bytes[0];
>
> This should be the last prefix,
>
> u8 prefix = insn->prefixes.bytes[3];
>
> (The last prefix always copied on the bytes[3])
And that is 0 on no-prefix, right?
> > +
> > + if (insn->opcode.bytes[0] != 0x0f)
> > + return false;
> > +
> > + switch (insn->opcode.bytes[1]) {
> > + case 0x01:
> > + switch (insn->opcode.bytes[2]) {
>
> Sorry, VMCALL etc. is in Grp7 (0f 01), the 3rd code is embedded
> in modrm instead of opcode. Thus it should be,
>
> switch (insn->modrm.value) {
Indeed, I was hoping (I really should've checked) that that byte was
duplicated in opcodes.
Also, since I already have modrm = insn->modrm.bytes[0], I should
probably use that anyway.
> > + case 0xc1: /* VMCALL */
> > + case 0xc2: /* VMLAUNCH */
> > + case 0xc3: /* VMRESUME */
> > + case 0xc4: /* VMXOFF */
>
> case 0xd4: /* VMFUNC */
As per Andrew, VMCALL and VMFUNC are SMV, and I really only need VMX in
this case. Including SMV is probably harmless, but I'm thinking a
smaller function is better.
> > + return true;
> > +
> > + default:
> > + break;
> > + }
> > + break;
> > +
> > + case 0x78: /* VMREAD */
> > + case 0x79: /* VMWRITE */
>
> return !insn_is_evex(insn);
>
> With EVEX prefix, these becomes vcvt* instructions.
Thanks!
On Wed, 8 Apr 2020 11:56:04 +0200
Peter Zijlstra <[email protected]> wrote:
> On Wed, Apr 08, 2020 at 05:09:34PM +0900, Masami Hiramatsu wrote:
> > On Tue, 07 Apr 2020 13:02:39 +0200
> > Peter Zijlstra <[email protected]> wrote:
> >
> > > +static bool insn_is_vmx(struct insn *insn)
> > > +{
> > > + u8 modrm = insn->modrm.bytes[0];
> > > + u8 modrm_mod = X86_MODRM_MOD(modrm);
> > > + u8 modrm_reg = X86_MODRM_REG(modrm);
> > > +
> > > + u8 prefix = insn->prefixes.bytes[0];
> >
> > This should be the last prefix,
> >
> > u8 prefix = insn->prefixes.bytes[3];
> >
> > (The last prefix always copied on the bytes[3])
>
> And that is 0 on no-prefix, right?
Yes, it should be.
> > > +
> > > + if (insn->opcode.bytes[0] != 0x0f)
> > > + return false;
> > > +
> > > + switch (insn->opcode.bytes[1]) {
> > > + case 0x01:
> > > + switch (insn->opcode.bytes[2]) {
> >
> > Sorry, VMCALL etc. is in Grp7 (0f 01), the 3rd code is embedded
> > in modrm instead of opcode. Thus it should be,
> >
> > switch (insn->modrm.value) {
>
> Indeed, I was hoping (I really should've checked) that that byte was
> duplicated in opcodes.
>
> Also, since I already have modrm = insn->modrm.bytes[0], I should
> probably use that anyway.
Yeah, and please use modrm.value instead of bytes[0].
(maybe bytes[0] will be OK since x86 is little-endian)
> > > + case 0xc1: /* VMCALL */
> > > + case 0xc2: /* VMLAUNCH */
> > > + case 0xc3: /* VMRESUME */
> > > + case 0xc4: /* VMXOFF */
> >
> > case 0xd4: /* VMFUNC */
>
> As per Andrew, VMCALL and VMFUNC are SMV, and I really only need VMX in
> this case. Including SMV is probably harmless, but I'm thinking a
> smaller function is better.
I got it.
>
> > > + return true;
> > > +
> > > + default:
> > > + break;
> > > + }
> > > + break;
> > > +
> > > + case 0x78: /* VMREAD */
> > > + case 0x79: /* VMWRITE */
> >
> > return !insn_is_evex(insn);
> >
> > With EVEX prefix, these becomes vcvt* instructions.
>
> Thanks!
Thank you,
--
Masami Hiramatsu <[email protected]>