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

["Jan Beulich" <JBeulich@suse.com>]

 >>> On 06.02.17 at 17:55, <andrew.cooper3@citrix.com> 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 ...

>  * 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 ...

>  * 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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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.

Jan


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