diff mbox

[v4,3/3] x86/vvmx: correctly emulate VMREAD

Message ID 20170213142110.26467-4-sergey.dyasli@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sergey Dyasli Feb. 13, 2017, 2:21 p.m. UTC
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@citrix.com>
---
v3 --> v4:
* VMX_INSN_SUCCEED is used instead of 0
* added TODO comment into get_vvmcs_virtual_safe() based on Andrew's email

 xen/arch/x86/hvm/vmx/vmcs.c        | 12 ++++++++++++
 xen/arch/x86/hvm/vmx/vvmx.c        | 26 +++++++++++++++++++++++++-
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 ++
 xen/include/asm-x86/hvm/vmx/vvmx.h |  8 ++++++++
 4 files changed, 47 insertions(+), 1 deletion(-)

Comments

Jan Beulich Feb. 13, 2017, 2:53 p.m. UTC | #1
>>> On 13.02.17 at 15:21, <sergey.dyasli@citrix.com> wrote:
> 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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Tian, Kevin Feb. 14, 2017, 2:48 a.m. UTC | #2
> From: Sergey Dyasli [mailto:sergey.dyasli@citrix.com]
> Sent: Monday, February 13, 2017 10:21 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@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index c4ba45e..454d444 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -943,6 +943,18 @@  u64 virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding)
     return res;
 }
 
+enum vmx_insn_errno virtual_vmcs_vmread_safe(const struct vcpu *v,
+                                             u32 vmcs_encoding, u64 *val)
+{
+    enum vmx_insn_errno ret;
+
+    virtual_vmcs_enter(v);
+    ret = vmread_safe(vmcs_encoding, val);
+    virtual_vmcs_exit(v);
+
+    return ret;
+}
+
 void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val)
 {
     virtual_vmcs_enter(v);
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index c5d7262..ec3b946 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -264,6 +264,25 @@  u64 get_vvmcs_real(const struct vcpu *v, u32 encoding)
     return virtual_vmcs_vmread(v, encoding);
 }
 
+enum vmx_insn_errno get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val)
+{
+    *val = get_vvmcs_virtual(vvmcs, encoding);
+
+    /*
+     * TODO: This should not always succeed. Fields and values need to be
+     * audited against the features offered to the guest in the VT-x MSRs.
+     * This should be fixed when the MSR levelling work is started, at which
+     * point there will be a cpuid_policy-like object.
+     */
+    return VMX_INSN_SUCCEED;
+}
+
+enum vmx_insn_errno get_vvmcs_real_safe(const struct vcpu *v, u32 encoding,
+                                        u64 *val)
+{
+    return virtual_vmcs_vmread_safe(v, encoding, val);
+}
+
 void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
 {
     union vmcs_encoding enc;
@@ -1733,7 +1752,12 @@  int nvmx_handle_vmread(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    value = get_vvmcs(v, reg_read(regs, decode.reg2));
+    rc = get_vvmcs_safe(v, reg_read(regs, decode.reg2), &value);
+    if ( rc != VMX_INSN_SUCCEED )
+    {
+        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 1212609..53161be 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -538,6 +538,8 @@  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);
+enum vmx_insn_errno virtual_vmcs_vmread_safe(const struct vcpu *v,
+                                             u32 vmcs_encoding, u64 *val);
 void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
 enum vmx_insn_errno virtual_vmcs_vmwrite_safe(const struct vcpu *v,
                                               u32 vmcs_encoding, u64 val);
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index e49a000..ca2fb25 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -183,6 +183,9 @@  u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
 u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
 void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
 void set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
+enum vmx_insn_errno get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val);
+enum vmx_insn_errno get_vvmcs_real_safe(const struct vcpu *, u32 encoding,
+                                        u64 *val);
 enum vmx_insn_errno set_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 val);
 enum vmx_insn_errno set_vvmcs_real_safe(const struct vcpu *, u32 encoding,
                                         u64 val);
@@ -197,6 +200,11 @@  enum vmx_insn_errno set_vvmcs_real_safe(const struct vcpu *, u32 encoding,
    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))
+
 #define set_vvmcs_safe(vcpu, encoding, val) \
   (cpu_has_vmx_vmcs_shadowing ? \
    set_vvmcs_real_safe(vcpu, encoding, val) : \