WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-changelog

[Xen-changelog] [xen-unstable] [HVM][VMX] Clean up and audit VMX uses of

To: xen-changelog@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-changelog] [xen-unstable] [HVM][VMX] Clean up and audit VMX uses of instruction-length
From: Xen patchbot-unstable <patchbot-unstable@xxxxxxxxxxxxxxxxxxx>
Date: Fri, 22 Sep 2006 11:50:18 +0000
Delivery-date: Fri, 22 Sep 2006 04:50:58 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-changelog-request@lists.xensource.com?subject=help>
List-id: BK change log <xen-changelog.lists.xensource.com>
List-post: <mailto:xen-changelog@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=unsubscribe>
Reply-to: xen-devel@xxxxxxxxxxxxxxxxxxx
Sender: xen-changelog-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User kfraser@xxxxxxxxxxxxxxxxxxxxx
# Node ID a753630a6456541bc90c32a17e4b452bcece825d
# Parent  140dff9d90dca30cb8f8e258e8976ce2dafb73e2
[HVM][VMX] Clean up and audit VMX uses of instruction-length
info field. Todo: use by mmio decoder needs to be removed.
Signed-off-by: Keir Fraser <keir@xxxxxxxxxxxxx>
---
 xen/arch/x86/hvm/svm/svm.c            |    4 
 xen/arch/x86/hvm/vmx/io.c             |    8 +
 xen/arch/x86/hvm/vmx/vmx.c            |  138 ++++++++++++++++++++++------------
 xen/include/asm-x86/hvm/svm/emulate.h |   32 +------
 xen/include/asm-x86/hvm/vmx/vmx.h     |   38 ---------
 5 files changed, 109 insertions(+), 111 deletions(-)

