Re: [Xen-devel] [PATCH] x86/vvmx: Improvements to INVEPT instruction handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/02/17 10:19, Jan Beulich wrote:
>  >>> On 06.02.17 at 17:55, <andrew.cooper3@xxxxxxxxxx> wrote:
>> * Latch current once at the start.
>>  * Avoid the memory operand read for INVEPT_ALL_CONTEXT.  Experimentally, 
>> this
>>    is how hardware behaves, and avoids an unnecessary pagewalk.
> Hmm, so you say #GP is being raised for all possible reasons, but
> #PF can't result here? That would be pretty unusual instruction
> semantics. But if it's that way (to be confirmed by Intel), and ...

No.  The memory operand is entirely ignored.  No #PF, or #GP or #SS for
bad segment or canonical setups.

>
>>  * Reject Reg/Reg encodings of the instruction.
> ... if this is possible to occur at all (I'd expect #UD to result instead
> of a VM exit), then ...

I'd hope so as well.  This addition is partly from an entirely emulation
point of view, as well as a proper sanity check of the decode.mem union
before use.

>
>>  * Audit eptp against maxphysaddr.
>>  * Introduce and use VMX_INSN_INVALID_INV_OPERAND to correct the vmfail
>>    semantics.
>>  * Add extra newlines for clarity
>>
>> Also, introduce some TODOs for further checks which should be performed.
>> These checks are hard to perform at the moment, as there is no easy way to 
>> see
>> which MSR values where given to the guest.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> with one minor adjustment request:
>
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -1770,33 +1770,64 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
>>  
>>  int nvmx_handle_invept(struct cpu_user_regs *regs)
>>  {
>> +    struct vcpu *curr = current;
>> +    struct domain *currd = curr->domain;
>>      struct vmx_inst_decoded decode;
>> -    unsigned long eptp;
>>      int ret;
>>  
>> -    if ( (ret = decode_vmx_inst(regs, &decode, &eptp, 0)) != X86EMUL_OKAY )
>> +    if ( (ret = decode_vmx_inst(regs, &decode, NULL, 0)) != X86EMUL_OKAY )
>>          return ret;
>>  
>> +    /* TODO - reject instruction completely if not configured. */
>> +
>> +    /* Instruction must have a memory operand. */
>> +    if ( decode.type != VMX_INST_MEMREG_TYPE_MEMORY )
>> +    {
>> +        hvm_inject_hw_exception(TRAP_invalid_op, 0);
>> +        return X86EMUL_EXCEPTION;
>> +    }
>> +
>>      switch ( reg_read(regs, decode.reg2) )
>>      {
>>      case INVEPT_SINGLE_CONTEXT:
>>      {
>> -        struct p2m_domain *p2m = p2m_get_nestedp2m(current, eptp);
>> +        struct p2m_domain *p2m;
>> +        pagefault_info_t pfinfo;
>> +        unsigned long eptp;
>> +
>> +        /* TODO - reject SINGLE_CONTEXT if not configured. */
>> +
>> +        ret = hvm_copy_from_guest_linear(&eptp, decode.mem, 8, 0, &pfinfo);
> Please use sizeof(eptp) here.

Ok, but in that case eptp needs to become uint64_t to match the ABI.  In
fact, I probably need to read all 128 bytes and perform the MBZ check on
the 2nd word.

~Andrew

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