x86: fix debug register handling

PATCH New Jan Beulich 1 participant 1 message Oct 29, 2007

Share & tools

Jan Beulich

author

2007-10-29 11:39 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

xen/arch/x86/acpi/suspend.c +16 ~~-10~~

Index: 2007-10-10/xen/arch/x86/acpi/suspend.c
===================================================================
@@ -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();

xen/arch/x86/domain.c +9 ~~-15~~

Index: 2007-10-10/xen/arch/x86/domain.c
===================================================================
@@ -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);
     }

xen/arch/x86/hvm/svm/svm.c +36 ~~-24~~

102 lines not on this page · raw message

xen/arch/x86/hvm/svm/vmcb.c +1 -1

Index: 2007-10-10/xen/arch/x86/hvm/svm/vmcb.c
===================================================================
@@ -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 |

xen/arch/x86/hvm/vmx/vmx.c +29 ~~-33~~

94 lines not on this page · raw message

xen/arch/x86/traps.c +6 -6

Index: 2007-10-10/xen/arch/x86/traps.c
===================================================================
@@ -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;

xen/include/asm-x86/hvm/svm/vmcb.h +6 -6

Index: 2007-10-10/xen/include/asm-x86/hvm/svm/vmcb.h
===================================================================
@@ -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
 {

xen/include/asm-x86/processor.h +24 -0

Index: 2007-10-10/xen/include/asm-x86/processor.h
===================================================================
@@ -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 {