[PATCH] x86/debug: 'Fix' ptrace dr6 output

[PATCH] x86/debug: 'Fix' ptrace dr6 output

[Peter Zijlstra]

Tom reported that one of the GDB test-cases failed, and Boris bisected
it to commit:

  d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")

The debugging session led us to commit:

  6c0aca288e72 ("x86: Ignore trap bits on single step exceptions")

Which describes a nice Wine case in it's bugzilla. Interaction between
TF and DR7 is such that TF has the highest priority, but if both are
triggered by the same instruction and are both trap, DR6 might contain
both BS and B# flags.

Commit 6c0aca288e72 made sure to not process the B# flags when the BS
flag is set, which is correct since the hardware will generate another
exception.

The result of commit d53d9bc0cf78 was that in this case the DR6
register presented to ptrace() would no longer reflect the B# flags
when BS was set.

Furthermore, previously it would pass down the DR6 bits as observed by
the hardware, which doesn't need to match the order that ptrace()
thinks (although typically there is a 1:1 mapping).

So explicitly copy DR6 B# bits when BS is set, but make sure to only
copy those bits ptrace() knows about and indexed consistent with what
ptrace expects.

This restores function to the GDB testcase and retains functionality
of the Wine test-case (which Thomas replicated in a small C program).

Noteworthy is that Wine explicitly ignores B# bits when BS is set,
because Windows isn't consistent about this either. Why GDB cares
about them is a bit of a mystery, but so be it.

Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
Reported-by: Tom de Vries <tdevries@suse.de>
Bisected-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Borislav Petkov <bp@alien8.de>
---
 arch/x86/include/asm/hw_breakpoint.h |    2 +
 arch/x86/kernel/hw_breakpoint.c      |   39 ++++++++++++++++++++++++-----------
 arch/x86/kernel/ptrace.c             |   26 ++++++++++++++++-------
 arch/x86/kernel/traps.c              |    5 ++++
 4 files changed, 52 insertions(+), 20 deletions(-)

