| Message ID | 1485861635-3473-2-git-send-email-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote: > --- a/tools/tests/x86_emulator/x86_emulate.h > +++ b/tools/tests/x86_emulator/x86_emulate.h > @@ -46,6 +46,12 @@ > #define MMAP_SZ 16384 > bool emul_test_make_stack_executable(void); > > +#ifdef __GCC_ASM_FLAG_OUTPUTS__ > +# define ASM_FLAG_OUT(yes, no) yes > +#else > +# define ASM_FLAG_OUT(yes, no) no > +#endif I ought to be sufficient to put this in tools/tests/x86_emulator/x86_emulate.c - no need for other consumers of this header to see it. > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h > @@ -526,6 +526,7 @@ enum vmx_insn_errno > VMX_INSN_VMPTRLD_INVALID_PHYADDR = 9, > VMX_INSN_UNSUPPORTED_VMCS_COMPONENT = 12, > VMX_INSN_VMXON_IN_VMX_ROOT = 15, > + VMX_INSN_FAIL_INVALID = ~0UL, > }; Is the UL really need here? I'd think -1, ~0, or ~0U to suffice. > @@ -423,6 +429,29 @@ static inline bool_t __vmread_safe(unsigned long field, unsigned long *value) > return okay; > } > > +static always_inline unsigned long __vmwrite_safe(unsigned long field, > + unsigned long value) Can we please avoid adding more of these (even double) underscore prefixed symbols? They're reserved to the compiler, so we should limit their use to places where we absolutely can't think of better alternatives. > +{ > + unsigned long ret = 0; > + bool fail_invalid, fail_valid; > + > + asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n", > + VMWRITE_OPCODE MODRM_EAX_ECX) > + ASM_FLAG_OUT(, "setb %[fail_invalid]\n") While they're synonyms, I'd prefer setc (and @ccc below) to be used here, as this is not the result of a comparison you're looking at. > + ASM_FLAG_OUT(, "sete %[fail_valid]\n") Similarly setz / @ccz here / below Also I think for the constraint names (but not the variable ones) you could omit the fail_ prefixes, to help readability. Plus, strictly speaking it is wrong for instruction mnemonics to start in column zero. Granted we have many violations of this, but please add \t here to avoid introducing more slightly wrong code. > + : ASM_FLAG_OUT("=@ccb", [fail_invalid] "=rm") (fail_invalid), > + ASM_FLAG_OUT("=@cce", [fail_valid] "=rm") (fail_valid) > + : [field] GAS_VMX_OP("r", "a") (field), > + [value] GAS_VMX_OP("rm", "c") (value)); > + > + if ( fail_invalid ) > + ret = VMX_INSN_FAIL_INVALID; > + else if ( fail_valid ) > + __vmread(VM_INSTRUCTION_ERROR, &ret); If already you don't fold this into the asm(), please at least add unlikely(). > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -162,4 +162,10 @@ void init_constructors(void); > void *bsearch(const void *key, const void *base, size_t num, size_t size, > int (*cmp)(const void *key, const void *elt)); > > +#ifdef __GCC_ASM_FLAG_OUTPUTS__ > +# define ASM_FLAG_OUT(yes, no) yes > +#else > +# define ASM_FLAG_OUT(yes, no) no > +#endif Is this useful on other than x86? Jan
diff --git a/tools/tests/x86_emulator/x86_emulate.h b/tools/tests/x86_emulator/x86_emulate.h index 3a6bade..6524bb3 100644 --- a/tools/tests/x86_emulator/x86_emulate.h +++ b/tools/tests/x86_emulator/x86_emulate.h @@ -46,6 +46,12 @@ #define MMAP_SZ 16384 bool emul_test_make_stack_executable(void); +#ifdef __GCC_ASM_FLAG_OUTPUTS__ +# define ASM_FLAG_OUT(yes, no) yes +#else +# define ASM_FLAG_OUT(yes, no) no +#endif + #include "x86_emulate/x86_emulate.h" static inline uint64_t xgetbv(uint32_t xcr) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 9caebe5..105a3c0 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -483,7 +483,8 @@ static void vmfail_invalid(struct cpu_user_regs *regs) static void vmfail(struct cpu_user_regs *regs, enum vmx_insn_errno errno) { - if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR ) + if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR && + errno != VMX_INSN_FAIL_INVALID ) vmfail_valid(regs, errno); else vmfail_invalid(regs); diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 5bb5bdf..d133f09 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -409,12 +409,6 @@ typedef union { (void *)((long)(__##var + __alignof(type) - __alignof(long)) \ & -__alignof(type)) -#ifdef __GCC_ASM_FLAG_OUTPUTS__ -# define ASM_FLAG_OUT(yes, no) yes -#else -# define ASM_FLAG_OUT(yes, no) no -#endif - /* MSRs. */ #define MSR_TSC 0x00000010 #define MSR_SYSENTER_CS 0x00000174 diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h index d71de04..cb02d53 100644 --- a/xen/include/asm-x86/hvm/vmx/vmcs.h +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h @@ -526,6 +526,7 @@ enum vmx_insn_errno VMX_INSN_VMPTRLD_INVALID_PHYADDR = 9, VMX_INSN_UNSUPPORTED_VMCS_COMPONENT = 12, VMX_INSN_VMXON_IN_VMX_ROOT = 15, + VMX_INSN_FAIL_INVALID = ~0UL, }; void vmx_disable_intercept_for_msr(struct vcpu *v, u32 msr, int type); diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index e5c6499..b0f3f3c 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -306,6 +306,12 @@ extern uint8_t posted_intr_vector; #define INVVPID_ALL_CONTEXT 2 #define INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 3 +#ifdef HAVE_GAS_VMX +# define GAS_VMX_OP(yes, no) yes +#else +# define GAS_VMX_OP(yes, no) no +#endif + static always_inline void __vmptrld(u64 addr) { asm volatile ( @@ -423,6 +429,29 @@ static inline bool_t __vmread_safe(unsigned long field, unsigned long *value) return okay; } +static always_inline unsigned long __vmwrite_safe(unsigned long field, + unsigned long value) +{ + unsigned long ret = 0; + bool fail_invalid, fail_valid; + + asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n", + VMWRITE_OPCODE MODRM_EAX_ECX) + ASM_FLAG_OUT(, "setb %[fail_invalid]\n") + ASM_FLAG_OUT(, "sete %[fail_valid]\n") + : ASM_FLAG_OUT("=@ccb", [fail_invalid] "=rm") (fail_invalid), + ASM_FLAG_OUT("=@cce", [fail_valid] "=rm") (fail_valid) + : [field] GAS_VMX_OP("r", "a") (field), + [value] GAS_VMX_OP("rm", "c") (value)); + + if ( fail_invalid ) + ret = VMX_INSN_FAIL_INVALID; + else if ( fail_valid ) + __vmread(VM_INSTRUCTION_ERROR, &ret); + + return ret; +} + static always_inline void __invept(unsigned long type, u64 eptp, u64 gpa) { struct { diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index d1171b7..6f6cac2 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -162,4 +162,10 @@ void init_constructors(void); void *bsearch(const void *key, const void *base, size_t num, size_t size, int (*cmp)(const void *key, const void *elt)); +#ifdef __GCC_ASM_FLAG_OUTPUTS__ +# define ASM_FLAG_OUT(yes, no) yes +#else +# define ASM_FLAG_OUT(yes, no) no +#endif + #endif /* __LIB_H__ */
Any fail during the original __vmwrite() leads to BUG() which can be easily exploited from a guest in the nested vmx mode. The new function returns error code depending on the outcome: VMsucceed: 0 VMfailValid: VM Instruction Error Number VMfailInvalid: a new VMX_INSN_FAIL_INVALID A new macro GAS_VMX_OP is introduced in order to improve the readability of asm. Existing ASM_FLAG_OUT macro is reused and moved into lib.h and x86_emulate.h Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- tools/tests/x86_emulator/x86_emulate.h | 6 ++++++ xen/arch/x86/hvm/vmx/vvmx.c | 3 ++- xen/arch/x86/x86_emulate/x86_emulate.c | 6 ------ xen/include/asm-x86/hvm/vmx/vmcs.h | 1 + xen/include/asm-x86/hvm/vmx/vmx.h | 29 +++++++++++++++++++++++++++++ xen/include/xen/lib.h | 6 ++++++ 6 files changed, 44 insertions(+), 7 deletions(-)