Re: [Xen-devel] [PATCH v2 1/4] x86/vmx: introduce vmwrite_safe()

[Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>]

On Tue, 2017-02-07 at 04:09 -0700, Jan Beulich wrote:
> > > > 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).

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);
+    }
 
-    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?

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel