[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/4] x86/vmx: introduce vmwrite_safe()
>>> On 06.02.17 at 15:57, <sergey.dyasli@xxxxxxxxxx> wrote: > 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 copied > into asm_defns.h > > Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx> > --- Please can you have the revision info for the individual patches here. I know you've put it in the overview mail, but for reviewers it's far more useful to (also) be here. > --- 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 = ~0, > }; The main reason for me to ask for the type change here was to ... > @@ -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\t", > + VMWRITE_OPCODE MODRM_EAX_ECX) > + ASM_FLAG_OUT(, "setc %[invalid]\n\t") > + ASM_FLAG_OUT(, "setz %[valid]\n\t") > + : ASM_FLAG_OUT("=@ccc", [invalid] "=rm") (fail_invalid), > + ASM_FLAG_OUT("=@ccz", [valid] "=rm") (fail_valid) > + : [field] GAS_VMX_OP("r", "a") (field), > + [value] GAS_VMX_OP("rm", "c") (value)); > + > + if ( unlikely(fail_invalid) ) > + ret = VMX_INSN_FAIL_INVALID; > + else if ( unlikely(fail_valid) ) > + __vmread(VM_INSTRUCTION_ERROR, &ret); > + > + return ret; > +} ... allow the function to return enum vmx_insn_errno, and that to not be a 64-bit quantity. As you're presumably aware, dealing with 32-bit quantities is on the average slightly more efficient than dealing with 64-bit ones. The code above should imo still BUG() if the value read from VM_INSTRUCTION_ERROR doesn't fit in 32 bits (as it's a 32-bit field only anyway). Also, looking at the entire asm() above another time, wouldn't it end up a bit less convoluted if you simply used CMOVC for the "invalid" code path? Similarly I wonder whether the second VMREAD wouldn't better be moved into the asm(), utilizing the UNLIKELY_START() et al constructs to get that code path entirely out of the main path. These aren't requirements though, just aspects to think about. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |