Set all active-low DR6 bits when resetting DR6 between testcases, as clearing active-low bits causes test failures when run on (virtual) CPUs that support such bits, due to the checks all expecting the active-low bits to be set. The bug has gone unnoticed because the default config uses a virtual CPU model that doesn't support any active-low bits. Signed-off-by: Sean Christopherson --- x86/debug.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/x86/debug.c b/x86/debug.c index eef0dfd8..c677af99 100644 --- a/x86/debug.c +++ b/x86/debug.c @@ -100,7 +100,7 @@ static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn) bool ign; n = 0; - write_dr6(0); + write_dr6(DR6_ACTIVE_LOW); start = test(); report_fn(start, ""); @@ -114,7 +114,7 @@ static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn) return; n = 0; - write_dr6(0); + write_dr6(DR6_ACTIVE_LOW); /* * Run the test in usermode. Use the expected start RIP from the first @@ -336,7 +336,7 @@ static void report_singlestep_with_movss_blocking_and_dr7_gd(unsigned long start static noinline unsigned long singlestep_with_movss_blocking_and_dr7_gd(void) { - unsigned long start_rip; + unsigned long scratch = DR6_ACTIVE_LOW; write_dr7(DR7_GD); @@ -348,7 +348,6 @@ static noinline unsigned long singlestep_with_movss_blocking_and_dr7_gd(void) * General Detect #DB. */ asm volatile( - "xor %0, %0\n\t" "pushf\n\t" "pop %%rax\n\t" "or $(1<<8),%%rax\n\t" @@ -361,9 +360,9 @@ static noinline unsigned long singlestep_with_movss_blocking_and_dr7_gd(void) "push %%rax\n\t" "popf\n\t" "lea 1b(%%rip),%0\n\t" - : "=r" (start_rip) : : "rax" + : "+r" (scratch) :: "rax" ); - return start_rip; + return scratch; } static void report_singlestep_with_sti_hlt(unsigned long start, @@ -480,7 +479,7 @@ int main(int ac, char **av) write_cr4(cr4 | X86_CR4_DE); read_dr4(); report(got_ud, "DR4 read got #UD with CR4.DE == 1"); - write_dr6(0); + write_dr6(DR6_ACTIVE_LOW); extern unsigned char sw_bp; asm volatile("int3; sw_bp:"); @@ -509,7 +508,7 @@ int main(int ac, char **av) n = 0; extern unsigned char hw_bp2; write_dr2(&hw_bp2); - write_dr6(DR6_BS | DR6_TRAP1); + write_dr6(DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1); asm volatile("hw_bp2: nop"); report(n == 1 && db_addr[0] == ((unsigned long)&hw_bp2) && @@ -528,7 +527,7 @@ int main(int ac, char **av) n = 0; write_dr1((void *)&value); - write_dr6(DR6_BS); + write_dr6(DR6_ACTIVE_LOW | DR6_BS); write_dr7(0x00d0040a); // 4-byte write extern unsigned char hw_wp1; @@ -542,7 +541,7 @@ int main(int ac, char **av) "hw watchpoint (test that dr6.BS is not cleared)"); n = 0; - write_dr6(0); + write_dr6(DR6_ACTIVE_LOW); extern unsigned char hw_wp2; asm volatile( @@ -555,7 +554,7 @@ int main(int ac, char **av) "hw watchpoint (test that dr6.BS is not set)"); n = 0; - write_dr6(0); + write_dr6(DR6_ACTIVE_LOW); extern unsigned char sw_icebp; asm volatile(".byte 0xf1; sw_icebp:"); report(n == 1 && -- 2.56.0.rc1.315.gc6ed9934b7-goog Run the debug test with "host" so that it picks up features like Bus Lock Detect. Signed-off-by: Sean Christopherson --- x86/unittests.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/x86/unittests.cfg b/x86/unittests.cfg index eb953249..cddcf642 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -485,6 +485,7 @@ check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y [debug] file = debug.flat +qemu_params = -cpu host arch = x86_64 [hyperv_synic] -- 2.56.0.rc1.315.gc6ed9934b7-goog Run the main debug testcases with a control DR6 value (the value written to DR6 prior to doing a #DB test) with both active-low bits set and active-low bits clear. Setting only one or the other means the test will miss bugs, e.g. will fail to detect cases where hardware/KVM incorrectlys sets/clears an active-low bit. Note, DR6.RTM and DR6.BLD have different semantics. DR6.RTM is modified by all #DBs, i.e. is explicitly set/cleared based on whether or not a #DB occurred in an RTM region. DR6.BLD on the other hand is never supposed to be set by hardware; it's cleared on Bus Lock #DBs, and otherwise isn't modified. Or at least, it's not supposed to be modified. AMD CPUs appear to have a ucode bug where DR6.BLD is forced to '1' on *all* writes to DR6 if Bus Lock Detect isn't fully enabled, including DR6 loads via VMRUN. To fudge around the quirk, simply enable BUS_LOCK_DETECT in DEBUGCTL so that the test can write DR6.BLD at will. As a bonus, enabling BUS_LOCK_DETECT will help catch spurious Bus Lock #DBs. Don't bother running the test that clobbers the #DB IDT entry directly with both configurations; just run it once at the end to avoid having to restore the IDT. Signed-off-by: Sean Christopherson --- x86/debug.c | 109 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 83 insertions(+), 26 deletions(-) diff --git a/x86/debug.c b/x86/debug.c index c677af99..5427797f 100644 --- a/x86/debug.c +++ b/x86/debug.c @@ -16,6 +16,9 @@ #include "desc.h" #include "usermode.h" +static unsigned long dr6_control_value; +static unsigned long dr6_base_value; + static volatile unsigned long bp_addr; static volatile unsigned long db_addr[10], dr6[10]; static volatile unsigned int n; @@ -49,17 +52,17 @@ static void handle_db(struct ex_regs *regs) static inline bool is_single_step_db(unsigned long dr6_val) { - return dr6_val == (DR6_ACTIVE_LOW | DR6_BS); + return dr6_val == (dr6_base_value | DR6_BS); } static inline bool is_general_detect_db(unsigned long dr6_val) { - return dr6_val == (DR6_ACTIVE_LOW | DR6_BD); + return dr6_val == (dr6_base_value | DR6_BD); } static inline bool is_icebp_db(unsigned long dr6_val) { - return dr6_val == DR6_ACTIVE_LOW; + return dr6_val == dr6_base_value; } extern unsigned char handle_db_save_rip; @@ -100,7 +103,7 @@ static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn) bool ign; n = 0; - write_dr6(DR6_ACTIVE_LOW); + write_dr6(dr6_control_value); start = test(); report_fn(start, ""); @@ -114,7 +117,7 @@ static void __run_single_step_db_test(db_test_fn test, db_report_fn report_fn) return; n = 0; - write_dr6(DR6_ACTIVE_LOW); + write_dr6(dr6_control_value); /* * Run the test in usermode. Use the expected start RIP from the first @@ -336,7 +339,7 @@ static void report_singlestep_with_movss_blocking_and_dr7_gd(unsigned long start static noinline unsigned long singlestep_with_movss_blocking_and_dr7_gd(void) { - unsigned long scratch = DR6_ACTIVE_LOW; + unsigned long scratch = dr6_control_value; write_dr7(DR7_GD); @@ -452,17 +455,34 @@ static void bus_lock_test(void) got_ac = false; } -int main(int ac, char **av) +static void run_tests(unsigned long __dr6_control_value) { + u64 debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); unsigned long cr4; - handle_exception(DB_VECTOR, handle_db); - handle_exception(BP_VECTOR, handle_bp); - handle_exception(UD_VECTOR, handle_ud); - handle_exception(AC_VECTOR, handle_ac); + dr6_control_value = __dr6_control_value; + + /* + * DR6.RTM is modified on all #DBs, and is '0' if and only if the #DB + * occurred in an RTM region. This test doesn't do RTM, and so DR6.RTM + * should always be set, even if it's '0' in the control value. + */ + dr6_base_value = dr6_control_value | DR6_FIXED_1 | DR6_RTM; + + /* DR6.BLD is fixed-1 if Bus Lock Detect is supported. */ + if (!this_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) + dr6_base_value |= DR6_BUS_LOCK; bus_lock_test(); + /* + * Enable Bus Lock Detect to workaround an AMD ucode bug where DR6.BLD + * is clobbered to '1', i.e. is "reset" (it's an active-low bit), on + * *any* DR6 load, including asynchronous loads via #VMEXIT => VMRUN. + */ + if (this_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) + wrmsr(MSR_IA32_DEBUGCTLMSR, debugctl | DEBUGCTLMSR_BUS_LOCK_DETECT); + /* * DR4 is an alias for DR6 (and DR5 aliases DR7) if CR4.DE is NOT set, * and is reserved if CR4.DE=1 (Debug Extensions enabled). @@ -471,15 +491,16 @@ int main(int ac, char **av) cr4 = read_cr4(); write_cr4(cr4 & ~X86_CR4_DE); write_dr4(0); - write_dr6(DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1); - report(read_dr4() == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1) && !got_ud, - "DR4==DR6 with CR4.DE == 0"); + write_dr6(dr6_control_value | DR6_BS | DR6_TRAP1); + report(read_dr4() == read_dr6() && !got_ud, + "DR4 (0x%lx) == DR6 (0x%lx) with CR4.DE == 0", + read_dr4(), read_dr6()); cr4 = read_cr4(); write_cr4(cr4 | X86_CR4_DE); read_dr4(); report(got_ud, "DR4 read got #UD with CR4.DE == 1"); - write_dr6(DR6_ACTIVE_LOW); + write_dr6(dr6_control_value); extern unsigned char sw_bp; asm volatile("int3; sw_bp:"); @@ -500,21 +521,21 @@ int main(int ac, char **av) asm volatile("hw_bp1: nop"); report(n == 1 && db_addr[0] == ((unsigned long)&hw_bp1) && - dr6[0] == (DR6_ACTIVE_LOW | DR6_TRAP2), + dr6[0] == (dr6_base_value | DR6_TRAP2), "Wanted #DB on 0x%lx w/ DR6 = 0x%lx, got %u #DBs, addr[0] = 0x%lx, DR6 = 0x%lx", - ((unsigned long)&hw_bp1), DR6_ACTIVE_LOW | DR6_TRAP2, + ((unsigned long)&hw_bp1), dr6_base_value | DR6_TRAP2, n, db_addr[0], dr6[0]); n = 0; extern unsigned char hw_bp2; write_dr2(&hw_bp2); - write_dr6(DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1); + write_dr6(dr6_control_value | DR6_BS | DR6_TRAP1); asm volatile("hw_bp2: nop"); report(n == 1 && db_addr[0] == ((unsigned long)&hw_bp2) && - dr6[0] == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP2), + dr6[0] == (dr6_base_value | DR6_BS | DR6_TRAP2), "Wanted #DB on 0x%lx w/ DR6 = 0x%lx, got %u #DBs, addr[0] = 0x%lx, DR6 = 0x%lx", - ((unsigned long)&hw_bp2), DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP2, + ((unsigned long)&hw_bp2), dr6_base_value | DR6_BS | DR6_TRAP2, n, db_addr[0], dr6[0]); run_ss_db_test(singlestep_basic); @@ -527,7 +548,7 @@ int main(int ac, char **av) n = 0; write_dr1((void *)&value); - write_dr6(DR6_ACTIVE_LOW | DR6_BS); + write_dr6(dr6_control_value | DR6_BS); write_dr7(0x00d0040a); // 4-byte write extern unsigned char hw_wp1; @@ -537,11 +558,11 @@ int main(int ac, char **av) : "=m" (value) : : "rax"); report(n == 1 && db_addr[0] == ((unsigned long)&hw_wp1) && - dr6[0] == (DR6_ACTIVE_LOW | DR6_BS | DR6_TRAP1), + dr6[0] == (dr6_base_value | DR6_BS | DR6_TRAP1), "hw watchpoint (test that dr6.BS is not cleared)"); n = 0; - write_dr6(DR6_ACTIVE_LOW); + write_dr6(dr6_control_value); extern unsigned char hw_wp2; asm volatile( @@ -550,18 +571,36 @@ int main(int ac, char **av) : "=m" (value) : : "rax"); report(n == 1 && db_addr[0] == ((unsigned long)&hw_wp2) && - dr6[0] == (DR6_ACTIVE_LOW | DR6_TRAP1), + dr6[0] == (dr6_base_value | DR6_TRAP1), "hw watchpoint (test that dr6.BS is not set)"); n = 0; - write_dr6(DR6_ACTIVE_LOW); + write_dr6(dr6_control_value); extern unsigned char sw_icebp; asm volatile(".byte 0xf1; sw_icebp:"); report(n == 1 && - db_addr[0] == (unsigned long)&sw_icebp && dr6[0] == DR6_ACTIVE_LOW, + db_addr[0] == (unsigned long)&sw_icebp && dr6[0] == dr6_base_value, "icebp"); + write_dr7(DR7_FIXED_1); + write_dr0(0); + write_dr1(0); + write_dr2(0); + write_dr3(0); + write_dr6(DR6_ACTIVE_LOW); + write_cr4(cr4); + wrmsr(MSR_IA32_DEBUGCTLMSR, debugctl); + + n = 0; + value = 0; + got_ud = false; + got_ac = false; +} + +static void test_watchpoints_precise(void) +{ write_dr7(0x400); + write_dr1((void *)&value); value = KERNEL_DS; write_dr7(0x00f0040a); // 4-byte read or write @@ -606,5 +645,23 @@ int main(int ac, char **av) extern unsigned char sw_bp2; report(n == 3 && bp_addr == (unsigned long)&sw_bp2, "MOV SS + watchpoint + INT3"); +} + +int main(int ac, char **av) +{ + handle_exception(DB_VECTOR, handle_db); + handle_exception(BP_VECTOR, handle_bp); + handle_exception(UD_VECTOR, handle_ud); + handle_exception(AC_VECTOR, handle_ac); + + run_tests(DR6_ACTIVE_LOW); + run_tests(0); + + /* + * Run the "precise" test only once as it is destructive (clobbers the + * #DB IDT entry). + */ + test_watchpoints_precise(); + return report_summary(); } -- 2.56.0.rc1.315.gc6ed9934b7-goog Extend the debug test to verify that bus locks to atomic accesses that split cache lines actually trigger Bus Lock Detect #DBs, while still allowing the access to go through. Note, as already commented in the test, Bus Lock Detect only applies to CPL>0. Signed-off-by: Sean Christopherson --- x86/debug.c | 48 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/x86/debug.c b/x86/debug.c index 5427797f..d359a281 100644 --- a/x86/debug.c +++ b/x86/debug.c @@ -429,30 +429,60 @@ static noinline uint64_t bus_lock(uint64_t magic) return READ_ONCE(*(uint64_t *)val); } -static void bus_lock_test(void) +static void bus_lock_test(u64 debugctl) { const uint64_t magic = 0xdeadbeefdeadbeefull; - bool bus_lock_db = false; uint64_t val; + bool ign; + + if (debugctl & DEBUGCTLMSR_BUS_LOCK_DETECT) + report_fail("DEBUGCTL.BUS_LOCK_DETECT should be '0' at RESET"); /* * Generate a bus lock (via a locked access that splits cache lines) * in CPL0 and again in CPL3 (Bus Lock Detect only affects CPL3), and * verify that no #AC or #DB is generated (the relevant features are - * not enabled). + * not yet enabled). */ val = bus_lock(magic); report(!got_ac && !n && val == magic, "CPL0 Split Lock #AC = %u (#DB = %u), val = %lx (wanted %lx)", got_ac, n, val, magic); - val = run_in_user((usermode_func)bus_lock, DB_VECTOR, magic, 0, 0, 0, &bus_lock_db); - report(!bus_lock_db && val == magic, - "CPL3 Bus Lock #DB = %u, val = %lx (wanted %lx)", - bus_lock_db, val, magic); + val = run_in_user((usermode_func)bus_lock, GP_VECTOR, magic, 0, 0, 0, &ign); + report(!got_ac && !n && val == magic, + "CPL3 Bus Lock #AC = %u, #DB = %u, val = %lx (wanted %lx)", + got_ac, n, val, magic); - n = 0; got_ac = false; + + if (!this_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) + return; + + wrmsr(MSR_IA32_DEBUGCTLMSR, debugctl | DEBUGCTLMSR_BUS_LOCK_DETECT); + + val = bus_lock(magic); + report(!got_ac && !n && val == magic, + "CPL0 Split Lock shouldn't trigger Bus Lock Detect: #DB = %u, #AC = %u, val = %lx (wanted %lx)", + n, got_ac, val, magic); + + val = run_in_user((usermode_func)bus_lock, GP_VECTOR, magic, 0, 0, 0, &ign); + report(n == 1 && !(dr6[0] & DR6_BUS_LOCK) && val == magic && !got_ac, + "CPL3 Bus Lock Detect #DB (nr #DBs = %u, DR6 = %lx, #AC = %u, val = %lx (wanted %lx))", + n, dr6[0], got_ac, val, magic); + + n = 0; + write_dr7(DR7_FIXED_1 | DR7_GD); + write_dr7(DR7_FIXED_1); + + /* Verify DR6.BLD is preserved on an unrelated #DB. */ + report(n == 1 && !(dr6[0] & DR6_BUS_LOCK) && (dr6[0] & DR6_BD), + "DR6.BLD preserved on General Detect #DB (nr #DBs = %u, DR6 = %lx)", + n, dr6[0]); + + n = 0; + write_dr6(dr6_control_value); + wrmsr(MSR_IA32_DEBUGCTLMSR, debugctl); } static void run_tests(unsigned long __dr6_control_value) @@ -473,7 +503,7 @@ static void run_tests(unsigned long __dr6_control_value) if (!this_cpu_has(X86_FEATURE_BUS_LOCK_DETECT)) dr6_base_value |= DR6_BUS_LOCK; - bus_lock_test(); + bus_lock_test(debugctl); /* * Enable Bus Lock Detect to workaround an AMD ucode bug where DR6.BLD -- 2.56.0.rc1.315.gc6ed9934b7-goog