--- a/arch/x86/include/asm/hw_breakpoint.h
+++ b/arch/x86/include/asm/hw_breakpoint.h
@@ -75,6 +75,8 @@ int decode_dr7(unsigned long dr7, int bp
 extern int arch_bp_generic_fields(int x86_len, int x86_type,
 				  int *gen_len, int *gen_type);
 
+extern int ptrace_bp_idx(struct perf_event *bp);
+
 extern struct pmu perf_ops_bp;
 
 #endif	/* _I386_HW_BREAKPOINT_H */
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -496,9 +496,32 @@ static int hw_breakpoint_handler(struct
 	dr6_p = (unsigned long *)ERR_PTR(args->err);
 	dr6 = *dr6_p;
 
-	/* If it's a single step, TRAP bits are random */
-	if (dr6 & DR_STEP)
+	/*
+	 * If DR_STEP is set, TRAP bits might also be set, but we must not
+	 * process them since another exception (without DR_STEP) will follow.
+	 */
+	if (dr6 & DR_STEP) {
+		/*
+		 * Still, userspace wants to see them, so copy those that are
+		 * due to ptrace() out into their right index.
+		 */
+		if (user_mode(args->regs)) {
+			int idx;
+
+			for (i = 0; i < HBP_NUM; i++) {
+				if (likely(!(dr6 & (DR_TRAP0 << i))))
+					continue;
+
+				bp = this_cpu_read(bp_per_reg[i]);
+				idx = ptrace_bp_idx(bp);
+				if (idx < 0)
+					continue;
+
+				current->thread.virtual_dr6 |= (DR_TRAP0 << idx);
+			}
+		}
 		return NOTIFY_DONE;
+	}
 
 	/* Do an early return if no trap bits are set in DR6 */
 	if ((dr6 & DR_TRAP_BITS) == 0)
@@ -509,14 +534,6 @@ static int hw_breakpoint_handler(struct
 		if (likely(!(dr6 & (DR_TRAP0 << i))))
 			continue;
 
-		/*
-		 * The counter may be concurrently released but that can only
-		 * occur from a call_rcu() path. We can then safely fetch
-		 * the breakpoint, use its callback, touch its counter
-		 * while we are in an rcu_read_lock() path.
-		 */
-		rcu_read_lock();
-
 		bp = this_cpu_read(bp_per_reg[i]);
 		/*
 		 * Reset the 'i'th TRAP bit in dr6 to denote completion of
@@ -540,8 +557,6 @@ static int hw_breakpoint_handler(struct
 		 */
 		if (bp->hw.info.type == X86_BREAKPOINT_EXECUTE)
 			args->regs->flags |= X86_EFLAGS_RF;
-
-		rcu_read_unlock();
 	}
 	/*
 	 * Further processing in do_debug() is needed for a) user-space
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -449,22 +449,33 @@ static int genregs_set(struct task_struc
 	return ret;
 }
 
+int ptrace_bp_idx(struct perf_event *bp)
+{
+	struct thread_struct *thread = &current->thread;
+	int i;
+
+	for (i = 0; i < HBP_NUM; i++) {
+		if (thread->ptrace_bps[i] == bp)
+			return i;
+	}
+
+	return -1;
+}
+
 static void ptrace_triggered(struct perf_event *bp,
 			     struct perf_sample_data *data,
 			     struct pt_regs *regs)
 {
-	int i;
 	struct thread_struct *thread = &(current->thread);
+	int i = ptrace_bp_idx(bp);
+
+	if (WARN_ON_ONCE(i < 0))
+		return;
 
 	/*
 	 * Store in the virtual DR6 register the fact that the breakpoint
 	 * was hit so the thread's debugger will see it.
 	 */
-	for (i = 0; i < HBP_NUM; i++) {
-		if (thread->ptrace_bps[i] == bp)
-			break;
-	}
-
 	thread->virtual_dr6 |= (DR_TRAP0 << i);
 }
 
@@ -518,8 +529,7 @@ ptrace_register_breakpoint(struct task_s
 	if (err)
 		return ERR_PTR(err);
 
-	return register_user_hw_breakpoint(&attr, ptrace_triggered,
-						 NULL, tsk);
+	return register_user_hw_breakpoint(&attr, ptrace_triggered, NULL, tsk);
 }
 
 static int ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -838,6 +840,9 @@ static bool notify_debug(struct pt_regs
 	 *
 	 * Notifiers will set bits in @virtual_dr6 to indicate the desire
 	 * for signals - ptrace_triggered(), kgdb_hw_overflow_handler().
+	 *
+	 * @dr6 is in hardware order
+	 * @virtual_dr6 is in ptrace order
 	 */
 	if (notify_die(DIE_DEBUG, "debug", regs, (long)dr6, 0, SIGTRAP) == NOTIFY_STOP)
 		return true;

Re: [PATCH] x86/debug: 'Fix' ptrace dr6 output

[Andrew Cooper]

On 28/01/2021 18:20, Peter Zijlstra wrote:
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -496,9 +496,32 @@ static int hw_breakpoint_handler(struct
>  	dr6_p = (unsigned long *)ERR_PTR(args->err);
>  	dr6 = *dr6_p;
>  
> -	/* If it's a single step, TRAP bits are random */
> -	if (dr6 & DR_STEP)
> +	/*
> +	 * If DR_STEP is set, TRAP bits might also be set, but we must not
> +	 * process them since another exception (without DR_STEP) will follow.

:) How lucky are you feeling?

Data breakpoints will in principle merge with DR_STEP (as will all other
#DB's with trap semantics), other than the many errata about breakpoints
not being recognised correctly.

Under VT-x because there is a still unfixed vmexit microcode bug which
loses all breakpoint information if DR_STEP is set.  %dr6 handling works
fine when #DB isn't intercepted, but then you get to keep the pipeline
livelock vulnerability as a consequence.

Instruction breakpoints on the following instruction will be delivered
as a second #DB, because fault style #DBs are raised at a separate
position in the instruction execution cycle.

~Andrew

Re: [PATCH] x86/debug: 'Fix' ptrace dr6 output

[Tom de Vries]

On 1/28/21 7:20 PM, Peter Zijlstra wrote:
> 
> Tom reported that one of the GDB test-cases failed, and Boris bisected
> it to commit:
> 
>   d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
> 
> The debugging session led us to commit:
> 
>   6c0aca288e72 ("x86: Ignore trap bits on single step exceptions")
> 
> Which describes a nice Wine case in it's bugzilla. Interaction between
> TF and DR7 is such that TF has the highest priority, but if both are
> triggered by the same instruction and are both trap, DR6 might contain
> both BS and B# flags.
> 
> Commit 6c0aca288e72 made sure to not process the B# flags when the BS
> flag is set, which is correct since the hardware will generate another
> exception.
> 
> The result of commit d53d9bc0cf78 was that in this case the DR6
> register presented to ptrace() would no longer reflect the B# flags
> when BS was set.
> 
> Furthermore, previously it would pass down the DR6 bits as observed by
> the hardware, which doesn't need to match the order that ptrace()
> thinks (although typically there is a 1:1 mapping).
> 
> So explicitly copy DR6 B# bits when BS is set, but make sure to only
> copy those bits ptrace() knows about and indexed consistent with what
> ptrace expects.
> 
> This restores function to the GDB testcase and retains functionality
> of the Wine test-case (which Thomas replicated in a small C program).
> 
> Noteworthy is that Wine explicitly ignores B# bits when BS is set,
> because Windows isn't consistent about this either. Why GDB cares
> about them is a bit of a mystery, but so be it.
> 

Looking in gdb.repo/gdb/nat/x86-dregs.c:
...
/* Did the watchpoint whose address is in the I'th register break?  */
#define X86_DR_WATCH_HIT(dr6, i) ((dr6) & (1 << (i)))
...
the point of B# bits seems to be to test which watchpoint hit.

My guess about why gdb wants to know about both BS and B# at the same
time: because either of them causes gdb to stop and present a user
prompt, and you want to report both events at one user stop, instead of
presenting two separate stops and user prompts at the same address.

Thanks,
- Tom

> Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
> Reported-by: Tom de Vries <tdevries@suse.de>
> Bisected-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Borislav Petkov <bp@alien8.de>
> ---
>  arch/x86/include/asm/hw_breakpoint.h |    2 +
>  arch/x86/kernel/hw_breakpoint.c      |   39 ++++++++++++++++++++++++-----------
>  arch/x86/kernel/ptrace.c             |   26 ++++++++++++++++-------
>  arch/x86/kernel/traps.c              |    5 ++++
>  4 files changed, 52 insertions(+), 20 deletions(-)
> 
> --- a/arch/x86/include/asm/hw_breakpoint.h
> +++ b/arch/x86/include/asm/hw_breakpoint.h
> @@ -75,6 +75,8 @@ int decode_dr7(unsigned long dr7, int bp
>  extern int arch_bp_generic_fields(int x86_len, int x86_type,
>  				  int *gen_len, int *gen_type);
>  
> +extern int ptrace_bp_idx(struct perf_event *bp);
> +
>  extern struct pmu perf_ops_bp;
>  
>  #endif	/* _I386_HW_BREAKPOINT_H */
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -496,9 +496,32 @@ static int hw_breakpoint_handler(struct
>  	dr6_p = (unsigned long *)ERR_PTR(args->err);
>  	dr6 = *dr6_p;
>  
> -	/* If it's a single step, TRAP bits are random */
> -	if (dr6 & DR_STEP)
> +	/*
> +	 * If DR_STEP is set, TRAP bits might also be set, but we must not
> +	 * process them since another exception (without DR_STEP) will follow.
> +	 */
> +	if (dr6 & DR_STEP) {
> +		/*
> +		 * Still, userspace wants to see them, so copy those that are
> +		 * due to ptrace() out into their right index.
> +		 */
> +		if (user_mode(args->regs)) {
> +			int idx;
> +
> +			for (i = 0; i < HBP_NUM; i++) {
> +				if (likely(!(dr6 & (DR_TRAP0 << i))))
> +					continue;
> +
> +				bp = this_cpu_read(bp_per_reg[i]);
> +				idx = ptrace_bp_idx(bp);
> +				if (idx < 0)
> +					continue;
> +
> +				current->thread.virtual_dr6 |= (DR_TRAP0 << idx);
> +			}
> +		}
>  		return NOTIFY_DONE;
> +	}
>  
>  	/* Do an early return if no trap bits are set in DR6 */
>  	if ((dr6 & DR_TRAP_BITS) == 0)
> @@ -509,14 +534,6 @@ static int hw_breakpoint_handler(struct
>  		if (likely(!(dr6 & (DR_TRAP0 << i))))
>  			continue;
>  
> -		/*
> -		 * The counter may be concurrently released but that can only
> -		 * occur from a call_rcu() path. We can then safely fetch
> -		 * the breakpoint, use its callback, touch its counter
> -		 * while we are in an rcu_read_lock() path.
> -		 */
> -		rcu_read_lock();
> -
>  		bp = this_cpu_read(bp_per_reg[i]);
>  		/*
>  		 * Reset the 'i'th TRAP bit in dr6 to denote completion of
> @@ -540,8 +557,6 @@ static int hw_breakpoint_handler(struct
>  		 */
>  		if (bp->hw.info.type == X86_BREAKPOINT_EXECUTE)
>  			args->regs->flags |= X86_EFLAGS_RF;
> -
> -		rcu_read_unlock();
>  	}
>  	/*
>  	 * Further processing in do_debug() is needed for a) user-space
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -449,22 +449,33 @@ static int genregs_set(struct task_struc
>  	return ret;
>  }
>  
> +int ptrace_bp_idx(struct perf_event *bp)
> +{
> +	struct thread_struct *thread = &current->thread;
> +	int i;
> +
> +	for (i = 0; i < HBP_NUM; i++) {
> +		if (thread->ptrace_bps[i] == bp)
> +			return i;
> +	}
> +
> +	return -1;
> +}
> +
>  static void ptrace_triggered(struct perf_event *bp,
>  			     struct perf_sample_data *data,
>  			     struct pt_regs *regs)
>  {
> -	int i;
>  	struct thread_struct *thread = &(current->thread);
> +	int i = ptrace_bp_idx(bp);
> +
> +	if (WARN_ON_ONCE(i < 0))
> +		return;
>  
>  	/*
>  	 * Store in the virtual DR6 register the fact that the breakpoint
>  	 * was hit so the thread's debugger will see it.
>  	 */
> -	for (i = 0; i < HBP_NUM; i++) {
> -		if (thread->ptrace_bps[i] == bp)
> -			break;
> -	}
> -
>  	thread->virtual_dr6 |= (DR_TRAP0 << i);
>  }
>  
> @@ -518,8 +529,7 @@ ptrace_register_breakpoint(struct task_s
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	return register_user_hw_breakpoint(&attr, ptrace_triggered,
> -						 NULL, tsk);
> +	return register_user_hw_breakpoint(&attr, ptrace_triggered, NULL, tsk);
>  }
>  
>  static int ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -838,6 +840,9 @@ static bool notify_debug(struct pt_regs
>  	 *
>  	 * Notifiers will set bits in @virtual_dr6 to indicate the desire
>  	 * for signals - ptrace_triggered(), kgdb_hw_overflow_handler().
> +	 *
> +	 * @dr6 is in hardware order
> +	 * @virtual_dr6 is in ptrace order
>  	 */
>  	if (notify_die(DIE_DEBUG, "debug", regs, (long)dr6, 0, SIGTRAP) == NOTIFY_STOP)
>  		return true;
> 

[PATCH v2] x86/debug: Fix DR6 handling

[Peter Zijlstra]

Tom reported that one of the GDB test-cases failed, and Boris bisected
it to commit:

  d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")

The debugging session led us to commit:

  6c0aca288e72 ("x86: Ignore trap bits on single step exceptions")

It turns out that TF and data breakpoints are both traps and will be
merged, while instruction breakpoints are faults and will not be
merged. This means 6c0aca288e72 is wrong, we only need to exclude TF
and instruction breakpoints while we can merge TF and data
breakpoints.

Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
Fixes: 6c0aca288e72 ("x86: Ignore trap bits on single step exceptions")
Reported-by: Tom de Vries <tdevries@suse.de>
Bisected-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/hw_breakpoint.c |   39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -491,15 +491,12 @@ static int hw_breakpoint_handler(struct
 	struct perf_event *bp;
 	unsigned long *dr6_p;
 	unsigned long dr6;
+	bool bpx;
 
 	/* The DR6 value is pointed by args->err */
 	dr6_p = (unsigned long *)ERR_PTR(args->err);
 	dr6 = *dr6_p;
 
-	/* If it's a single step, TRAP bits are random */
-	if (dr6 & DR_STEP)
-		return NOTIFY_DONE;
-
 	/* Do an early return if no trap bits are set in DR6 */
 	if ((dr6 & DR_TRAP_BITS) == 0)
 		return NOTIFY_DONE;
@@ -509,28 +506,29 @@ static int hw_breakpoint_handler(struct
 		if (likely(!(dr6 & (DR_TRAP0 << i))))
 			continue;
 
+		bp = this_cpu_read(bp_per_reg[i]);
+		if (!bp)
+			continue;
+
+		bpx = bp->hw.info.type == X86_BREAKPOINT_EXECUTE;
+
 		/*
-		 * The counter may be concurrently released but that can only
-		 * occur from a call_rcu() path. We can then safely fetch
-		 * the breakpoint, use its callback, touch its counter
-		 * while we are in an rcu_read_lock() path.
+		 * TF and data breakpoints are traps and can be merged, however
+		 * instruction breakpoints are faults and will be raised
+		 * separately.
+		 *
+		 * However DR6 can indicate both TF and instruction
+		 * breakpoints. In that case take TF as that has precedence and
+		 * delay the instruction breakpoint for the next exception.
 		 */
-		rcu_read_lock();
+		if (bpx && (dr6 & DR_STEP))
+			continue;
 
-		bp = this_cpu_read(bp_per_reg[i]);
 		/*
 		 * Reset the 'i'th TRAP bit in dr6 to denote completion of
 		 * exception handling
 		 */
 		(*dr6_p) &= ~(DR_TRAP0 << i);
-		/*
-		 * bp can be NULL due to lazy debug register switching
-		 * or due to concurrent perf counter removing.
-		 */
-		if (!bp) {
-			rcu_read_unlock();
-			break;
-		}
 
 		perf_bp_event(bp, args->regs);
 
@@ -538,11 +536,10 @@ static int hw_breakpoint_handler(struct
 		 * Set up resume flag to avoid breakpoint recursion when
 		 * returning back to origin.
 		 */
-		if (bp->hw.info.type == X86_BREAKPOINT_EXECUTE)
+		if (bpx)
 			args->regs->flags |= X86_EFLAGS_RF;
-
-		rcu_read_unlock();
 	}
+
 	/*
 	 * Further processing in do_debug() is needed for a) user-space
 	 * breakpoints (to generate signals) and b) when the system has

[PATCH] selftests: breakpoints: Add "WINE" test for x86

[Thomas Gleixner]

The DR6 handling failure which made GDB upset was caused by a commit
which addressed a WINE test case regression. The test case runs a
trivial

        NOP; NOP; RET;

sequence in the tracee. The tracer runs the following steps:

 1 - Set data breakpoint on the first instruction in DR0
   - Continue tracee, wait for trap
   - Expect DR6::BS == 0 and DR6::BR0 == 1 and IP == first instruction

 2 - Clear DR6, set TF in EFLAGS
   - Set data breakpoint on the second instruction in DR0
   - Continue tracee, wait for trap
   - Expect DR6::BS == 1 and DR6::BR0 == X and IP == second instruction

 3 - Clear DR6, set TF in EFLAGS
   - Continue tracee, wait for trap
   - Expect DR6::BS == 0 and DR6::BR0 == 1 and IP == second instruction

 4 - Clear DR6, set TF in EFLAGS
   - Remove data breakpoint from DR0
   - Continue tracee, wait for trap
   - Expect DR6::BS == 1 and DR6::BR0 == 0 and IP == third instruction

The important step is #2. WINE does not care about the state of DR6::BR0
as Windows versions seem to be inconsistent. The offending commit just
excluded the BR bits when delivering the single step (BS == 1) which
made WINE happy, but broke GDB which expects the BR bits to be merged
when the single step resulted in triggering an armed data breakpoint at
the same time.

Add a test case which covers this scenario. This is modeled after the
WINE testcase, but changes the expect in step #2 to:

   - Expect DR6::BS == 1 and DR6::BR0 == 1 and IP == second instruction

to ensure that the GDB expectations are met as well.

Make it work for both 32 and 64 bit and fix the broken calculation of
number of tests for 32 bit as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 tools/testing/selftests/breakpoints/breakpoint_test.c |  115 ++++++++++++++++--
 1 file changed, 107 insertions(+), 8 deletions(-)

--- a/tools/testing/selftests/breakpoints/breakpoint_test.c
+++ b/tools/testing/selftests/breakpoints/breakpoint_test.c
@@ -23,6 +23,13 @@
 #define COUNT_ISN_BPS	4
 #define COUNT_WPS	4
 
+#ifdef __i386__
+# define rip		eip
+# define COUNT_LEN	3
+#else
+# define COUNT_LEN	4
+#endif
+
 /* Breakpoint access modes */
 enum {
 	BP_X = 1,
@@ -50,6 +57,16 @@ static void set_breakpoint_addr(void *ad
 			strerror(errno));
 }
 
+static void set_dr7(unsigned long dr7)
+{
+	int ret;
+
+	ret = ptrace(PTRACE_POKEUSER, child_pid,
+		     offsetof(struct user, u_debugreg[7]), dr7);
+	if (ret)
+		ksft_exit_fail_msg("Can't set dr7: %s\n", strerror(errno));
+}
+
 static void toggle_breakpoint(int n, int type, int len,
 			      int local, int global, int set)
 {
@@ -105,12 +122,7 @@ static void toggle_breakpoint(int n, int
 	else
 		dr7 &= ~vdr7;
 
-	ret = ptrace(PTRACE_POKEUSER, child_pid,
-		     offsetof(struct user, u_debugreg[7]), dr7);
-	if (ret) {
-		ksft_print_msg("Can't set dr7: %s\n", strerror(errno));
-		exit(-1);
-	}
+	set_dr7(dr7);
 }
 
 /* Dummy variables to test read/write accesses */
@@ -196,6 +208,13 @@ static void read_var(int len)
 	}
 }
 
+void __wine_test(void);
+
+static void wine_test(void)
+{
+	asm volatile (".global __wine_test; __wine_test: nop; nop; ret\n");
+}
+
 /*
  * Do the r/w/x accesses to trigger the breakpoints. And run
  * the usual traps.
@@ -258,6 +277,11 @@ static void trigger_tests(void)
 	asm("int $3\n");
 	check_trapped();
 
+	/*
+	 * The wine test: NOP, NOP, RET with singlestep and data breakpoint.
+	 */
+	wine_test();
+
 	kill(getpid(), SIGUSR1);
 }
 
@@ -327,6 +351,76 @@ static void launch_watchpoints(char *buf
 	}
 }
 
+static void wine_test_step(int step, void *ip, int bs, int b0, char *buf)
+{
+	unsigned long eflags, dr6, child_ip, ipoffs;
+	int dr6_bs, dr6_b0, status;
+
+	ptrace(PTRACE_CONT, child_pid, NULL, 0);
+	wait(&status);
+
+	if (WSTOPSIG(status) != SIGTRAP)
+		ksft_exit_fail_msg("Expected SIGTRAP got %d\n", status);
+
+	/* Get DR6 from the child and clear it */
+	dr6 = ptrace(PTRACE_PEEKUSER, child_pid,
+		     offsetof(struct user, u_debugreg[6]), 0),
+	ptrace(PTRACE_POKEUSER, child_pid,
+	       offsetof(struct user, u_debugreg[6]), 0);
+
+	/* Get the IP from the child */
+	child_ip = ptrace(PTRACE_PEEKUSER, child_pid,
+			  offsetof(struct user, regs.rip), 0);
+
+	/* Except for the last step, set TF in eflags */
+	if (step < 3) {
+		eflags = ptrace(PTRACE_PEEKUSER, child_pid,
+				offsetof(struct user, regs.eflags), 0);
+		ptrace(PTRACE_POKEUSER, child_pid,
+		       offsetof(struct user, regs.eflags), eflags | 0x100);
+	}
+
+	/* Calculate the IP offset and isolate the DR6 bits to check */
+	ipoffs = (unsigned long) ip - child_ip;
+	dr6_bs = !!(dr6 & 0x4000);
+	dr6_b0 = !!(dr6 & 0x0001);
+
+	sprintf(buf, "Test wine_test step: %d dr6_bs: %d (%d) dr6_b0: %d (%d) ipoffs: %lx\n",
+		step, dr6_bs, bs, dr6_b0, b0, ipoffs);
+
+	nr_tests++;
+
+	/* Fail if IPOFFS != 0 or BS, B0 are not matching */
+	if (ipoffs || bs != dr6_bs || b0 != dr6_b0)
+		ksft_test_result_fail(buf);
+	else
+		ksft_test_result_pass(buf);
+}
+
+static void launch_wine_test(char *buf)
+{
+	void *addr = __wine_test;
+
+	/* Data break point (RW) on first instruction */
+	set_breakpoint_addr(addr, 0);
+	set_dr7(0x03);
+	/* Expect: DR6::BS == 0 DR6::BR0 == 1 IP == instr[0] */
+	wine_test_step(0, addr, 0, 1, buf);
+	/* Data break point (RW) on second instruction */
+	set_breakpoint_addr(++addr, 0);
+	/*
+	 * Expect: DR6::BS == 1 DR6::BR0 == 1  IP == instr[1]
+	 * Wine does not care about BR0 here but GDB does ...
+	 */
+	wine_test_step(1, addr, 1, 1, buf);
+	/* Expect: DR6::BS == 0 DR6::BR0 == 1  IP == instr[1] */
+	wine_test_step(2, addr, 0, 1, buf);
+	/* Remove the data breakpoint
+	set_breakpoint_addr(NULL, 0);
+	/* Expect: DR6::BS == 1 DR6::BR0 == 0 IP == instr[2] */
+	wine_test_step(3, ++addr, 1, 0, buf);
+}
+
 /* Set the breakpoints and check the child successfully trigger them */
 static void launch_tests(void)
 {
@@ -335,9 +429,12 @@ static void launch_tests(void)
 	int len, local, global, i;
 
 	tests += 3 * COUNT_ISN_BPS;
-	tests += sizeof(long) / 2 * 3 * COUNT_WPS;
-	tests += sizeof(long) / 2 * 3 * COUNT_WPS;
+	tests += COUNT_LEN * 3 * COUNT_WPS;
+	tests += COUNT_LEN * 3 * COUNT_WPS;
+	/* ICEBP, INT3 */
 	tests += 2;
+	/* WINE test */
+	tests += 4;
 	ksft_set_plan(tests);
 
 	/* Instruction breakpoints */
@@ -381,6 +478,8 @@ static void launch_tests(void)
 	ptrace(PTRACE_CONT, child_pid, NULL, 0);
 	check_success("Test int 3 trap\n");
 
+	launch_wine_test(buf);
+
 	ptrace(PTRACE_CONT, child_pid, NULL, 0);
 }
 

Re: [PATCH v2] x86/debug: Fix DR6 handling

[Borislav Petkov]

On Fri, Jan 29, 2021 at 04:41:09PM +0100, Oleg Nesterov wrote:
> This seems to fix the problem reported by Jan, see his test-case below.

Should it be part of

tools/testing/selftests/breakpoints/

?

tglx has one destined for there already, wouldn't hurt to have a second
one:

https://lkml.kernel.org/r/87eei4d4k6.fsf@nanos.tec.linutronix.de

after applying kernel coding style to that one.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2] x86/debug: Fix DR6 handling

[Tom de Vries]

On 1/29/21 3:48 PM, Borislav Petkov wrote:
> On Thu, Jan 28, 2021 at 10:16:27PM +0100, Peter Zijlstra wrote:
>>
>> Tom reported that one of the GDB test-cases failed, and Boris bisected
>> it to commit:
>>
>>   d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
>>
>> The debugging session led us to commit:
>>
>>   6c0aca288e72 ("x86: Ignore trap bits on single step exceptions")
>>
>> It turns out that TF and data breakpoints are both traps and will be
>> merged, while instruction breakpoints are faults and will not be
>> merged. This means 6c0aca288e72 is wrong, we only need to exclude TF
>> and instruction breakpoints while we can merge TF and data
>> breakpoints.
>>
>> Fixes: d53d9bc0cf78 ("x86/debug: Change thread.debugreg6 to thread.virtual_dr6")
>> Fixes: 6c0aca288e72 ("x86: Ignore trap bits on single step exceptions")
>> Reported-by: Tom de Vries <tdevries@suse.de>
>> Bisected-by: Borislav Petkov <bp@alien8.de>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> I guess
> 
> Cc: <stable@vger.kernel.org>
> 
> Also,
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>
> 
> And gdb testsuite is a bit happier:
> 
> --- before
> +++ after
>                  === gdb Summary ===
>  
> -# of expected passes            70822
> -# of unexpected failures        899
> +# of expected passes            70852
> +# of unexpected failures        869
>  # of expected failures          74
>  # of known failures             99
>  # of untested testcases         114
> 
> You just fixed 30(!) testcases.
> 
> :-)
> 

Hi Boris,

thanks for testing this, and just to confirm: the total number of
regressions I see in the gdb testsuite related to watchpoints is indeed 30.

Thanks,
- Tom

Re: [PATCH] selftests: breakpoints: Add "WINE" test for x86

[Thomas Gleixner]

On Fri, Jan 29 2021 at 10:08, Peter Zijlstra wrote:
> On Fri, Jan 29, 2021 at 12:28:41AM +0100, Thomas Gleixner wrote:
>
>> Add a test case which covers this scenario. This is modeled after the
>> WINE testcase, but changes the expect in step #2 to:
>> 
>>    - Expect DR6::BS == 1 and DR6::BR0 == 1 and IP == second instruction
>> 
>> to ensure that the GDB expectations are met as well.
>
>> +	/*
>> +	 * Expect: DR6::BS == 1 DR6::BR0 == 1  IP == instr[1]
>> +	 * Wine does not care about BR0 here but GDB does ...
>> +	 */
>> +	wine_test_step(1, addr, 1, 1, buf);
>
>
> So my v2 patch will fail this, while it will pass the actual gdb
> testcase.
>
> The thing it does is process _data_ breakpoints along with TF, but it
> will exclude instruction breakpoints and TF.
>
> Since the above test is using instruction breakpoints, it will report
> 0x4000 and 0x0001 respectively for two consequtive exceptions.

Yes, I'm a moron....