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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@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);

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?
>


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