[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH] x86: fix debug register handling



This is intended to fix a number of problems:
- loading of DR7 in generic __context_switch() allowed HVM guests to
  set breakpoints in ways that would crash the hypervisor
- loading of DRs only dependent upon DR7 (for HVM guests even just
  DR7.Ln/Gn) being non-zero leaked information (the other DRs) and
  potentially corrupted state (if other DRs were set prior to a
  domain being preempted and any intermediately scheduled vCPU had a
  non-zero DR7)
- {svm,vmx}_save_dr() did not save anything due to a broken inline asm
  constraint

As a side effect, it also does away with the proliferation of loaddebug()/
savedebug() macros in various source files.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx>

Index: 2007-10-10/xen/arch/x86/acpi/suspend.c
===================================================================
--- 2007-10-10.orig/xen/arch/x86/acpi/suspend.c 2007-10-29 08:59:07.000000000 
+0100
+++ 2007-10-10/xen/arch/x86/acpi/suspend.c      2007-10-26 16:05:20.000000000 
+0200
@@ -29,9 +29,6 @@ void save_rest_processor_state(void)
 #endif
 }
 
-#define loaddebug(_v,_reg) \
-    __asm__ __volatile__ ("mov %0,%%db" #_reg : : "r" ((_v)->debugreg[_reg]))
-
 void restore_rest_processor_state(void)
 {
     int cpu = smp_processor_id();
@@ -54,16 +51,25 @@ void restore_rest_processor_state(void)
 #endif
 
     /* Maybe load the debug registers. */
-    if ( !is_idle_vcpu(v) && unlikely(v->arch.guest_context.debugreg[7]) )
+    if ( !is_idle_vcpu(v) )
     {
-        loaddebug(&v->arch.guest_context, 0);
-        loaddebug(&v->arch.guest_context, 1);
-        loaddebug(&v->arch.guest_context, 2);
-        loaddebug(&v->arch.guest_context, 3);
+        if ( unlikely(v->arch.guest_context.debugreg[0]) )
+            loaddebug(v, 0);
+        if ( unlikely(v->arch.guest_context.debugreg[1]) )
+            loaddebug(v, 1);
+        if ( unlikely(v->arch.guest_context.debugreg[2]) )
+            loaddebug(v, 2);
+        if ( unlikely(v->arch.guest_context.debugreg[3]) )
+            loaddebug(v, 3);
         /* no 4 and 5 */
-        loaddebug(&v->arch.guest_context, 6);
-        loaddebug(&v->arch.guest_context, 7);
+        if ( unlikely(v->arch.guest_context.debugreg[6]) )
+            loaddebug(v, 6);
+        if ( unlikely(v->arch.guest_context.debugreg[7]) )
+            loaddebug(v, 7);
     }
+    else
+        memset(v->arch.guest_context.debugreg, 0,
+               sizeof(v->arch.guest_context.debugreg));
 
     /* Reload FPU state on next FPU use. */
     stts();
Index: 2007-10-10/xen/arch/x86/domain.c
===================================================================
--- 2007-10-10.orig/xen/arch/x86/domain.c       2007-10-29 08:59:07.000000000 
+0100
+++ 2007-10-10/xen/arch/x86/domain.c    2007-10-26 16:47:52.000000000 +0200
@@ -1194,10 +1194,16 @@ static void paravirt_ctxt_switch_to(stru
 {
     set_int80_direct_trap(v);
     switch_kernel_stack(v);
-}
 
-#define loaddebug(_v,_reg) \
-    asm volatile ( "mov %0,%%db" #_reg : : "r" ((_v)->debugreg[_reg]) )
+    /* Maybe switch the debug registers. */
+    cond_loaddebug(v, 0);
+    cond_loaddebug(v, 1);
+    cond_loaddebug(v, 2);
+    cond_loaddebug(v, 3);
+    /* no 4 and 5 */
+    cond_loaddebug(v, 6);
+    cond_loaddebug(v, 7);
+}
 
 static void __context_switch(void)
 {
@@ -1223,18 +1229,6 @@ static void __context_switch(void)
         memcpy(stack_regs,
                &n->arch.guest_context.user_regs,
                CTXT_SWITCH_STACK_BYTES);
-
-        /* Maybe switch the debug registers. */
-        if ( unlikely(n->arch.guest_context.debugreg[7]) )
-        {
-            loaddebug(&n->arch.guest_context, 0);
-            loaddebug(&n->arch.guest_context, 1);
-            loaddebug(&n->arch.guest_context, 2);
-            loaddebug(&n->arch.guest_context, 3);
-            /* no 4 and 5 */
-            loaddebug(&n->arch.guest_context, 6);
-            loaddebug(&n->arch.guest_context, 7);
-        }
         n->arch.ctxt_switch_to(n);
     }
 
Index: 2007-10-10/xen/arch/x86/hvm/svm/svm.c
===================================================================
--- 2007-10-10.orig/xen/arch/x86/hvm/svm/svm.c  2007-10-29 08:59:07.000000000 
+0100
+++ 2007-10-10/xen/arch/x86/hvm/svm/svm.c       2007-10-29 09:05:02.000000000 
+0100
@@ -138,38 +138,53 @@ static enum handler_return long_mode_do_
 }
 
 
-#define loaddebug(_v,_reg) \
-    asm volatile ("mov %0,%%db" #_reg : : "r" ((_v)->debugreg[_reg]))
-#define savedebug(_v,_reg) \
-    asm volatile ("mov %%db" #_reg ",%0" : : "r" ((_v)->debugreg[_reg]))
-
 static void svm_save_dr(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 
+    v->arch.guest_context.debugreg[6] = vmcb->dr6;
+
     if ( !v->arch.hvm_vcpu.flag_dr_dirty )
         return;
 
     /* Clear the DR dirty flag and re-enable intercepts for DR accesses. */
     v->arch.hvm_vcpu.flag_dr_dirty = 0;
-    v->arch.hvm_svm.vmcb->dr_intercepts = DR_INTERCEPT_ALL_WRITES;
+    v->arch.hvm_svm.vmcb->dr_intercepts = DR_INTERCEPTS;
 
-    savedebug(&v->arch.guest_context, 0);
-    savedebug(&v->arch.guest_context, 1);
-    savedebug(&v->arch.guest_context, 2);
-    savedebug(&v->arch.guest_context, 3);
-    v->arch.guest_context.debugreg[6] = vmcb->dr6;
+    savedebug(v, 0);
+    savedebug(v, 1);
+    savedebug(v, 2);
+    savedebug(v, 3);
     v->arch.guest_context.debugreg[7] = vmcb->dr7;
 }
 
+/*
+ * DR7 is saved and restored on every vmexit.  Other debug registers only
+ * need to be restored if their value is going to affect execution -- i.e.,
+ * if one of the breakpoints or general detect is enabled.  So mask out all
+ * bits that don't enable some breakpoint functionality.
+ */
+#define DR7_ACTIVE_MASK 0x20ff
 
 static void __restore_debug_registers(struct vcpu *v)
 {
-    loaddebug(&v->arch.guest_context, 0);
-    loaddebug(&v->arch.guest_context, 1);
-    loaddebug(&v->arch.guest_context, 2);
-    loaddebug(&v->arch.guest_context, 3);
-    /* DR6 and DR7 are loaded from the VMCB. */
+    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+
+    cond_loaddebug(v, 0);
+    cond_loaddebug(v, 1);
+    cond_loaddebug(v, 2);
+    cond_loaddebug(v, 3);
+    vmcb->dr6 = v->arch.guest_context.debugreg[6];
+    vmcb->dr7 = v->arch.guest_context.debugreg[7];
+}
+
+static void svm_restore_dr(struct vcpu *v)
+{
+    if ( unlikely(v->arch.guest_context.debugreg[7] & DR7_ACTIVE_MASK) )
+    {
+        __restore_debug_registers(v);
+        v->arch.hvm_svm.vmcb->dr_intercepts = DR_WRITE_INTERCEPTS;
+    }
 }
 
 
@@ -421,12 +436,6 @@ static int svm_load_vmcb_ctxt(struct vcp
     return 0;
 }
 
-static void svm_restore_dr(struct vcpu *v)
-{
-    if ( unlikely(v->arch.guest_context.debugreg[7] & 0xFF) )
-        __restore_debug_registers(v);
-}
-
 static enum hvm_intblk svm_interrupt_blocked(
     struct vcpu *v, struct hvm_intack intack)
 {
@@ -1158,9 +1167,11 @@ static void svm_dr_access(struct vcpu *v
 
     HVMTRACE_0D(DR_WRITE, v);
 
+    ASSERT(!v->arch.hvm_vcpu.flag_dr_dirty);
     v->arch.hvm_vcpu.flag_dr_dirty = 1;
 
-    __restore_debug_registers(v);
+    if ( likely(!(v->arch.guest_context.debugreg[7] & DR7_ACTIVE_MASK)) )
+        __restore_debug_registers(v);
 
     /* allow the guest full access to the debug registers */
     vmcb->dr_intercepts = 0;
@@ -2270,7 +2281,8 @@ asmlinkage void svm_vmexit_handler(struc
                       TYPE_MOV_TO_CR, regs);
         break;
 
-    case VMEXIT_DR0_WRITE ... VMEXIT_DR7_WRITE:
+    case VMEXIT_DR0_READ ... VMEXIT_DR15_READ:
+    case VMEXIT_DR0_WRITE ... VMEXIT_DR15_WRITE:
         svm_dr_access(v, regs);
         break;
 
Index: 2007-10-10/xen/arch/x86/hvm/svm/vmcb.c
===================================================================
--- 2007-10-10.orig/xen/arch/x86/hvm/svm/vmcb.c 2007-10-29 08:59:07.000000000 
+0100
+++ 2007-10-10/xen/arch/x86/hvm/svm/vmcb.c      2007-10-26 16:20:28.000000000 
+0200
@@ -130,7 +130,7 @@ static int construct_vmcb(struct vcpu *v
         GENERAL2_INTERCEPT_SKINIT      | GENERAL2_INTERCEPT_RDTSCP;
 
     /* Intercept all debug-register writes. */
-    vmcb->dr_intercepts = DR_INTERCEPT_ALL_WRITES;
+    vmcb->dr_intercepts = DR_INTERCEPTS;
 
     /* Intercept all control-register accesses except for CR2 and CR8. */
     vmcb->cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
Index: 2007-10-10/xen/arch/x86/hvm/vmx/vmx.c
===================================================================
--- 2007-10-10.orig/xen/arch/x86/hvm/vmx/vmx.c  2007-10-29 08:59:07.000000000 
+0100
+++ 2007-10-10/xen/arch/x86/hvm/vmx/vmx.c       2007-10-29 09:01:31.000000000 
+0100
@@ -382,11 +382,6 @@ static enum handler_return long_mode_do_
 
 #endif /* __i386__ */
 
-#define loaddebug(_v,_reg)  \
-    __asm__ __volatile__ ("mov %0,%%db" #_reg : : "r" ((_v)->debugreg[_reg]))
-#define savedebug(_v,_reg)  \
-    __asm__ __volatile__ ("mov %%db" #_reg ",%0" : : "r" 
((_v)->debugreg[_reg]))
-
 static int vmx_guest_x86_mode(struct vcpu *v)
 {
     unsigned int cs_ar_bytes;
@@ -412,23 +407,38 @@ static void vmx_save_dr(struct vcpu *v)
     v->arch.hvm_vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
     __vmwrite(CPU_BASED_VM_EXEC_CONTROL, v->arch.hvm_vmx.exec_control);
 
-    savedebug(&v->arch.guest_context, 0);
-    savedebug(&v->arch.guest_context, 1);
-    savedebug(&v->arch.guest_context, 2);
-    savedebug(&v->arch.guest_context, 3);
-    savedebug(&v->arch.guest_context, 6);
+    savedebug(v, 0);
+    savedebug(v, 1);
+    savedebug(v, 2);
+    savedebug(v, 3);
+    savedebug(v, 6);
     v->arch.guest_context.debugreg[7] = __vmread(GUEST_DR7);
 }
 
 static void __restore_debug_registers(struct vcpu *v)
 {
-    loaddebug(&v->arch.guest_context, 0);
-    loaddebug(&v->arch.guest_context, 1);
-    loaddebug(&v->arch.guest_context, 2);
-    loaddebug(&v->arch.guest_context, 3);
+    cond_loaddebug(v, 0);
+    cond_loaddebug(v, 1);
+    cond_loaddebug(v, 2);
+    cond_loaddebug(v, 3);
     /* No 4 and 5 */
-    loaddebug(&v->arch.guest_context, 6);
-    /* DR7 is loaded from the VMCS. */
+    cond_loaddebug(v, 6);
+    __vmwrite(GUEST_DR7, v->arch.guest_context.debugreg[7]);
+}
+
+/*
+ * DR7 is saved and restored on every vmexit.  Other debug registers only
+ * need to be restored if their value is going to affect execution -- i.e.,
+ * if one of the breakpoints is enabled.  So mask out all bits that don't
+ * enable some breakpoint functionality.
+ */
+#define DR7_ACTIVE_MASK 0xff
+
+static void vmx_restore_dr(struct vcpu *v)
+{
+    /* NB. __vmread() is not usable here, so we cannot read from the VMCS. */
+    if ( unlikely(v->arch.guest_context.debugreg[7] & DR7_ACTIVE_MASK) )
+        __restore_debug_registers(v);
 }
 
 void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
@@ -704,21 +714,6 @@ static int vmx_load_vmcs_ctxt(struct vcp
     return 0;
 }
 
-/*
- * DR7 is saved and restored on every vmexit.  Other debug registers only
- * need to be restored if their value is going to affect execution -- i.e.,
- * if one of the breakpoints is enabled.  So mask out all bits that don't
- * enable some breakpoint functionality.
- */
-#define DR7_ACTIVE_MASK 0xff
-
-static void vmx_restore_dr(struct vcpu *v)
-{
-    /* NB. __vmread() is not usable here, so we cannot read from the VMCS. */
-    if ( unlikely(v->arch.guest_context.debugreg[7] & DR7_ACTIVE_MASK) )
-        __restore_debug_registers(v);
-}
-
 static void vmx_ctxt_switch_from(struct vcpu *v)
 {
     vmx_save_guest_msrs(v);
@@ -1319,10 +1314,11 @@ static void vmx_dr_access(unsigned long 
 
     HVMTRACE_0D(DR_WRITE, v);
 
+    ASSERT(!v->arch.hvm_vcpu.flag_dr_dirty);
     v->arch.hvm_vcpu.flag_dr_dirty = 1;
 
-    /* We could probably be smarter about this */
-    __restore_debug_registers(v);
+    if ( likely(!(v->arch.guest_context.debugreg[7] & DR7_ACTIVE_MASK)) )
+        __restore_debug_registers(v);
 
     /* Allow guest direct access to DR registers */
     v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MOV_DR_EXITING;
Index: 2007-10-10/xen/arch/x86/traps.c
===================================================================
--- 2007-10-10.orig/xen/arch/x86/traps.c        2007-10-29 08:59:07.000000000 
+0100
+++ 2007-10-10/xen/arch/x86/traps.c     2007-10-26 15:53:13.000000000 +0200
@@ -2323,25 +2323,25 @@ long set_debugreg(struct vcpu *p, int re
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
         if ( p == current ) 
-            asm volatile ( "mov %0, %%db0" : : "r" (value) );
+            __loaddebug(p, 0, value);
         break;
     case 1: 
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
         if ( p == current ) 
-            asm volatile ( "mov %0, %%db1" : : "r" (value) );
+            __loaddebug(p, 1, value);
         break;
     case 2: 
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
         if ( p == current ) 
-            asm volatile ( "mov %0, %%db2" : : "r" (value) );
+            __loaddebug(p, 2, value);
         break;
     case 3:
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
         if ( p == current ) 
-            asm volatile ( "mov %0, %%db3" : : "r" (value) );
+            __loaddebug(p, 3, value);
         break;
     case 6:
         /*
@@ -2351,7 +2351,7 @@ long set_debugreg(struct vcpu *p, int re
         value &= 0xffffefff; /* reserved bits => 0 */
         value |= 0xffff0ff0; /* reserved bits => 1 */
         if ( p == current ) 
-            asm volatile ( "mov %0, %%db6" : : "r" (value) );
+            __loaddebug(p, 6, value);
         break;
     case 7:
         /*
@@ -2372,7 +2372,7 @@ long set_debugreg(struct vcpu *p, int re
                 if ( ((value >> (i+16)) & 3) == 2 ) return -EPERM;
         }
         if ( p == current ) 
-            asm volatile ( "mov %0, %%db7" : : "r" (value) );
+            __loaddebug(p, 7, value);
         break;
     default:
         return -EINVAL;
Index: 2007-10-10/xen/include/asm-x86/hvm/svm/vmcb.h
===================================================================
--- 2007-10-10.orig/xen/include/asm-x86/hvm/svm/vmcb.h  2007-10-29 
08:59:07.000000000 +0100
+++ 2007-10-10/xen/include/asm-x86/hvm/svm/vmcb.h       2007-10-29 
09:09:26.000000000 +0100
@@ -151,12 +151,12 @@ enum DRInterceptBits
     DR_INTERCEPT_DR15_WRITE = 1 << 31,
 };
 
-/* for lazy save/restore we'd like to intercept all DR writes */
-#define DR_INTERCEPT_ALL_WRITES \
-    (DR_INTERCEPT_DR0_WRITE|DR_INTERCEPT_DR1_WRITE|DR_INTERCEPT_DR2_WRITE \
-    |DR_INTERCEPT_DR3_WRITE|DR_INTERCEPT_DR4_WRITE|DR_INTERCEPT_DR5_WRITE \
-    |DR_INTERCEPT_DR6_WRITE|DR_INTERCEPT_DR7_WRITE) 
-
+/* For lazy save/restore we'd like to intercept most DR accesses (DR6 and DR7
+ * are implicitly handled through the VMCB, but DR7 writes must be watched).
+ */
+#define DR_INTERCEPTS (~(DR_INTERCEPT_DR6_READ|DR_INTERCEPT_DR7_READ| \
+                         DR_INTERCEPT_DR6_WRITE))
+#define DR_WRITE_INTERCEPTS (DR_INTERCEPTS & 0xffff0000)
 
 enum VMEXIT_EXITCODE
 {
Index: 2007-10-10/xen/include/asm-x86/processor.h
===================================================================
--- 2007-10-10.orig/xen/include/asm-x86/processor.h     2007-10-29 
08:59:07.000000000 +0100
+++ 2007-10-10/xen/include/asm-x86/processor.h  2007-10-26 16:50:47.000000000 
+0200
@@ -478,6 +478,30 @@ long set_gdt(struct vcpu *d, 
              unsigned long *frames, 
              unsigned int entries);
 
+#define __loaddebug(v, reg, val) do {                                    \
+    idle_vcpu[(v)->processor]->arch.guest_context.debugreg[reg] = (val); \
+    asm volatile ( "mov %0,%%db" #reg : : "r" (val) );                   \
+} while (0)
+
+#define loaddebug(v, reg) do {                                           \
+    unsigned long __val = (v)->arch.guest_context.debugreg[reg];         \
+    __loaddebug(v, reg, __val);                                          \
+} while (0)
+
+#define cond_loaddebug(v, reg) do {                                      \
+    unsigned long __val = (v)->arch.guest_context.debugreg[reg];         \
+    if ( idle_vcpu[(v)->processor]->arch.guest_context.debugreg[reg] !=  \
+         (__val) )                                                       \
+        __loaddebug(v, reg, __val);                                      \
+} while (0)
+
+#define savedebug(v, reg) do {                                           \
+    unsigned long __val;                                                 \
+    asm volatile ("mov %%db" #reg ",%0" : "=r" (__val));                 \
+    (v)->arch.guest_context.debugreg[reg] = __val;                       \
+    idle_vcpu[(v)->processor]->arch.guest_context.debugreg[reg] = __val; \
+} while (0)
+
 long set_debugreg(struct vcpu *p, int reg, unsigned long value);
 
 struct microcode_header {



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.