| Message ID | 1486480006.3301.3.camel@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/02/17 15:06, Sergey Dyasli wrote: > On Tue, 2017-02-07 at 04:09 -0700, Jan Beulich wrote: >>>>> On 06.02.17 at 15:57, <sergey.dyasli@citrix.com> 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@citrix.com> >>> --- >> 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). > If I understood correctly, you are suggesting the following change: > > diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h > index 24fbbd4..f9b3bf1 100644 > --- a/xen/include/asm-x86/hvm/vmx/vmx.h > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h > @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long field, > return ret; > } > > -static always_inline unsigned long vmwrite_safe(unsigned long field, > - unsigned long value) > +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field, > + unsigned long value) > { > unsigned long ret = 0; > bool fail_invalid, fail_valid; > @@ -440,11 +440,16 @@ static always_inline unsigned long vmwrite_safe(unsigned long 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); > + BUG_ON(ret >= ~0U); I really don't think the BUG_ON() is necessary. If hardware already guarentees to hand us back a 32bit quantity, and if hardware is malfunctioning, we have already lost. Also, this BUG_ON() will prevent inlining the function if alway_inline is reduced to inline (which is a good idea). ~Andrew > + } > > - return ret; > + return (enum vmx_insn_errno) ret; > } > > And I have noticed one inconsistency: vmwrite_safe() is "always_inline" > while vmread_safe() is plain "inline". I believe that plain inline is > enough here, what do you think? >
>>> On 07.02.17 at 16:06, <sergey.dyasli@citrix.com> wrote: > If I understood correctly, you are suggesting the following change: Mostly. > --- a/xen/include/asm-x86/hvm/vmx/vmx.h > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h > @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long field, > return ret; > } > > -static always_inline unsigned long vmwrite_safe(unsigned long field, > - unsigned long value) > +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field, > + unsigned long value) > { > unsigned long ret = 0; > bool fail_invalid, fail_valid; > @@ -440,11 +440,16 @@ static always_inline unsigned long vmwrite_safe(unsigned long field, > [value] GAS_VMX_OP("rm", "c") (value)); > > if ( unlikely(fail_invalid) ) > + { > ret = VMX_INSN_FAIL_INVALID; > + } No need to add braces here and ... > else if ( unlikely(fail_valid) ) > + { > __vmread(VM_INSTRUCTION_ERROR, &ret); > + BUG_ON(ret >= ~0U); > + } > > - return ret; > + return (enum vmx_insn_errno) ret; ... no need for the cast here. (See Andrew's reply for the BUG_ON().) > And I have noticed one inconsistency: vmwrite_safe() is "always_inline" > while vmread_safe() is plain "inline". I believe that plain inline is > enough here, what do you think? I would assume plain inline to be enough, but maybe the VMX maintainers know why always_inline was used. Jan
On 07/02/17 16:22, Jan Beulich wrote: >>>> On 07.02.17 at 16:06, <sergey.dyasli@citrix.com> wrote: >> If I understood correctly, you are suggesting the following change: > Mostly. > >> --- a/xen/include/asm-x86/hvm/vmx/vmx.h >> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h >> @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long field, >> return ret; >> } >> >> -static always_inline unsigned long vmwrite_safe(unsigned long field, >> - unsigned long value) >> +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field, >> + unsigned long value) >> { >> unsigned long ret = 0; >> bool fail_invalid, fail_valid; >> @@ -440,11 +440,16 @@ static always_inline unsigned long vmwrite_safe(unsigned long field, >> [value] GAS_VMX_OP("rm", "c") (value)); >> >> if ( unlikely(fail_invalid) ) >> + { >> ret = VMX_INSN_FAIL_INVALID; >> + } > No need to add braces here and ... > >> else if ( unlikely(fail_valid) ) >> + { >> __vmread(VM_INSTRUCTION_ERROR, &ret); >> + BUG_ON(ret >= ~0U); >> + } >> >> - return ret; >> + return (enum vmx_insn_errno) ret; > ... no need for the cast here. (See Andrew's reply for the BUG_ON().) > >> And I have noticed one inconsistency: vmwrite_safe() is "always_inline" >> while vmread_safe() is plain "inline". I believe that plain inline is >> enough here, what do you think? > I would assume plain inline to be enough, but maybe the VMX > maintainers know why always_inline was used. The always_inline was my doing IIRC, because the use of unlikely sections caused GCC to create a separate identical functions in each translation unit, in an attempt to minimise the quantity of out-of-line code. ~Andrew
>>> On 07.02.17 at 17:34, <andrew.cooper3@citrix.com> wrote: > On 07/02/17 16:22, Jan Beulich wrote: >>>>> On 07.02.17 at 16:06, <sergey.dyasli@citrix.com> wrote: >>> If I understood correctly, you are suggesting the following change: >> Mostly. >> >>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h >>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h >>> @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long > field, >>> return ret; >>> } >>> >>> -static always_inline unsigned long vmwrite_safe(unsigned long field, >>> - unsigned long value) >>> +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field, >>> + unsigned long value) >>> { >>> unsigned long ret = 0; >>> bool fail_invalid, fail_valid; >>> @@ -440,11 +440,16 @@ static always_inline unsigned long > vmwrite_safe(unsigned long field, >>> [value] GAS_VMX_OP("rm", "c") (value)); >>> >>> if ( unlikely(fail_invalid) ) >>> + { >>> ret = VMX_INSN_FAIL_INVALID; >>> + } >> No need to add braces here and ... >> >>> else if ( unlikely(fail_valid) ) >>> + { >>> __vmread(VM_INSTRUCTION_ERROR, &ret); >>> + BUG_ON(ret >= ~0U); >>> + } >>> >>> - return ret; >>> + return (enum vmx_insn_errno) ret; >> ... no need for the cast here. (See Andrew's reply for the BUG_ON().) >> >>> And I have noticed one inconsistency: vmwrite_safe() is "always_inline" >>> while vmread_safe() is plain "inline". I believe that plain inline is >>> enough here, what do you think? >> I would assume plain inline to be enough, but maybe the VMX >> maintainers know why always_inline was used. > > The always_inline was my doing IIRC, because the use of unlikely > sections caused GCC to create a separate identical functions in each > translation unit, in an attempt to minimise the quantity of out-of-line > code. In which case it's not needed in these new flavors. Jan
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index 24fbbd4..f9b3bf1 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long field, return ret; } -static always_inline unsigned long vmwrite_safe(unsigned long field, - unsigned long value) +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field, + unsigned long value) { unsigned long ret = 0; bool fail_invalid, fail_valid; @@ -440,11 +440,16 @@ static always_inline unsigned long vmwrite_safe(unsigned long 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); + BUG_ON(ret >= ~0U); + } - return ret; + return (enum vmx_insn_errno) ret; } And I have noticed one inconsistency: vmwrite_safe() is "always_inline"