diff -r 140dff9d90dc -r a753630a6456 xen/arch/x86/hvm/svm/svm.c
--- a/xen/arch/x86/hvm/svm/svm.c        Fri Sep 22 09:12:00 2006 +0100
+++ b/xen/arch/x86/hvm/svm/svm.c        Fri Sep 22 11:33:03 2006 +0100
@@ -1847,12 +1847,12 @@ static int svm_cr_access(struct vcpu *v,
        where the prefix lives later on */
     index = skip_prefix_bytes(buffer, sizeof(buffer));
     
-    if (type == TYPE_MOV_TO_CR) 
+    if ( type == TYPE_MOV_TO_CR )
     {
         inst_len = __get_instruction_length_from_list(
             vmcb, list_a, ARR_SIZE(list_a), &buffer[index], &match);
     }
-    else
+    else /* type == TYPE_MOV_FROM_CR */
     {
         inst_len = __get_instruction_length_from_list(
             vmcb, list_b, ARR_SIZE(list_b), &buffer[index], &match);
diff -r 140dff9d90dc -r a753630a6456 xen/arch/x86/hvm/vmx/io.c
--- a/xen/arch/x86/hvm/vmx/io.c Fri Sep 22 09:12:00 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/io.c Fri Sep 22 11:33:03 2006 +0100
@@ -108,11 +108,17 @@ asmlinkage void vmx_intr_assist(void)
         return;
     }
 
+    /* This could be moved earlier in the VMX resume sequence. */
     __vmread(IDT_VECTORING_INFO_FIELD, &idtv_info_field);
     if (unlikely(idtv_info_field & INTR_INFO_VALID_MASK)) {
         __vmwrite(VM_ENTRY_INTR_INFO_FIELD, idtv_info_field);
 
-        __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
+        /*
+         * Safe: the length will only be interpreted for software exceptions
+         * and interrupts. If we get here then delivery of some event caused a
+         * fault, and this always results in defined VM_EXIT_INSTRUCTION_LEN.
+         */
+        __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len); /* Safe */
         __vmwrite(VM_ENTRY_INSTRUCTION_LEN, inst_len);
 
         if (unlikely(idtv_info_field & 0x800)) { /* valid error code */
diff -r 140dff9d90dc -r a753630a6456 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c        Fri Sep 22 09:12:00 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c        Fri Sep 22 11:33:03 2006 +0100
@@ -597,7 +597,7 @@ static int vmx_instruction_length(struct
 {
     unsigned long inst_len;
 
-    if (__vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len))
+    if ( __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len) ) /* XXX Unsafe XXX */
         return 0;
     return inst_len;
 }
@@ -836,11 +836,16 @@ int start_vmx(void)
 
 /*
  * Not all cases receive valid value in the VM-exit instruction length field.
+ * Callers must know what they're doing!
  */
-#define __get_instruction_length(len) \
-    __vmread(VM_EXIT_INSTRUCTION_LEN, &(len)); \
-     if ((len) < 1 || (len) > 15) \
-        __hvm_bug(&regs);
+static int __get_instruction_length(void)
+{
+    int len;
+    __vmread(VM_EXIT_INSTRUCTION_LEN, &len); /* Safe: callers audited */
+    if ( (len < 1) || (len > 15) )
+        __hvm_bug(guest_cpu_user_regs());
+    return len;
+}
 
 static void inline __update_guest_eip(unsigned long inst_len)
 {
@@ -1051,15 +1056,18 @@ static int check_for_null_selector(unsig
     int i, inst_len;
     int inst_copy_from_guest(unsigned char *, unsigned long, int);
 
-    __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
+    inst_len = __get_instruction_length(); /* Safe: INS/OUTS */
     memset(inst, 0, MAX_INST_LEN);
-    if (inst_copy_from_guest(inst, eip, inst_len) != inst_len) {
+    if ( inst_copy_from_guest(inst, eip, inst_len) != inst_len )
+    {
         printf("check_for_null_selector: get guest instruction failed\n");
         domain_crash_synchronous();
     }
 
-    for (i = 0; i < inst_len; i++) {
-        switch (inst[i]) {
+    for ( i = 0; i < inst_len; i++ )
+    {
+        switch ( inst[i] )
+        {
         case 0xf3: /* REPZ */
         case 0xf2: /* REPNZ */
         case 0xf0: /* LOCK */
@@ -1184,15 +1192,14 @@ static void vmx_io_instruction(unsigned 
     }
 }
 
-int
-vmx_world_save(struct vcpu *v, struct vmx_assist_context *c)
-{
-    unsigned long inst_len;
+static int vmx_world_save(struct vcpu *v, struct vmx_assist_context *c)
+{
     int error = 0;
 
-    error |= __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
+    /* NB. Skip transition instruction. */
     error |= __vmread(GUEST_RIP, &c->eip);
-    c->eip += inst_len; /* skip transition instruction */
+    c->eip += __get_instruction_length(); /* Safe: MOV Cn, LMSW, CLTS */
+
     error |= __vmread(GUEST_RSP, &c->esp);
     error |= __vmread(GUEST_RFLAGS, &c->eflags);
 
@@ -1249,8 +1256,7 @@ vmx_world_save(struct vcpu *v, struct vm
     return !error;
 }
 
-int
-vmx_world_restore(struct vcpu *v, struct vmx_assist_context *c)
+static int vmx_world_restore(struct vcpu *v, struct vmx_assist_context *c)
 {
     unsigned long mfn, old_cr4, old_base_mfn;
     int error = 0;
@@ -1364,8 +1370,7 @@ vmx_world_restore(struct vcpu *v, struct
 
 enum { VMX_ASSIST_INVOKE = 0, VMX_ASSIST_RESTORE };
 
-int
-vmx_assist(struct vcpu *v, int mode)
+static int vmx_assist(struct vcpu *v, int mode)
 {
     struct vmx_assist_context c;
     u32 magic;
@@ -1408,8 +1413,8 @@ vmx_assist(struct vcpu *v, int mode)
         break;
 
         /*
-         * Restore the VMXASSIST_OLD_CONTEXT that was saved by 
VMX_ASSIST_INVOKE
-         * above.
+         * Restore the VMXASSIST_OLD_CONTEXT that was saved by
+         * VMX_ASSIST_INVOKE above.
          */
     case VMX_ASSIST_RESTORE:
         /* save the old context */
@@ -1552,7 +1557,8 @@ static int vmx_set_cr0(unsigned long val
             }
         }
 
-        if ( vmx_assist(v, VMX_ASSIST_INVOKE) ) {
+        if ( vmx_assist(v, VMX_ASSIST_INVOKE) )
+        {
             set_bit(VMX_CPU_STATE_ASSIST_ENABLED, &v->arch.hvm_vmx.cpu_state);
             __vmread(GUEST_RIP, &eip);
             HVM_DBG_LOG(DBG_LEVEL_1,
@@ -1815,7 +1821,8 @@ static void mov_from_cr(int cr, int gp, 
     HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR%d, value = %lx", cr, value);
 }
 
-static int vmx_cr_access(unsigned long exit_qualification, struct 
cpu_user_regs *regs)
+static int vmx_cr_access(unsigned long exit_qualification,
+                         struct cpu_user_regs *regs)
 {
     unsigned int gp, cr;
     unsigned long value;
@@ -2069,6 +2076,47 @@ void restore_cpu_user_regs(struct cpu_us
 }
 #endif
 
+static void vmx_reflect_exception(struct vcpu *v)
+{
+    int error_code, intr_info, vector;
+
+    __vmread(VM_EXIT_INTR_INFO, &intr_info);
+    vector = intr_info & 0xff;
+    if ( intr_info & INTR_INFO_DELIVER_CODE_MASK )
+        __vmread(VM_EXIT_INTR_ERROR_CODE, &error_code);
+    else
+        error_code = VMX_DELIVER_NO_ERROR_CODE;
+
+#ifndef NDEBUG
+    {
+        unsigned long rip;
+
+        __vmread(GUEST_RIP, &rip);
+        HVM_DBG_LOG(DBG_LEVEL_1, "rip = %lx, error_code = %x",
+                    rip, error_code);
+    }
+#endif /* NDEBUG */
+
+    /*
+     * According to Intel Virtualization Technology Specification for
+     * the IA-32 Intel Architecture (C97063-002 April 2005), section
+     * 2.8.3, SW_EXCEPTION should be used for #BP and #OV, and
+     * HW_EXCEPTION used for everything else.  The main difference
+     * appears to be that for SW_EXCEPTION, the EIP/RIP is incremented
+     * by VM_ENTER_INSTRUCTION_LEN bytes, whereas for HW_EXCEPTION,
+     * it is not.
+     */
+    if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) == INTR_TYPE_SW_EXCEPTION )
+    {
+        int ilen = __get_instruction_length(); /* Safe: software exception */
+        vmx_inject_sw_exception(v, vector, ilen);
+    }
+    else
+    {
+        vmx_inject_hw_exception(v, vector, error_code);
+    }
+}
+
 asmlinkage void vmx_vmexit_handler(struct cpu_user_regs regs)
 {
     unsigned int exit_reason;
@@ -2116,7 +2164,8 @@ asmlinkage void vmx_vmexit_handler(struc
 
     TRACE_VMEXIT(0,exit_reason);
 
-    switch ( exit_reason ) {
+    switch ( exit_reason )
+    {
     case EXIT_REASON_EXCEPTION_NMI:
     {
         /*
@@ -2242,43 +2291,38 @@ asmlinkage void vmx_vmexit_handler(struc
         domain_crash_synchronous();
         break;
     case EXIT_REASON_CPUID:
+        inst_len = __get_instruction_length(); /* Safe: CPUID */
+        __update_guest_eip(inst_len);
         vmx_vmexit_do_cpuid(&regs);
-        __get_instruction_length(inst_len);
-        __update_guest_eip(inst_len);
         break;
     case EXIT_REASON_HLT:
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: HLT */
         __update_guest_eip(inst_len);
         vmx_vmexit_do_hlt();
         break;
     case EXIT_REASON_INVLPG:
     {
-        unsigned long   va;
-
+        unsigned long va;
+        inst_len = __get_instruction_length(); /* Safe: INVLPG */
+        __update_guest_eip(inst_len);
         __vmread(EXIT_QUALIFICATION, &va);
         vmx_vmexit_do_invlpg(va);
-        __get_instruction_length(inst_len);
+        break;
+    }
+    case EXIT_REASON_VMCALL:
+    {
+        inst_len = __get_instruction_length(); /* Safe: VMCALL */
         __update_guest_eip(inst_len);
-        break;
-    }
-    case EXIT_REASON_VMCALL:
-    {
-        __get_instruction_length(inst_len);
         __vmread(GUEST_RIP, &rip);
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-
         hvm_do_hypercall(&regs);
-        __update_guest_eip(inst_len);
         break;
     }
     case EXIT_REASON_CR_ACCESS:
     {
         __vmread(GUEST_RIP, &rip);
-        __get_instruction_length(inst_len);
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-
-        HVM_DBG_LOG(DBG_LEVEL_1, "rip = %lx, inst_len =%lx, exit_qualification 
= %lx",
-                    rip, inst_len, exit_qualification);
+        inst_len = __get_instruction_length(); /* Safe: MOV Cn, LMSW, CLTS */
         if ( vmx_cr_access(exit_qualification, &regs) )
             __update_guest_eip(inst_len);
         TRACE_VMEXIT(3,regs.error_code);
@@ -2291,19 +2335,19 @@ asmlinkage void vmx_vmexit_handler(struc
         break;
     case EXIT_REASON_IO_INSTRUCTION:
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: IN, INS, OUT, OUTS */
         vmx_io_instruction(exit_qualification, inst_len);
         TRACE_VMEXIT(4,exit_qualification);
         break;
     case EXIT_REASON_MSR_READ:
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: RDMSR */
+        __update_guest_eip(inst_len);
         vmx_do_msr_read(&regs);
+        break;
+    case EXIT_REASON_MSR_WRITE:
+        inst_len = __get_instruction_length(); /* Safe: WRMSR */
         __update_guest_eip(inst_len);
-        break;
-    case EXIT_REASON_MSR_WRITE:
         vmx_do_msr_write(&regs);
-        __get_instruction_length(inst_len);
-        __update_guest_eip(inst_len);
         break;
     case EXIT_REASON_MWAIT_INSTRUCTION:
     case EXIT_REASON_MONITOR_INSTRUCTION:
diff -r 140dff9d90dc -r a753630a6456 xen/include/asm-x86/hvm/svm/emulate.h
--- a/xen/include/asm-x86/hvm/svm/emulate.h     Fri Sep 22 09:12:00 2006 +0100
+++ b/xen/include/asm-x86/hvm/svm/emulate.h     Fri Sep 22 11:33:03 2006 +0100
@@ -94,15 +94,14 @@ static inline int __get_instruction_leng
 static inline int __get_instruction_length(struct vmcb_struct *vmcb, 
         enum instruction_index instr, u8 *guest_eip_buf)
 {
-    return __get_instruction_length_from_list(vmcb, &instr, 1, guest_eip_buf, 
-            NULL);
+    return __get_instruction_length_from_list(
+        vmcb, &instr, 1, guest_eip_buf, NULL);
 }
 
 
 static inline unsigned int is_prefix(u8 opc)
 {
-    switch(opc)
-    {
+    switch ( opc ) {
     case 0x66:
     case 0x67:
     case 0x2E:
@@ -115,22 +114,7 @@ static inline unsigned int is_prefix(u8 
     case 0xF3:
     case 0xF2:
 #if __x86_64__
-    case 0x40:
-    case 0x41:
-    case 0x42:
-    case 0x43:
-    case 0x44:
-    case 0x45:
-    case 0x46:
-    case 0x47:
-    case 0x48:
-    case 0x49:
-    case 0x4a:
-    case 0x4b:
-    case 0x4c:
-    case 0x4d:
-    case 0x4e:
-    case 0x4f:
+    case 0x40 ... 0x4f:
 #endif /* __x86_64__ */
         return 1;
     }
@@ -141,15 +125,15 @@ static inline int skip_prefix_bytes(u8 *
 static inline int skip_prefix_bytes(u8 *buf, size_t size)
 {
     int index;
-    for (index = 0; index < size && is_prefix(buf[index]); index ++)  
-        /* do nothing */ ;
+    for ( index = 0; index < size && is_prefix(buf[index]); index++ )
+        continue;
     return index;
 }
 
 
 
-static void inline __update_guest_eip(struct vmcb_struct *vmcb, 
-        int inst_len) 
+static void inline __update_guest_eip(
+    struct vmcb_struct *vmcb, int inst_len) 
 {
     ASSERT(inst_len > 0);
     vmcb->rip += inst_len;
diff -r 140dff9d90dc -r a753630a6456 xen/include/asm-x86/hvm/vmx/vmx.h
--- a/xen/include/asm-x86/hvm/vmx/vmx.h Fri Sep 22 09:12:00 2006 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h Fri Sep 22 11:33:03 2006 +0100
@@ -469,7 +469,7 @@ static inline void __vmx_inject_exceptio
     if ( error_code != VMX_DELIVER_NO_ERROR_CODE ) {
         __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
         intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
-     }
+    }
 
     if ( ilen )
       __vmwrite(VM_ENTRY_INSTRUCTION_LEN, ilen);
@@ -499,40 +499,4 @@ static inline void vmx_inject_extint(str
     __vmwrite(GUEST_INTERRUPTIBILITY_INFO, 0);
 }
 
-static inline void vmx_reflect_exception(struct vcpu *v)
-{
-    int error_code, intr_info, vector;
-
-    __vmread(VM_EXIT_INTR_INFO, &intr_info);
-    vector = intr_info & 0xff;
-    if ( intr_info & INTR_INFO_DELIVER_CODE_MASK )
-        __vmread(VM_EXIT_INTR_ERROR_CODE, &error_code);
-    else
-        error_code = VMX_DELIVER_NO_ERROR_CODE;
-
-#ifndef NDEBUG
-    {
-        unsigned long rip;
-
-        __vmread(GUEST_RIP, &rip);
-        HVM_DBG_LOG(DBG_LEVEL_1, "rip = %lx, error_code = %x",
-                    rip, error_code);
-    }
-#endif /* NDEBUG */
-
-    /* According to Intel Virtualization Technology Specification for
-       the IA-32 Intel Architecture (C97063-002 April 2005), section
-       2.8.3, SW_EXCEPTION should be used for #BP and #OV, and
-       HW_EXCPEPTION used for everything else.  The main difference
-       appears to be that for SW_EXCEPTION, the EIP/RIP is incremented
-       by VM_ENTER_INSTRUCTION_LEN bytes, whereas for HW_EXCEPTION,
-       it is not.  */
-    if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) == INTR_TYPE_SW_EXCEPTION ) {
-        int ilen;
-        __vmread(VM_EXIT_INSTRUCTION_LEN, &ilen);
-        vmx_inject_sw_exception(v, vector, ilen);
-    } else
-        vmx_inject_hw_exception(v, vector, error_code);
-}
-
 #endif /* __ASM_X86_HVM_VMX_VMX_H__ */

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-changelog] [xen-unstable] [HVM][VMX] Clean up and audit VMX uses of instruction-length, Xen patchbot-unstable <=