[PATCH 0/9] x86/vvmx: Read instruction operands correctly on VM exit

[PATCH 0/9] x86/vvmx: Read instruction operands correctly on VM exit

[Euan Harris]

decode_vmx_inst() does not read instruction operands correctly on VM exit:

 * It incorrectly uses vmx_inst_info's address_size field to calculate
   the sizes of the exit-causing instruction's operands.  The sizes of
   the operands are specified in the SDM and might depend on whether the
   guest is running in 32-bit or 64-bit mode, but they have nothing to do
   with the address_size field.

 * It includes its own segmentation logic, duplicating code elsewhere.
   This segmentation logic is also incorrect and will raise #GP fault
   rather than a #SS fault in response to an invalid memory access
   through the stack segment.
 
Patches 1-6 (up to 'Remove operand decoding from decode_vmx_inst()')
refactor decode_vmx_inst() in preparation for fixing the bugs mentioned
above.  They remove unnecessary code and extract the logic for reading
operands from decode_vmx_inst() into a new operand_read() function.
These patches should not cause any functional changes.

Patch 7 ('Use correct sizes when reading operands') replaces the incorrect
operand size calculations based on address_size with the correct sizes
from the SDM.

Patches 8 and 9 add new hvm_copy_{to,from}_guest_virt() helpers and use
them to read memory operands in place of the incorrect segmentation
logic in decode_vmx_inst().

Euan Harris (9):
  x86/vvmx: Remove enum vmx_regs_enc
  x86/vvmx: Unify operands in struct vmx_inst_decoded
  x86/vvmx: Extract operand reading logic into operand_read()
  x86/vvmx: Remove unnecessary VMX operand reads
  x86/vvmx: Replace direct calls to reg_read() with operand_read()
  x86/vvmx: Remove operand reading from decode_vmx_inst()
  x86/vvmx: Use correct sizes when reading operands
  x86/hvm: Add hvm_copy_{to,from}_guest_virt() helpers
  x86/vvmx: Use hvm_copy_{to,from}_guest_virt() to read operands

 xen/arch/x86/hvm/hvm.c             |  57 ++++++++++
 xen/arch/x86/hvm/vmx/vvmx.c        | 216 +++++++++++++++++++++----------------
 xen/include/asm-x86/hvm/support.h  |  12 +++
 xen/include/asm-x86/hvm/vmx/vvmx.h |  22 ----
 4 files changed, 195 insertions(+), 112 deletions(-)

-- 
2.13.6


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

[PATCH 1/9] x86/vvmx: Remove enum vmx_regs_enc

[Euan Harris]

This is the standard register encoding, is not VVMX-specific and is only
used in a couple of places.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c        |  8 ++++----
 xen/include/asm-x86/hvm/vmx/vvmx.h | 22 ----------------------
 2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index dde02c076b..9d87786894 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -201,10 +201,10 @@ struct vmx_inst_decoded {
             unsigned long mem;
             unsigned int  len;
         };
-        enum vmx_regs_enc reg1;
+        unsigned int reg1;
     };
 
