Re: [Xen-devel] [PATCH v2 4/4] x86/vvmx: correctly emulate VMREAD

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Sergey Dyasli [mailto:sergey.dyasli@xxxxxxxxxx]
> Sent: Monday, February 06, 2017 10:58 PM
> 
> There is an issue with the original __vmread() in nested vmx mode:
> emulation of a guest's VMREAD with invalid arguments leads to BUG().
> 
> Fix this by using vmread_safe() and reporting any kind of VMfail back
> to the guest.
> 
> A new safe versions of get_vvmcs() macro and related functions are
> introduced because of new function signatures and lots of existing
> users.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>

after reading this patch I realized my earlier ack to 3/4 might not hold,
since now you have mismatched get/set pairs:

get_vvmcs
get_vvmcs_safe
set_vvmcs

suggest to also introduce a set_vvmcs_safe counterpart in 3/4, as
there are still many existing callers on set_vvmcs which don't 
expect error.

> ---
>  xen/arch/x86/hvm/vmx/vmcs.c        |  9 +++++----
>  xen/arch/x86/hvm/vmx/vvmx.c        | 26 +++++++++++++++++++++++---
>  xen/include/asm-x86/hvm/vmx/vmcs.h |  3 ++-
>  xen/include/asm-x86/hvm/vmx/vvmx.h |  7 +++++++
>  4 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 1d83a69..e04b002 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -932,15 +932,16 @@ void virtual_vmcs_exit(const struct vcpu *v)
>          __vmptrld(cur);
>  }
> 
> -u64 virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding)
> +unsigned long virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding,
> +                                  u64 *val)
>  {
> -    u64 res;
> +    unsigned long ret;
> 
>      virtual_vmcs_enter(v);
> -    __vmread(vmcs_encoding, &res);
> +    ret = vmread_safe(vmcs_encoding, val);
>      virtual_vmcs_exit(v);
> 
> -    return res;
> +    return ret;
>  }
> 
>  unsigned long virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding,
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 1a3d1d2..6fda44a 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -261,7 +261,23 @@ u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding)
> 
>  u64 get_vvmcs_real(const struct vcpu *v, u32 encoding)
>  {
> -    return virtual_vmcs_vmread(v, encoding);
> +    u64 val;
> +
> +    virtual_vmcs_vmread(v, encoding, &val);
> +
> +    return val;
> +}
> +
> +unsigned long get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val)
> +{
> +    *val = get_vvmcs_virtual(vvmcs, encoding);
> +
> +    return 0;
> +}
> +
> +unsigned long get_vvmcs_real_safe(const struct vcpu *v, u32 encoding, u64 
> *val)
> +{
> +    return virtual_vmcs_vmread(v, encoding, val);
>  }
> 
>  unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
> @@ -1710,13 +1726,17 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
>      struct vmx_inst_decoded decode;
>      pagefault_info_t pfinfo;
>      u64 value = 0;
> -    int rc;
> +    unsigned long rc;
> 
>      rc = decode_vmx_inst(regs, &decode, NULL, 0);
>      if ( rc != X86EMUL_OKAY )
>          return rc;
> 
> -    value = get_vvmcs(v, reg_read(regs, decode.reg2));
> +    if ( (rc = get_vvmcs_safe(v, reg_read(regs, decode.reg2), &value)) )
> +    {
> +        vmfail(regs, rc);
> +        return X86EMUL_OKAY;
> +    }
> 
>      switch ( decode.type ) {
>      case VMX_INST_MEMREG_TYPE_MEMORY:
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 72dc5fc..4f16250 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -540,7 +540,8 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector);
>  int vmx_check_msr_bitmap(unsigned long *msr_bitmap, u32 msr, int 
> access_type);
>  void virtual_vmcs_enter(const struct vcpu *);
>  void virtual_vmcs_exit(const struct vcpu *);
> -u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
> +unsigned long virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding,
> +                                  u64 *val);
>  unsigned long virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 
> val);
> 
>  static inline int vmx_add_guest_msr(u32 msr)
> diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h
> b/xen/include/asm-x86/hvm/vmx/vvmx.h
> index d60e0bb..28e2503 100644
> --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
> @@ -181,6 +181,8 @@ enum vvmcs_encoding_type {
> 
>  u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
>  u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
> +unsigned long get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val);
> +unsigned long get_vvmcs_real_safe(const struct vcpu *, u32 encoding, u64 
> *val);
>  unsigned long set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
>  unsigned long set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
> 
> @@ -194,6 +196,11 @@ unsigned long set_vvmcs_real(const struct vcpu *, u32 
> encoding,
> u64 val);
>     set_vvmcs_real(vcpu, encoding, val) : \
>     set_vvmcs_virtual(vcpu_nestedhvm(vcpu).nv_vvmcx, encoding, val))
> 
> +#define get_vvmcs_safe(vcpu, encoding, val) \
> +  (cpu_has_vmx_vmcs_shadowing ? \
> +   get_vvmcs_real_safe(vcpu, encoding, val) : \
> +   get_vvmcs_virtual_safe(vcpu_nestedhvm(vcpu).nv_vvmcx, encoding, val))
> +
>  uint64_t get_shadow_eptp(struct vcpu *v);
> 
>  void nvmx_destroy_vmcs(struct vcpu *v);
> --
> 2.9.3


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