-    enum vmx_regs_enc reg2;
+    unsigned int reg2;
 };
 
 enum vmx_ops_result {
@@ -345,7 +345,7 @@ enum vmx_insn_errno set_vvmcs_real_safe(const struct vcpu *v, u32 encoding,
 }
 
 static unsigned long reg_read(struct cpu_user_regs *regs,
-                              enum vmx_regs_enc index)
+                              unsigned int index)
 {
     unsigned long *pval = decode_register(index, regs, 0);
 
@@ -353,7 +353,7 @@ static unsigned long reg_read(struct cpu_user_regs *regs,
 }
 
 static void reg_write(struct cpu_user_regs *regs,
-                      enum vmx_regs_enc index,
+                      unsigned int index,
                       unsigned long value)
 {
     unsigned long *pval = decode_register(index, regs, 0);
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index 3285b03bbb..9ea35eb795 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -64,28 +64,6 @@ struct nestedvmx {
 /* bit 0-8, and 12 must be 1 */
 #define VMX_ENTRY_CTLS_DEFAULT1		0x11ff
 
-/*
- * Encode of VMX instructions base on Table 24-11 & 24-12 of SDM 3B
- */
-
-enum vmx_regs_enc {
-    VMX_REG_RAX,
-    VMX_REG_RCX,
-    VMX_REG_RDX,
-    VMX_REG_RBX,
-    VMX_REG_RSP,
-    VMX_REG_RBP,
-    VMX_REG_RSI,
-    VMX_REG_RDI,
-    VMX_REG_R8,
-    VMX_REG_R9,
-    VMX_REG_R10,
-    VMX_REG_R11,
-    VMX_REG_R12,
-    VMX_REG_R13,
-    VMX_REG_R14,
-    VMX_REG_R15,
-};
 
 union vmx_inst_info {
     struct {
-- 
2.13.6


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

[PATCH 2/9] x86/vvmx: Unify operands in struct vmx_inst_decoded

[Euan Harris]

This change prepares the way for abstracting the code which reads
memory and register operands into a generic operand_read() function in
a future patch.

Operand 1 may either be a memory location or a register, depending on
the instruction being handled.   Operand 2 may only be a register, but
it is now represented in the same way as operand 1 and so has a type
field.   This will always equal VMX_INST_MEMREG_TYPE_REG.

References to the old structure map to the new one as follows:

  mem  -> op[0].mem
  len  -> op[0].len
  reg1 -> op[0].reg
  reg2 -> op[1].reg

References to type always map to op[0].type in this patch because
previously operand 2 could only be a register and so type was never
checked when accessing it.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 52 ++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9d87786894..20e5e29031 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -195,16 +195,16 @@ bool_t nvmx_ept_enabled(struct vcpu *v)
 struct vmx_inst_decoded {
 #define VMX_INST_MEMREG_TYPE_MEMORY 0
 #define VMX_INST_MEMREG_TYPE_REG    1
-    int type;
-    union {
-        struct {
-            unsigned long mem;
-            unsigned int  len;
+    struct vmx_inst_op {
+        int type;
+        union {
+            struct {
+                unsigned long mem;
+                unsigned int  len;
+            };
+            unsigned int reg_idx;
         };
-        unsigned int reg1;
-    };
-
-    unsigned int reg2;
+    } op[2];
 };
 
 enum vmx_ops_result {
@@ -437,16 +437,16 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     info.word = offset;
 
     if ( info.fields.memreg ) {
-        decode->type = VMX_INST_MEMREG_TYPE_REG;
-        decode->reg1 = info.fields.reg1;
+        decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
+        decode->op[0].reg_idx = info.fields.reg1;
         if ( poperandS != NULL )
-            *poperandS = reg_read(regs, decode->reg1);
+            *poperandS = reg_read(regs, decode->op[0].reg_idx);
     }
     else
     {
         bool mode_64bit = (vmx_guest_x86_mode(v) == 8);
 
-        decode->type = VMX_INST_MEMREG_TYPE_MEMORY;
+        decode->op[0].type = VMX_INST_MEMREG_TYPE_MEMORY;
 
         if ( info.fields.segment > x86_seg_gs )
             goto gp_fault;
@@ -486,11 +486,13 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
             if ( rc != HVMTRANS_okay )
                 return X86EMUL_EXCEPTION;
         }
-        decode->mem = base;
-        decode->len = size;
+
+        decode->op[0].mem = base;
+        decode->op[0].len = size;
     }
 
-    decode->reg2 = info.fields.reg2;
+    decode->op[1].type = VMX_INST_MEMREG_TYPE_REG;
+    decode->op[1].reg_idx = info.fields.reg2;
 
     return X86EMUL_OKAY;
 
@@ -1770,7 +1772,8 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
 
     gpa = nvcpu->nv_vvmcxaddr;
 
-    rc = hvm_copy_to_guest_linear(decode.mem, &gpa, decode.len, 0, &pfinfo);
+    rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
+                                  decode.op[0].len, 0, &pfinfo);
     if ( rc == HVMTRANS_bad_linear_to_gfn )
         hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
     if ( rc != HVMTRANS_okay )
@@ -1850,23 +1853,24 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
-    rc = get_vvmcs_safe(v, reg_read(regs, decode.reg2), &value);
+    rc = get_vvmcs_safe(v, reg_read(regs, decode.op[1].reg_idx), &value);
     if ( rc != VMX_INSN_SUCCEED )
     {
         vmfail(regs, rc);
         return X86EMUL_OKAY;
     }
 
-    switch ( decode.type ) {
+    switch ( decode.op[0].type ) {
     case VMX_INST_MEMREG_TYPE_MEMORY:
-        rc = hvm_copy_to_guest_linear(decode.mem, &value, decode.len, 0, &pfinfo);
+        rc = hvm_copy_to_guest_linear(decode.op[0].mem, &value,
+                                      decode.op[0].len, 0, &pfinfo);
         if ( rc == HVMTRANS_bad_linear_to_gfn )
             hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
         if ( rc != HVMTRANS_okay )
             return X86EMUL_EXCEPTION;
         break;
     case VMX_INST_MEMREG_TYPE_REG:
-        reg_write(regs, decode.reg1, value);
+        reg_write(regs, decode.op[0].reg_idx, value);
         break;
     }
 
@@ -1893,7 +1897,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
-    vmcs_encoding = reg_read(regs, decode.reg2);
+    vmcs_encoding = reg_read(regs, decode.op[1].reg_idx);
     err = set_vvmcs_safe(v, vmcs_encoding, operand);
     if ( err != VMX_INSN_SUCCEED )
     {
@@ -1931,7 +1935,7 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
     if ( (ret = decode_vmx_inst(regs, &decode, &eptp, 0)) != X86EMUL_OKAY )
         return ret;
 
-    switch ( reg_read(regs, decode.reg2) )
+    switch ( reg_read(regs, decode.op[1].reg_idx) )
     {
     case INVEPT_SINGLE_CONTEXT:
     {
@@ -1959,7 +1963,7 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
     if ( (ret = decode_vmx_inst(regs, &decode, &vpid, 0)) != X86EMUL_OKAY )
         return ret;
 
-    switch ( reg_read(regs, decode.reg2) )
+    switch ( reg_read(regs, decode.op[1].reg_idx) )
     {
     /* Just invalidate all tlb entries for all types! */
     case INVVPID_INDIVIDUAL_ADDR:
-- 
2.13.6


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

[PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Euan Harris]

Extract the logic for reading operands from decode_vmx_inst() into
operand_read().   Future patches will replace operand reading logic
in elsewhere with calls to operand_read().

operand_read() must explicitly handle different operand sizes to avoid
corrupting the caller's stack.   This patch should not change the overall
behaviour of the code.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 59 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 20e5e29031..df84592490 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -361,6 +361,40 @@ static void reg_write(struct cpu_user_regs *regs,
     *pval = value;
 }
 
+static int operand_read(void *buf, struct vmx_inst_op *op,
+                        struct cpu_user_regs *regs, unsigned int bytes)
+{
+    if ( op->type == VMX_INST_MEMREG_TYPE_REG )
+    {
+        switch ( bytes )
+        {
+        case 4:
+            *(uint32_t *)buf = reg_read(regs, op->reg_idx);
+
+        case 8:
+            *(uint64_t *)buf = reg_read(regs, op->reg_idx);
+
+        default:
+            ASSERT_UNREACHABLE();
+            return X86EMUL_UNHANDLEABLE;
+        }
+
+        return X86EMUL_OKAY;
+    }
+    else
+    {
+        pagefault_info_t pfinfo;
+        int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, &pfinfo);
+
+        if ( rc == HVMTRANS_bad_linear_to_gfn )
+            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
+        if ( rc != HVMTRANS_okay )
+            return X86EMUL_EXCEPTION;
+
+        return X86EMUL_OKAY;
+    }
+}
+
 static inline u32 __n2_pin_exec_control(struct vcpu *v)
 {
     return get_vvmcs(v, PIN_BASED_VM_EXEC_CONTROL);
@@ -440,7 +474,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
         decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
         decode->op[0].reg_idx = info.fields.reg1;
         if ( poperandS != NULL )
-            *poperandS = reg_read(regs, decode->op[0].reg_idx);
+        {
+            int rc = operand_read(poperandS, &decode->op[0], regs,
+                                  decode->op[0].len);
+            if ( rc != X86EMUL_OKAY )
+                return rc;
+        }
     }
     else
     {
@@ -475,20 +514,16 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
               offset + size - 1 > seg.limit) )
             goto gp_fault;
 
+        decode->op[0].mem = base;
+        decode->op[0].len = size;
+
         if ( poperandS != NULL )
         {
-            pagefault_info_t pfinfo;
-            int rc = hvm_copy_from_guest_linear(poperandS, base, size,
-                                                0, &pfinfo);
-
-            if ( rc == HVMTRANS_bad_linear_to_gfn )
-                hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
-            if ( rc != HVMTRANS_okay )
-                return X86EMUL_EXCEPTION;
+            int rc = operand_read(poperandS, &decode->op[0], regs,
+                                  decode->op[0].len);
+            if ( rc != X86EMUL_OKAY )
+                return rc;
         }
-
-        decode->op[0].mem = base;
-        decode->op[0].len = size;
     }
 
     decode->op[1].type = VMX_INST_MEMREG_TYPE_REG;
-- 
2.13.6


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

[PATCH 4/9] x86/vvmx: Remove unnecessary VMX operand reads

[Euan Harris]

 * vpid is never used in nvmx_handle_invept() so there is no point in
   reading it.

 * vmptrst's operand is the memory address to which to write the VMCS
   pointer.   gpa is the pointer to write.   Reading the address into
   gpa is meaningless.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index df84592490..32c07eca3d 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1801,7 +1801,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
     unsigned long gpa = 0;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
+    rc = decode_vmx_inst(regs, &decode, NULL, 0);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1992,10 +1992,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
 int nvmx_handle_invvpid(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
-    unsigned long vpid;
     int ret;
 
-    if ( (ret = decode_vmx_inst(regs, &decode, &vpid, 0)) != X86EMUL_OKAY )
+    if ( (ret = decode_vmx_inst(regs, &decode, NULL, 0)) != X86EMUL_OKAY )
         return ret;
 
     switch ( reg_read(regs, decode.op[1].reg_idx) )
-- 
2.13.6


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

[PATCH 5/9] x86/vvmx: Replace direct calls to reg_read() with operand_read()

[Euan Harris]

Use operand_read() to read register operands in the following functions:

 * nvmx_handle_vmread()
 * nvmx_handle_vmwrite()
 * nvmx_handle_invept()

Direct reading of memory operands will be replaced in a separate patch.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 32c07eca3d..7cda37b136 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1877,6 +1877,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
     pagefault_info_t pfinfo;
     u64 value = 0;
     int rc;
+    unsigned long vmcs_encoding = 0;
 
     rc = decode_vmx_inst(regs, &decode, NULL, 0);
     if ( rc != X86EMUL_OKAY )
@@ -1888,7 +1889,11 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
-    rc = get_vvmcs_safe(v, reg_read(regs, decode.op[1].reg_idx), &value);
+    rc = operand_read(&vmcs_encoding, &decode.op[1], regs, decode.op[1].len);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    rc = get_vvmcs_safe(v, vmcs_encoding, &value);
     if ( rc != VMX_INSN_SUCCEED )
     {
         vmfail(regs, rc);
@@ -1918,9 +1923,10 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     struct vcpu *v = current;
     struct vmx_inst_decoded decode;
     unsigned long operand; 
-    u64 vmcs_encoding;
+    unsigned long vmcs_encoding = 0;
     bool_t okay = 1;
     enum vmx_insn_errno err;
+    int rc;
 
     if ( decode_vmx_inst(regs, &decode, &operand, 0)
              != X86EMUL_OKAY )
@@ -1932,7 +1938,10 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
-    vmcs_encoding = reg_read(regs, decode.op[1].reg_idx);
+    rc = operand_read(&vmcs_encoding, &decode.op[1], regs, decode.op[1].len);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
     err = set_vvmcs_safe(v, vmcs_encoding, operand);
     if ( err != VMX_INSN_SUCCEED )
     {
@@ -1965,12 +1974,17 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
     unsigned long eptp;
+    unsigned long invept_type = 0;
     int ret;
 
     if ( (ret = decode_vmx_inst(regs, &decode, &eptp, 0)) != X86EMUL_OKAY )
         return ret;
 
-    switch ( reg_read(regs, decode.op[1].reg_idx) )
+    ret = operand_read(&invept_type, &decode.op[1], regs, decode.op[1].len);
+    if ( ret != X86EMUL_OKAY )
+        return ret;
+
+    switch ( invept_type )
     {
     case INVEPT_SINGLE_CONTEXT:
     {
@@ -1992,12 +2006,17 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
 int nvmx_handle_invvpid(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
+    unsigned long invvpid_type = 0;
     int ret;
 
     if ( (ret = decode_vmx_inst(regs, &decode, NULL, 0)) != X86EMUL_OKAY )
         return ret;
 
-    switch ( reg_read(regs, decode.op[1].reg_idx) )
+    ret = operand_read(&invvpid_type, &decode.op[1], regs, decode.op[1].len);
+    if ( ret != X86EMUL_OKAY )
+        return ret;
+
+    switch ( invvpid_type )
     {
     /* Just invalidate all tlb entries for all types! */
     case INVVPID_INDIVIDUAL_ADDR:
-- 
2.13.6


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

[PATCH 6/9] x86/vvmx: Remove operand reading from decode_vmx_inst()

[Euan Harris]

Use operand_read() to read memory operands instead of using the value
read by decode_vmx_inst() in the following functions:

 * nvmx_handle_invept()
 * nvmx_handle_invvpid()
 * nvmx_handle_vmclear()
 * nvmx_handle_vmptrld()
 * nvmx_handle_vmxon()
 * nvmx_handle_vmwrite()

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 57 ++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 7cda37b136..fc2123c7c0 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -456,7 +456,7 @@ gp_fault:
 
 static int decode_vmx_inst(struct cpu_user_regs *regs,
                            struct vmx_inst_decoded *decode,
-                           unsigned long *poperandS, int vmxon_check)
+                           int vmxon_check)
 {
     struct vcpu *v = current;
     union vmx_inst_info info;
@@ -473,13 +473,6 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     if ( info.fields.memreg ) {
         decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
         decode->op[0].reg_idx = info.fields.reg1;
-        if ( poperandS != NULL )
-        {
-            int rc = operand_read(poperandS, &decode->op[0], regs,
-                                  decode->op[0].len);
-            if ( rc != X86EMUL_OKAY )
-                return rc;
-        }
     }
     else
     {
@@ -516,14 +509,6 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
 
         decode->op[0].mem = base;
         decode->op[0].len = size;
-
-        if ( poperandS != NULL )
-        {
-            int rc = operand_read(poperandS, &decode->op[0], regs,
-                                  decode->op[0].len);
-            if ( rc != X86EMUL_OKAY )
-                return rc;
-        }
     }
 
     decode->op[1].type = VMX_INST_MEMREG_TYPE_REG;
@@ -1513,7 +1498,11 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     uint32_t nvmcs_revid;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 1);
+    rc = decode_vmx_inst(regs, &decode, 1);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    rc = operand_read(&gpa, &decode.op[0], regs, decode.op[0].len);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1729,7 +1718,11 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
     unsigned long gpa = 0;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
+    rc = decode_vmx_inst(regs, &decode, 0);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    rc = operand_read(&gpa, &decode.op[0], regs, decode.op[0].len);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1801,7 +1794,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
     unsigned long gpa = 0;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, NULL, 0);
+    rc = decode_vmx_inst(regs, &decode, 0);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1828,7 +1821,11 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
     void *vvmcs;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
+    rc = decode_vmx_inst(regs, &decode, 0);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    rc = operand_read(&gpa, &decode.op[0], regs, decode.op[0].len);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1879,7 +1876,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
     int rc;
     unsigned long vmcs_encoding = 0;
 
-    rc = decode_vmx_inst(regs, &decode, NULL, 0);
+    rc = decode_vmx_inst(regs, &decode, 0);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1928,10 +1925,13 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     enum vmx_insn_errno err;
     int rc;
 
-    if ( decode_vmx_inst(regs, &decode, &operand, 0)
-             != X86EMUL_OKAY )
+    if ( decode_vmx_inst(regs, &decode, 0) != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
+    rc = operand_read(&operand, &decode.op[0], regs, decode.op[0].len);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
     if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
     {
         vmfail_invalid(regs);
@@ -1973,11 +1973,10 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
 int nvmx_handle_invept(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
-    unsigned long eptp;
     unsigned long invept_type = 0;
     int ret;
 
-    if ( (ret = decode_vmx_inst(regs, &decode, &eptp, 0)) != X86EMUL_OKAY )
+    if ( (ret = decode_vmx_inst(regs, &decode, 0)) != X86EMUL_OKAY )
         return ret;
 
     ret = operand_read(&invept_type, &decode.op[1], regs, decode.op[1].len);
@@ -1988,6 +1987,12 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
     {
     case INVEPT_SINGLE_CONTEXT:
     {
+        unsigned long eptp;
+
+        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
+        if ( ret )
+            return ret;
+
         np2m_flush_base(current, eptp);
         break;
     }
@@ -2009,7 +2014,7 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
     unsigned long invvpid_type = 0;
     int ret;
 
-    if ( (ret = decode_vmx_inst(regs, &decode, NULL, 0)) != X86EMUL_OKAY )
+    if ( (ret = decode_vmx_inst(regs, &decode, 0)) != X86EMUL_OKAY )
         return ret;
 
     ret = operand_read(&invvpid_type, &decode.op[1], regs, decode.op[1].len);
-- 
2.13.6


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

[PATCH 7/9] x86/vvmx: Use correct sizes when reading operands

[Euan Harris]

The sizes of VMX operands are defined in the Intel SDM and have nothing
to do with the addr_size field of struct vmx_inst_info:

    invept:   r32/r64, m128
    invvpid:  r32/r64, m128
    vmclear:  m64
    vmptrld:  m64
    vmptrst:  m64
    vmread:   r32/64 or m32/64, r32/64
    vmwrite:  r32/r64, r32/64 or m32/64
    vmon:     m64

* Register operands are 32-bit or 64-bit depending on the guest mode.

* Memory operands are almost always of fixed size, usually 64-bit, but
  for vmread and vmwrite their size depends on the guest mode.

* invept has a 128-bit memory operand but the upper 64 bits are reserved
  and therefore need not be read.

* invvpid has a 128-bit memory operand but we only require the VPID value
  which lies in the lower 64 bits.

When reading variable-size operands, we pass the operand size calculated
by decode_vmx_inst() and stored in strcut vmx_inst_op.   When reading
fixed-size operands, we pass the size of the variable into which the
operand is to be read.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 48 +++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index fc2123c7c0..9a4e6177ad 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -197,11 +197,9 @@ struct vmx_inst_decoded {
 #define VMX_INST_MEMREG_TYPE_REG    1
     struct vmx_inst_op {
         int type;
+        unsigned int bytes;
         union {
-            struct {
-                unsigned long mem;
-                unsigned int  len;
-            };
+            unsigned long mem;
             unsigned int reg_idx;
         };
     } op[2];
@@ -464,6 +462,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     unsigned long base, index, seg_base, disp, offset;
     int scale, size;
 
+    unsigned int bytes = vmx_guest_x86_mode(v);
+
     if ( vmx_inst_check_privilege(regs, vmxon_check) != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
@@ -473,10 +473,11 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     if ( info.fields.memreg ) {
         decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
         decode->op[0].reg_idx = info.fields.reg1;
+        decode->op[0].bytes = bytes;
     }
     else
     {
-        bool mode_64bit = (vmx_guest_x86_mode(v) == 8);
+        bool mode_64bit = (bytes == 8);
 
         decode->op[0].type = VMX_INST_MEMREG_TYPE_MEMORY;
 
@@ -508,11 +509,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
             goto gp_fault;
 
         decode->op[0].mem = base;
-        decode->op[0].len = size;
+        decode->op[0].bytes = bytes;
     }
 
     decode->op[1].type = VMX_INST_MEMREG_TYPE_REG;
     decode->op[1].reg_idx = info.fields.reg2;
+    decode->op[1].bytes = bytes;
 
     return X86EMUL_OKAY;
 
@@ -1494,7 +1496,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     struct vmx_inst_decoded decode;
-    unsigned long gpa = 0;
+    uint64_t gpa;
     uint32_t nvmcs_revid;
     int rc;
 
@@ -1502,7 +1504,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    rc = operand_read(&gpa, &decode.op[0], regs, decode.op[0].len);
+    rc = operand_read(&gpa, &decode.op[0], regs, sizeof(gpa));
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1715,14 +1717,14 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
     struct vcpu *v = current;
     struct vmx_inst_decoded decode;
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
-    unsigned long gpa = 0;
+    uint64_t gpa;
     int rc;
 
     rc = decode_vmx_inst(regs, &decode, 0);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    rc = operand_read(&gpa, &decode.op[0], regs, decode.op[0].len);
+    rc = operand_read(&gpa, &decode.op[0], regs, sizeof(gpa));
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1801,7 +1803,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
     gpa = nvcpu->nv_vvmcxaddr;
 
     rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
-                                  decode.op[0].len, 0, &pfinfo);
+                                  decode.op[0].bytes, 0, &pfinfo);
     if ( rc == HVMTRANS_bad_linear_to_gfn )
         hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
     if ( rc != HVMTRANS_okay )
@@ -1817,7 +1819,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
     struct vmx_inst_decoded decode;
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
-    unsigned long gpa = 0;
+    uint64_t gpa;
     void *vvmcs;
     int rc;
 
@@ -1825,7 +1827,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    rc = operand_read(&gpa, &decode.op[0], regs, decode.op[0].len);
+    rc = operand_read(&gpa, &decode.op[0], regs, sizeof(gpa));
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1886,7 +1888,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
-    rc = operand_read(&vmcs_encoding, &decode.op[1], regs, decode.op[1].len);
+    rc = operand_read(&vmcs_encoding, &decode.op[1], regs, decode.op[1].bytes);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1900,7 +1902,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
     switch ( decode.op[0].type ) {
     case VMX_INST_MEMREG_TYPE_MEMORY:
         rc = hvm_copy_to_guest_linear(decode.op[0].mem, &value,
-                                      decode.op[0].len, 0, &pfinfo);
+                                      decode.op[0].bytes, 0, &pfinfo);
         if ( rc == HVMTRANS_bad_linear_to_gfn )
             hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
         if ( rc != HVMTRANS_okay )
@@ -1928,7 +1930,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     if ( decode_vmx_inst(regs, &decode, 0) != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
-    rc = operand_read(&operand, &decode.op[0], regs, decode.op[0].len);
+    rc = operand_read(&operand, &decode.op[0], regs, decode.op[0].bytes);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1938,7 +1940,7 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
-    rc = operand_read(&vmcs_encoding, &decode.op[1], regs, decode.op[1].len);
+    rc = operand_read(&vmcs_encoding, &decode.op[1], regs, decode.op[1].bytes);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1973,13 +1975,13 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
 int nvmx_handle_invept(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
-    unsigned long invept_type = 0;
+    uint64_t invept_type;
     int ret;
 
     if ( (ret = decode_vmx_inst(regs, &decode, 0)) != X86EMUL_OKAY )
         return ret;
 
-    ret = operand_read(&invept_type, &decode.op[1], regs, decode.op[1].len);
+    ret = operand_read(&invept_type, &decode.op[1], regs, decode.op[1].bytes);
     if ( ret != X86EMUL_OKAY )
         return ret;
 
@@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
     {
     case INVEPT_SINGLE_CONTEXT:
     {
-        unsigned long eptp;
+        uint64_t eptp;
 
-        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
+        ret = operand_read(&eptp, &decode.op[0], regs, sizeof(eptp));
         if ( ret )
             return ret;
 
@@ -2011,13 +2013,13 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
 int nvmx_handle_invvpid(struct cpu_user_regs *regs)
 {
     struct vmx_inst_decoded decode;
-    unsigned long invvpid_type = 0;
+    uint64_t invvpid_type;
     int ret;
 
     if ( (ret = decode_vmx_inst(regs, &decode, 0)) != X86EMUL_OKAY )
         return ret;
 
-    ret = operand_read(&invvpid_type, &decode.op[1], regs, decode.op[1].len);
+    ret = operand_read(&invvpid_type, &decode.op[1], regs, decode.op[1].bytes);
     if ( ret != X86EMUL_OKAY )
         return ret;
 
-- 
2.13.6


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

[PATCH 8/9] x86/hvm: Add hvm_copy_{to, from}_guest_virt() helpers

[Euan Harris]

hvm_copy_{to,from}_guest_virt() copy data to and from a guest, performing
segmentatino and paging checks on the provided seg:offset virtual address.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/hvm.c            | 57 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/support.h | 12 +++++++++
 2 files changed, 69 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 205b4cb685..5d2bdd6b2b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3312,6 +3312,63 @@ unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len)
     return rc ? len : 0; /* fake a copy_from_user() return code */
 }
 
+static int _hvm_copy_guest_virt(
+    enum x86_segment seg, unsigned long offset, void *buf, unsigned int bytes,
+    uint32_t pfec, unsigned int flags)
+{
+    struct vcpu *curr = current;
+    struct segment_register sreg, cs;
+    enum hvm_translation_result res;
+    pagefault_info_t pfinfo;
+    unsigned long linear;
+
+    ASSERT(is_x86_user_segment(seg));
+
+    hvm_get_segment_register(curr, seg, &sreg);
+    hvm_get_segment_register(curr, x86_seg_cs, &cs);
+
+    if ( !hvm_virtual_to_linear_addr(
+             seg, &sreg, offset, bytes,
+             flags & HVMCOPY_to_guest ? hvm_access_write : hvm_access_read,
+             &cs, &linear) )
+    {
+        hvm_inject_hw_exception(
+            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
+        return X86EMUL_EXCEPTION;
+    }
+
+    if ( flags & HVMCOPY_to_guest )
+        res = hvm_copy_to_guest_linear(linear, buf, bytes, pfec, &pfinfo);
+    else
+        res = hvm_copy_from_guest_linear(buf, linear, bytes, pfec, &pfinfo);
+
+    if ( res == HVMTRANS_bad_linear_to_gfn )
+    {
+        hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
+        return X86EMUL_EXCEPTION;
+    }
+    else if ( res )
+        return X86EMUL_RETRY;
+
+    return X86EMUL_OKAY;
+}
+
+int hvm_copy_to_guest_virt(
+    enum x86_segment seg, unsigned long offset, void *buf, unsigned int bytes,
+    uint32_t pfec)
+{
+    return _hvm_copy_guest_virt(seg, offset, buf, bytes, pfec,
+                                HVMCOPY_to_guest);
+}
+
+int hvm_copy_from_guest_virt(
+    void *buf, enum x86_segment seg, unsigned long offset, unsigned int bytes,
+    uint32_t pfec)
+{
+    return _hvm_copy_guest_virt(seg, offset, buf, bytes, pfec,
+                                HVMCOPY_from_guest);
+}
+
 bool hvm_check_cpuid_faulting(struct vcpu *v)
 {
     const struct msr_vcpu_policy *vp = v->arch.msr;
diff --git a/xen/include/asm-x86/hvm/support.h b/xen/include/asm-x86/hvm/support.h
index d784fc1856..9af2ae77b7 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -115,6 +115,18 @@ enum hvm_translation_result hvm_translate_get_page(
     pagefault_info_t *pfinfo, struct page_info **page_p,
     gfn_t *gfn_p, p2m_type_t *p2mt_p);
 
+/*
+ * Copy data to and from a guest, performing segmentation and paging checks
+ * on the provided seg:offset virtual address.
+ * Returns X86EMUL_* and raises exceptions with the current vcpu.
+ */
+int hvm_copy_to_guest_virt(
+    enum x86_segment seg, unsigned long offset, void *buf, unsigned int bytes,
+    uint32_t pfec);
+int hvm_copy_from_guest_virt(
+    void *buf, enum x86_segment seg, unsigned long offset, unsigned int bytes,
+    uint32_t pfec);
+
 #define HVM_HCALL_completed  0 /* hypercall completed - no further action */
 #define HVM_HCALL_preempted  1 /* hypercall preempted - re-execute VMCALL */
 int hvm_hypercall(struct cpu_user_regs *regs);
-- 
2.13.6


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

[PATCH 9/9] x86/vvmx: Use hvm_copy_{to, from}_guest_virt() to read operands

[Euan Harris]

decode_vmx_inst() contains its own segmentation logic.    This
unnecessarily duplicates segmentation code used elsewhere and contains
errors: it raises a #GP fault instead of an #SS fault for an invalid
memory access through the stack segment.

Replace this logic with hvm_copy_{to,from}_guest_virt(), which use
well-tested memory access code used in various other parts of the
hypervisor.

Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 80 +++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 54 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9a4e6177ad..f0a2242711 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -199,7 +199,10 @@ struct vmx_inst_decoded {
         int type;
         unsigned int bytes;
         union {
-            unsigned long mem;
+            struct {
+                enum x86_segment seg;
+                unsigned long offset;
+            };
             unsigned int reg_idx;
         };
     } op[2];
@@ -380,17 +383,7 @@ static int operand_read(void *buf, struct vmx_inst_op *op,
         return X86EMUL_OKAY;
     }
     else
-    {
-        pagefault_info_t pfinfo;
-        int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, &pfinfo);
-
-        if ( rc == HVMTRANS_bad_linear_to_gfn )
-            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
-        if ( rc != HVMTRANS_okay )
-            return X86EMUL_EXCEPTION;
-
-        return X86EMUL_OKAY;
-    }
+        return hvm_copy_from_guest_virt(buf, op->seg, op->offset, bytes, 0);
 }
 
 static inline u32 __n2_pin_exec_control(struct vcpu *v)
@@ -458,9 +451,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
 {
     struct vcpu *v = current;
     union vmx_inst_info info;
-    struct segment_register seg;
-    unsigned long base, index, seg_base, disp, offset;
-    int scale, size;
+    unsigned long base, index, disp, offset;
+    int scale;
 
     unsigned int bytes = vmx_guest_x86_mode(v);
 
@@ -477,14 +469,11 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     }
     else
     {
-        bool mode_64bit = (bytes == 8);
-
-        decode->op[0].type = VMX_INST_MEMREG_TYPE_MEMORY;
-
         if ( info.fields.segment > x86_seg_gs )
-            goto gp_fault;
-        hvm_get_segment_register(v, info.fields.segment, &seg);
-        seg_base = seg.base;
+        {
+            ASSERT_UNREACHABLE();
+            return X86EMUL_UNHANDLEABLE;
+        }
 
         base = info.fields.base_reg_invalid ? 0 :
             reg_read(regs, info.fields.base_reg);
@@ -496,19 +485,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
 
         __vmread(EXIT_QUALIFICATION, &disp);
 
-        size = 1 << (info.fields.addr_size + 1);
-
-        offset = base + index * scale + disp;
-        base = !mode_64bit || info.fields.segment >= x86_seg_fs ?
-               seg_base + offset : offset;
-        if ( offset + size - 1 < offset ||
-             (mode_64bit ?
-              !is_canonical_address((long)base < 0 ? base :
-                                    base + size - 1) :
-              offset + size - 1 > seg.limit) )
-            goto gp_fault;
+        decode->op[0].type = VMX_INST_MEMREG_TYPE_MEMORY;
+        decode->op[0].seg = info.fields.segment;
+        decode->op[0].offset = base + index * scale + disp;
+        if ( info.fields.addr_size < 2 )
+            decode->op[0].offset = (uint32_t)decode->op[0].offset;
 
-        decode->op[0].mem = base;
         decode->op[0].bytes = bytes;
     }
 
@@ -517,10 +499,6 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     decode->op[1].bytes = bytes;
 
     return X86EMUL_OKAY;
-
-gp_fault:
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
-    return X86EMUL_EXCEPTION;
 }
 
 static void vmsucceed(struct cpu_user_regs *regs)
@@ -1792,8 +1770,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
     struct vcpu *v = current;
     struct vmx_inst_decoded decode;
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
-    pagefault_info_t pfinfo;
-    unsigned long gpa = 0;
+    uint64_t gpa;
     int rc;
 
     rc = decode_vmx_inst(regs, &decode, 0);
@@ -1802,12 +1779,10 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
 
     gpa = nvcpu->nv_vvmcxaddr;
 
-    rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
-                                  decode.op[0].bytes, 0, &pfinfo);
-    if ( rc == HVMTRANS_bad_linear_to_gfn )
-        hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
-    if ( rc != HVMTRANS_okay )
-        return X86EMUL_EXCEPTION;
+    rc = hvm_copy_to_guest_virt(decode.op[0].seg, decode.op[0].offset, 
+                                &gpa, sizeof(gpa), 0);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
 
     vmsucceed(regs);
     return X86EMUL_OKAY;
@@ -1873,8 +1848,7 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
     struct vmx_inst_decoded decode;
-    pagefault_info_t pfinfo;
-    u64 value = 0;
+    uint64_t value;
     int rc;
     unsigned long vmcs_encoding = 0;
 
@@ -1901,12 +1875,10 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
 
     switch ( decode.op[0].type ) {
     case VMX_INST_MEMREG_TYPE_MEMORY:
-        rc = hvm_copy_to_guest_linear(decode.op[0].mem, &value,
-                                      decode.op[0].bytes, 0, &pfinfo);
-        if ( rc == HVMTRANS_bad_linear_to_gfn )
-            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
-        if ( rc != HVMTRANS_okay )
-            return X86EMUL_EXCEPTION;
+        rc = hvm_copy_to_guest_virt(decode.op[0].seg, decode.op[0].offset,
+                                    &value, sizeof(value), 0);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
         break;
     case VMX_INST_MEMREG_TYPE_REG:
         reg_write(regs, decode.op[0].reg_idx, value);
-- 
2.13.6


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

Re: [PATCH 0/9] x86/vvmx: Read instruction operands correctly on VM exit

[Andrew Cooper]

On 26/10/17 18:03, Euan Harris wrote:
> decode_vmx_inst() does not read instruction operands correctly on VM exit:
>
>  * It incorrectly uses vmx_inst_info's address_size field to calculate
>    the sizes of the exit-causing instruction's operands.  The sizes of
>    the operands are specified in the SDM and might depend on whether the
>    guest is running in 32-bit or 64-bit mode, but they have nothing to do
>    with the address_size field.
>
>  * It includes its own segmentation logic, duplicating code elsewhere.
>    This segmentation logic is also incorrect and will raise #GP fault
>    rather than a #SS fault in response to an invalid memory access
>    through the stack segment.
>  
> Patches 1-6 (up to 'Remove operand decoding from decode_vmx_inst()')
> refactor decode_vmx_inst() in preparation for fixing the bugs mentioned
> above.  They remove unnecessary code and extract the logic for reading
> operands from decode_vmx_inst() into a new operand_read() function.
> These patches should not cause any functional changes.
>
> Patch 7 ('Use correct sizes when reading operands') replaces the incorrect
> operand size calculations based on address_size with the correct sizes
> from the SDM.
>
> Patches 8 and 9 add new hvm_copy_{to,from}_guest_virt() helpers and use
> them to read memory operands in place of the incorrect segmentation
> logic in decode_vmx_inst().
>
> Euan Harris (9):
>   x86/vvmx: Remove enum vmx_regs_enc
>   x86/vvmx: Unify operands in struct vmx_inst_decoded
>   x86/vvmx: Extract operand reading logic into operand_read()
>   x86/vvmx: Remove unnecessary VMX operand reads
>   x86/vvmx: Replace direct calls to reg_read() with operand_read()
>   x86/vvmx: Remove operand reading from decode_vmx_inst()
>   x86/vvmx: Use correct sizes when reading operands
>   x86/hvm: Add hvm_copy_{to,from}_guest_virt() helpers
>   x86/vvmx: Use hvm_copy_{to,from}_guest_virt() to read operands

All Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>.  I've
noticed a few trivial style issues which can be fixed up on commit if
there are no other issues.

~Andrew

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

Re: [PATCH 4/9] x86/vvmx: Remove unnecessary VMX operand reads

[Tian, Kevin]

> From: Euan Harris [mailto:euan.harris@citrix.com]
> Sent: Friday, October 27, 2017 1:03 AM
> 
>  * vpid is never used in nvmx_handle_invept() so there is no point in
>    reading it.

I think you meant nvmx_handle_invvpid

> 
>  * vmptrst's operand is the memory address to which to write the VMCS
>    pointer.   gpa is the pointer to write.   Reading the address into
>    gpa is meaningless.
> 
> Signed-off-by: Euan Harris <euan.harris@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> index df84592490..32c07eca3d 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1801,7 +1801,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs
> *regs)
>      unsigned long gpa = 0;
>      int rc;
> 
> -    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
> +    rc = decode_vmx_inst(regs, &decode, NULL, 0);
>      if ( rc != X86EMUL_OKAY )
>          return rc;
> 
> @@ -1992,10 +1992,9 @@ int nvmx_handle_invept(struct cpu_user_regs
> *regs)
>  int nvmx_handle_invvpid(struct cpu_user_regs *regs)
>  {
>      struct vmx_inst_decoded decode;
> -    unsigned long vpid;
>      int ret;
> 
> -    if ( (ret = decode_vmx_inst(regs, &decode, &vpid, 0)) != X86EMUL_OKAY )
> +    if ( (ret = decode_vmx_inst(regs, &decode, NULL, 0)) != X86EMUL_OKAY )
>          return ret;
> 
>      switch ( reg_read(regs, decode.op[1].reg_idx) )
> --
> 2.13.6


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

Re: [PATCH 0/9] x86/vvmx: Read instruction operands correctly on VM exit

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, October 27, 2017 1:59 AM
> 
> On 26/10/17 18:03, Euan Harris wrote:
> > decode_vmx_inst() does not read instruction operands correctly on VM
> exit:
> >
> >  * It incorrectly uses vmx_inst_info's address_size field to calculate
> >    the sizes of the exit-causing instruction's operands.  The sizes of
> >    the operands are specified in the SDM and might depend on whether
> the
> >    guest is running in 32-bit or 64-bit mode, but they have nothing to do
> >    with the address_size field.
> >
> >  * It includes its own segmentation logic, duplicating code elsewhere.
> >    This segmentation logic is also incorrect and will raise #GP fault
> >    rather than a #SS fault in response to an invalid memory access
> >    through the stack segment.
> >
> > Patches 1-6 (up to 'Remove operand decoding from decode_vmx_inst()')
> > refactor decode_vmx_inst() in preparation for fixing the bugs mentioned
> > above.  They remove unnecessary code and extract the logic for reading
> > operands from decode_vmx_inst() into a new operand_read() function.
> > These patches should not cause any functional changes.
> >
> > Patch 7 ('Use correct sizes when reading operands') replaces the incorrect
> > operand size calculations based on address_size with the correct sizes
> > from the SDM.
> >
> > Patches 8 and 9 add new hvm_copy_{to,from}_guest_virt() helpers and
> use
> > them to read memory operands in place of the incorrect segmentation
> > logic in decode_vmx_inst().
> >
> > Euan Harris (9):
> >   x86/vvmx: Remove enum vmx_regs_enc
> >   x86/vvmx: Unify operands in struct vmx_inst_decoded
> >   x86/vvmx: Extract operand reading logic into operand_read()
> >   x86/vvmx: Remove unnecessary VMX operand reads
> >   x86/vvmx: Replace direct calls to reg_read() with operand_read()
> >   x86/vvmx: Remove operand reading from decode_vmx_inst()
> >   x86/vvmx: Use correct sizes when reading operands
> >   x86/hvm: Add hvm_copy_{to,from}_guest_virt() helpers
> >   x86/vvmx: Use hvm_copy_{to,from}_guest_virt() to read operands
> 
> All Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>.  I've
> noticed a few trivial style issues which can be fixed up on commit if
> there are no other issues.
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 0/9] x86/vvmx: Read instruction operands correctly on VM exit

[Andrew Cooper]

On 02/11/17 07:23, Tian, Kevin wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Friday, October 27, 2017 1:59 AM
>>
>> On 26/10/17 18:03, Euan Harris wrote:
>>> decode_vmx_inst() does not read instruction operands correctly on VM
>> exit:
>>>  * It incorrectly uses vmx_inst_info's address_size field to calculate
>>>    the sizes of the exit-causing instruction's operands.  The sizes of
>>>    the operands are specified in the SDM and might depend on whether
>> the
>>>    guest is running in 32-bit or 64-bit mode, but they have nothing to do
>>>    with the address_size field.
>>>
>>>  * It includes its own segmentation logic, duplicating code elsewhere.
>>>    This segmentation logic is also incorrect and will raise #GP fault
>>>    rather than a #SS fault in response to an invalid memory access
>>>    through the stack segment.
>>>
>>> Patches 1-6 (up to 'Remove operand decoding from decode_vmx_inst()')
>>> refactor decode_vmx_inst() in preparation for fixing the bugs mentioned
>>> above.  They remove unnecessary code and extract the logic for reading
>>> operands from decode_vmx_inst() into a new operand_read() function.
>>> These patches should not cause any functional changes.
>>>
>>> Patch 7 ('Use correct sizes when reading operands') replaces the incorrect
>>> operand size calculations based on address_size with the correct sizes
>>> from the SDM.
>>>
>>> Patches 8 and 9 add new hvm_copy_{to,from}_guest_virt() helpers and
>> use
>>> them to read memory operands in place of the incorrect segmentation
>>> logic in decode_vmx_inst().
>>>
>>> Euan Harris (9):
>>>   x86/vvmx: Remove enum vmx_regs_enc
>>>   x86/vvmx: Unify operands in struct vmx_inst_decoded
>>>   x86/vvmx: Extract operand reading logic into operand_read()
>>>   x86/vvmx: Remove unnecessary VMX operand reads
>>>   x86/vvmx: Replace direct calls to reg_read() with operand_read()
>>>   x86/vvmx: Remove operand reading from decode_vmx_inst()
>>>   x86/vvmx: Use correct sizes when reading operands
>>>   x86/hvm: Add hvm_copy_{to,from}_guest_virt() helpers
>>>   x86/vvmx: Use hvm_copy_{to,from}_guest_virt() to read operands
>> All Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>.  I've
>> noticed a few trivial style issues which can be fixed up on commit if
>> there are no other issues.
>>
> Acked-by: Kevin Tian <kevin.tian@intel.com>

Pulled into x86-next, with the comments addressed.

~Andrew

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

Re: [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Jan Beulich]

>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -361,6 +361,40 @@ static void reg_write(struct cpu_user_regs *regs,
>      *pval = value;
>  }
>  
> +static int operand_read(void *buf, struct vmx_inst_op *op,
> +                        struct cpu_user_regs *regs, unsigned int bytes)
> +{
> +    if ( op->type == VMX_INST_MEMREG_TYPE_REG )
> +    {
> +        switch ( bytes )
> +        {
> +        case 4:
> +            *(uint32_t *)buf = reg_read(regs, op->reg_idx);
> +
> +        case 8:
> +            *(uint64_t *)buf = reg_read(regs, op->reg_idx);
> +
> +        default:
> +            ASSERT_UNREACHABLE();
> +            return X86EMUL_UNHANDLEABLE;
> +        }

Looks like there are two missing break statements up there.

> +        return X86EMUL_OKAY;

This and ...

> +    }
> +    else
> +    {
> +        pagefault_info_t pfinfo;
> +        int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, &pfinfo);
> +
> +        if ( rc == HVMTRANS_bad_linear_to_gfn )
> +            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
> +        if ( rc != HVMTRANS_okay )
> +            return X86EMUL_EXCEPTION;
> +
> +        return X86EMUL_OKAY;

... this should become a uniform ...

> +    }

... return here.

> @@ -440,7 +474,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>          decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
>          decode->op[0].reg_idx = info.fields.reg1;
>          if ( poperandS != NULL )
> -            *poperandS = reg_read(regs, decode->op[0].reg_idx);
> +        {
> +            int rc = operand_read(poperandS, &decode->op[0], regs,
> +                                  decode->op[0].len);
> +            if ( rc != X86EMUL_OKAY )

Blank line between declaration(s) and statement(s) please.

Jan


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

Re: [PATCH 4/9] x86/vvmx: Remove unnecessary VMX operand reads

[Jan Beulich]

 >>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1801,7 +1801,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
>      unsigned long gpa = 0;
>      int rc;
>  
> -    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
> +    rc = decode_vmx_inst(regs, &decode, NULL, 0);

You should then also drop gpa's initializer.

Jan


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

Re: [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Andrew Cooper]

On 27/11/17 17:01, Jan Beulich wrote:
>>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -361,6 +361,40 @@ static void reg_write(struct cpu_user_regs *regs,
>>      *pval = value;
>>  }
>>  
>> +static int operand_read(void *buf, struct vmx_inst_op *op,
>> +                        struct cpu_user_regs *regs, unsigned int bytes)
>> +{
>> +    if ( op->type == VMX_INST_MEMREG_TYPE_REG )
>> +    {
>> +        switch ( bytes )
>> +        {
>> +        case 4:
>> +            *(uint32_t *)buf = reg_read(regs, op->reg_idx);
>> +
>> +        case 8:
>> +            *(uint64_t *)buf = reg_read(regs, op->reg_idx);
>> +
>> +        default:
>> +            ASSERT_UNREACHABLE();
>> +            return X86EMUL_UNHANDLEABLE;
>> +        }
> Looks like there are two missing break statements up there.

I fixed these up x86-next.  (Although it was only because Coverity
noticed and complained at me...)

>
>> +        return X86EMUL_OKAY;
> This and ...
>
>> +    }
>> +    else
>> +    {
>> +        pagefault_info_t pfinfo;
>> +        int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, &pfinfo);
>> +
>> +        if ( rc == HVMTRANS_bad_linear_to_gfn )
>> +            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
>> +        if ( rc != HVMTRANS_okay )
>> +            return X86EMUL_EXCEPTION;
>> +
>> +        return X86EMUL_OKAY;
> ... this should become a uniform ...
>
>> +    }
> ... return here.

I tried this, but later patches in the series need them to move back. 
Overall, this layout reduces the code churn across the series.

>
>> @@ -440,7 +474,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>>          decode->op[0].type = VMX_INST_MEMREG_TYPE_REG;
>>          decode->op[0].reg_idx = info.fields.reg1;
>>          if ( poperandS != NULL )
>> -            *poperandS = reg_read(regs, decode->op[0].reg_idx);
>> +        {
>> +            int rc = operand_read(poperandS, &decode->op[0], regs,
>> +                                  decode->op[0].len);
>> +            if ( rc != X86EMUL_OKAY )
> Blank line between declaration(s) and statement(s) please.

I can fix this.

~Andrew

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

Re: [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Jan Beulich]

>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
> +static int operand_read(void *buf, struct vmx_inst_op *op,
> +                        struct cpu_user_regs *regs, unsigned int bytes)

const (twice)

> +{
> +    if ( op->type == VMX_INST_MEMREG_TYPE_REG )
> +    {
> +        switch ( bytes )
> +        {
> +        case 4:
> +            *(uint32_t *)buf = reg_read(regs, op->reg_idx);

Looking at patch 7, you leave the upper half of 64-bit variables
uninitialized here as well as in the memory case further down
when passing in a smaller value for "bytes". A decent static
analyzer should flag this, and I think things also wouldn't work
right in a few cases.

Jan


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

Re: [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands

[Jan Beulich]

>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
> * invvpid has a 128-bit memory operand but we only require the VPID value
>   which lies in the lower 64 bits.

The memory operand (wrongly) isn't being read at all - I don't
understand the above bullet point for that reason.

> @@ -464,6 +462,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>      unsigned long base, index, seg_base, disp, offset;
>      int scale, size;
>  
> +    unsigned int bytes = vmx_guest_x86_mode(v);
> +

Except in extraordinary circumstances please don't add blank lines in
the middle of declarations.

Also - don't you get 2 here for 16-bit protected mode, which you'd
need to convert to 4?

> @@ -1801,7 +1803,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
>      gpa = nvcpu->nv_vvmcxaddr;
>  
>      rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
> -                                  decode.op[0].len, 0, &pfinfo);
> +                                  decode.op[0].bytes, 0, &pfinfo);

Just like for vmptrld the operand size is uniformly 64 bits here.

> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
>      {
>      case INVEPT_SINGLE_CONTEXT:
>      {
> -        unsigned long eptp;
> +        uint64_t eptp;
>  
> -        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
> +        ret = operand_read(&eptp, &decode.op[0], regs, sizeof(eptp));

I think you should read the full 128 bits for correct faulting behavior
(also for invvpid). I also can't derive from the SDM that this reading
won't occur for the INVEPT_ALL_CONTEXT case. Sadly the SDM
doesn't say whether the reserved upper half is enforced to be zero
(which we would then need to emulate), or whether it is being
ignored. For invvpid however it does state that bits 16:63 are being
checked.

Jan


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

Re: [PATCH 8/9] x86/hvm: Add hvm_copy_{to, from}_guest_virt() helpers

[Jan Beulich]

>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
> hvm_copy_{to,from}_guest_virt() copy data to and from a guest, performing
> segmentatino and paging checks on the provided seg:offset virtual address.

I'm not sure "paging" is worthwhile to mention here, as that's not
done by the new functions, but by the existing one acting on linear
addresses.

> +static int _hvm_copy_guest_virt(
> +    enum x86_segment seg, unsigned long offset, void *buf, unsigned int bytes,
> +    uint32_t pfec, unsigned int flags)
> +{
> +    struct vcpu *curr = current;
> +    struct segment_register sreg, cs;
> +    enum hvm_translation_result res;
> +    pagefault_info_t pfinfo;
> +    unsigned long linear;
> +
> +    ASSERT(is_x86_user_segment(seg));
> +
> +    hvm_get_segment_register(curr, seg, &sreg);
> +    hvm_get_segment_register(curr, x86_seg_cs, &cs);
> +
> +    if ( !hvm_virtual_to_linear_addr(
> +             seg, &sreg, offset, bytes,
> +             flags & HVMCOPY_to_guest ? hvm_access_write : hvm_access_read,
> +             &cs, &linear) )
> +    {
> +        hvm_inject_hw_exception(
> +            (seg == x86_seg_ss) ? TRAP_stack_error : TRAP_gp_fault, 0);
> +        return X86EMUL_EXCEPTION;
> +    }
> +
> +    if ( flags & HVMCOPY_to_guest )
> +        res = hvm_copy_to_guest_linear(linear, buf, bytes, pfec, &pfinfo);
> +    else
> +        res = hvm_copy_from_guest_linear(buf, linear, bytes, pfec, &pfinfo);
> +
> +    if ( res == HVMTRANS_bad_linear_to_gfn )
> +    {
> +        hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
> +        return X86EMUL_EXCEPTION;
> +    }
> +    else if ( res )
> +        return X86EMUL_RETRY;
> +
> +    return X86EMUL_OKAY;

Please either use switch() here, or at least omit the pointless "else".

> +int hvm_copy_to_guest_virt(
> +    enum x86_segment seg, unsigned long offset, void *buf, unsigned int bytes,
> +    uint32_t pfec)
> +{
> +    return _hvm_copy_guest_virt(seg, offset, buf, bytes, pfec,
> +                                HVMCOPY_to_guest);
> +}
> +
> +int hvm_copy_from_guest_virt(
> +    void *buf, enum x86_segment seg, unsigned long offset, unsigned int bytes,
> +    uint32_t pfec)
> +{
> +    return _hvm_copy_guest_virt(seg, offset, buf, bytes, pfec,
> +                                HVMCOPY_from_guest);
> +}

Is there any reason the parameter order is different between the
two wrappers and the actual worker? That'll cause unnecessary
register shuffling, when really the wrappers could just be a load
of a constant into a register plus a branch.

Jan


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

Re: [PATCH 3/9] x86/vvmx: Extract operand reading logic into operand_read()

[Jan Beulich]

>>> On 27.11.17 at 19:08, <andrew.cooper3@citrix.com> wrote:
> On 27/11/17 17:01, Jan Beulich wrote:
>>>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
>>> +        return X86EMUL_OKAY;
>> This and ...
>>
>>> +    }
>>> +    else
>>> +    {
>>> +        pagefault_info_t pfinfo;
>>> +        int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, &pfinfo);
>>> +
>>> +        if ( rc == HVMTRANS_bad_linear_to_gfn )
>>> +            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
>>> +        if ( rc != HVMTRANS_okay )
>>> +            return X86EMUL_EXCEPTION;
>>> +
>>> +        return X86EMUL_OKAY;
>> ... this should become a uniform ...
>>
>>> +    }
>> ... return here.
> 
> I tried this, but later patches in the series need them to move back. 
> Overall, this layout reduces the code churn across the series.

Ah, I see.

Jan


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

Re: [PATCH 9/9] x86/vvmx: Use hvm_copy_{to, from}_guest_virt() to read operands

[Jan Beulich]

>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:

In the title please use "read/write" or "access".

> @@ -380,17 +383,7 @@ static int operand_read(void *buf, struct vmx_inst_op *op,
>          return X86EMUL_OKAY;
>      }
>      else
> -    {
> -        pagefault_info_t pfinfo;
> -        int rc = hvm_copy_from_guest_linear(buf, op->mem, bytes, 0, &pfinfo);
> -
> -        if ( rc == HVMTRANS_bad_linear_to_gfn )
> -            hvm_inject_page_fault(pfinfo.ec, pfinfo.linear);
> -        if ( rc != HVMTRANS_okay )
> -            return X86EMUL_EXCEPTION;
> -
> -        return X86EMUL_OKAY;
> -    }
> +        return hvm_copy_from_guest_virt(buf, op->seg, op->offset, bytes, 0);
>  }

Please also drop the now pointless "else".

> @@ -458,9 +451,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>  {
>      struct vcpu *v = current;
>      union vmx_inst_info info;
> -    struct segment_register seg;
> -    unsigned long base, index, seg_base, disp, offset;
> -    int scale, size;
> +    unsigned long base, index, disp, offset;
> +    int scale;

unsigned int please, if you touch it anyway.

> @@ -496,19 +485,12 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>  
>          __vmread(EXIT_QUALIFICATION, &disp);
>  
> -        size = 1 << (info.fields.addr_size + 1);
> -
> -        offset = base + index * scale + disp;
> -        base = !mode_64bit || info.fields.segment >= x86_seg_fs ?
> -               seg_base + offset : offset;
> -        if ( offset + size - 1 < offset ||
> -             (mode_64bit ?
> -              !is_canonical_address((long)base < 0 ? base :
> -                                    base + size - 1) :
> -              offset + size - 1 > seg.limit) )
> -            goto gp_fault;
> +        decode->op[0].type = VMX_INST_MEMREG_TYPE_MEMORY;
> +        decode->op[0].seg = info.fields.segment;
> +        decode->op[0].offset = base + index * scale + disp;
> +        if ( info.fields.addr_size < 2 )
> +            decode->op[0].offset = (uint32_t)decode->op[0].offset;

For 16-bit addressing you need to truncate to 16 bits.

Jan


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

Re: [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands

[Andrew Cooper]

On 28/11/17 08:32, Jan Beulich wrote:
>>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
>> * invvpid has a 128-bit memory operand but we only require the VPID value
>>   which lies in the lower 64 bits.
> The memory operand (wrongly) isn't being read at all - I don't
> understand the above bullet point for that reason.
>
>> @@ -464,6 +462,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>>      unsigned long base, index, seg_base, disp, offset;
>>      int scale, size;
>>  
>> +    unsigned int bytes = vmx_guest_x86_mode(v);
>> +
> Except in extraordinary circumstances please don't add blank lines in
> the middle of declarations.
>
> Also - don't you get 2 here for 16-bit protected mode, which you'd
> need to convert to 4?

We can never reach this point from a 16 bit segment.  All VMX
instructions #UD if executed outside of a 64bit (in long mode) or 32bit
(outside of long mode) segment.

>
>> @@ -1801,7 +1803,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
>>      gpa = nvcpu->nv_vvmcxaddr;
>>  
>>      rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
>> -                                  decode.op[0].len, 0, &pfinfo);
>> +                                  decode.op[0].bytes, 0, &pfinfo);
> Just like for vmptrld the operand size is uniformly 64 bits here.
>
>> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
>>      {
>>      case INVEPT_SINGLE_CONTEXT:
>>      {
>> -        unsigned long eptp;
>> +        uint64_t eptp;
>>  
>> -        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
>> +        ret = operand_read(&eptp, &decode.op[0], regs, sizeof(eptp));
> I think you should read the full 128 bits for correct faulting behavior
> (also for invvpid). I also can't derive from the SDM that this reading
> won't occur for the INVEPT_ALL_CONTEXT case. Sadly the SDM
> doesn't say whether the reserved upper half is enforced to be zero
> (which we would then need to emulate), or whether it is being
> ignored. For invvpid however it does state that bits 16:63 are being
> checked.

Observations on real hardware to the contrary.  Each of the generations
I've tried never read the operand, or the part of the operand that they
don't need.

It makes sense from a performance point of view.  No point having
microcode make unnecessary memory accesses.

~Andrew

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

Re: [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands

[Jan Beulich]

>>> On 01.12.17 at 19:45, <andrew.cooper3@citrix.com> wrote:
> On 28/11/17 08:32, Jan Beulich wrote:
>>>>> On 26.10.17 at 19:03, <euan.harris@citrix.com> wrote:
>>> * invvpid has a 128-bit memory operand but we only require the VPID value
>>>   which lies in the lower 64 bits.
>> The memory operand (wrongly) isn't being read at all - I don't
>> understand the above bullet point for that reason.
>>
>>> @@ -464,6 +462,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>>>      unsigned long base, index, seg_base, disp, offset;
>>>      int scale, size;
>>>  
>>> +    unsigned int bytes = vmx_guest_x86_mode(v);
>>> +
>> Except in extraordinary circumstances please don't add blank lines in
>> the middle of declarations.
>>
>> Also - don't you get 2 here for 16-bit protected mode, which you'd
>> need to convert to 4?
> 
> We can never reach this point from a 16 bit segment.  All VMX
> instructions #UD if executed outside of a 64bit (in long mode) or 32bit
> (outside of long mode) segment.

That's certainly not what the insn pages say: The common conditional
used is

IF (not in VMX operation) or (CR0.PE = 0) or (RFLAGS.VM = 1) or (IA32_EFER.LMA = 1 and CS.L = 0)
THEN #UD;

which excludes real, VM86, and compat modes, but not 16-bit
protected mode. Of course there's the option of hardware
behaving like you say and the manual just being wrong.

>>> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
>>>      {
>>>      case INVEPT_SINGLE_CONTEXT:
>>>      {
>>> -        unsigned long eptp;
>>> +        uint64_t eptp;
>>>  
>>> -        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
>>> +        ret = operand_read(&eptp, &decode.op[0], regs, sizeof(eptp));
>> I think you should read the full 128 bits for correct faulting behavior
>> (also for invvpid). I also can't derive from the SDM that this reading
>> won't occur for the INVEPT_ALL_CONTEXT case. Sadly the SDM
>> doesn't say whether the reserved upper half is enforced to be zero
>> (which we would then need to emulate), or whether it is being
>> ignored. For invvpid however it does state that bits 16:63 are being
>> checked.
> 
> Observations on real hardware to the contrary.  Each of the generations
> I've tried never read the operand, or the part of the operand that they
> don't need.

Correct faulting behavior implies more than whether the actual read
occurs: A quick test confirms that the ept_sync_all() used during
VMX enabling ends in #GP when the memory address of INVEPT is
being changed to a non-canonical one, and in #PF if changed to a
suitable canonical one (on a Westmere system).

Jan


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