Re: [PATCH] ARM: hw_breakpoint: Handle inexact watchpoint addresses

[Matthias Kaehlcke]

On Sat, Oct 19, 2019 at 11:12:26AM -0700, Douglas Anderson wrote:
> This is commit fdfeff0f9e3d ("arm64: hw_breakpoint: Handle inexact
> watchpoint addresses") but ported to arm32, which has the same
> problem.
> 
> This problem was found by Android CTS tests, notably the
> "watchpoint_imprecise" test [1].  I tested locally against a copycat
> (simplified) version of the test though.
> 
> [1] https://android.googlesource.com/platform/bionic/+/master/tests/sys_ptrace_test.cpp
> 
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> 
>  arch/arm/kernel/hw_breakpoint.c | 96 ++++++++++++++++++++++++---------
>  1 file changed, 70 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index b0c195e3a06d..d394878409db 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -680,26 +680,62 @@ static void disable_single_step(struct perf_event *bp)
>  	arch_install_hw_breakpoint(bp);
>  }
>  
> +/*
> + * Arm32 hardware does not always report a watchpoint hit address that matches
> + * one of the watchpoints set. It can also report an address "near" the
> + * watchpoint if a single instruction access both watched and unwatched
> + * addresses. There is no straight-forward way, short of disassembling the
> + * offending instruction, to map that address back to the watchpoint. This
> + * function computes the distance of the memory access from the watchpoint as a
> + * heuristic for the likelyhood that a given access triggered the watchpoint.
> + *
> + * See this same function in the arm64 platform code, which has the same
> + * problem.
> + *
> + * The function returns the distance of the address from the bytes watched by
> + * the watchpoint. In case of an exact match, it returns 0.
> + */
> +static u32 get_distance_from_watchpoint(unsigned long addr, u32 val,
> +					struct arch_hw_breakpoint_ctrl *ctrl)
> +{
> +	u32 wp_low, wp_high;
> +	u32 lens, lene;
> +
> +	lens = __ffs(ctrl->len);

Doesn't this always end up with 'lens == 0'? IIUC ctrl->len can have
the values ARM_BREAKPOINT_LEN_{1,2,4,8}:

#define ARM_BREAKPOINT_LEN_1	0x1
#define ARM_BREAKPOINT_LEN_2	0x3
#define ARM_BREAKPOINT_LEN_4	0xf
#define ARM_BREAKPOINT_LEN_8	0xff

> +	lene = __fls(ctrl->len);
> +
> +	wp_low = val + lens;
> +	wp_high = val + lene;

First I thought these values are off by one, but in difference to
ffs() from glibc the kernel functions start with index 0, instead
of using zero as 'no bit set'.

> +	if (addr < wp_low)
> +		return wp_low - addr;
> +	else if (addr > wp_high)
> +		return addr - wp_high;
> +	else
> +		return 0;
> +}
> +
>  static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  			       struct pt_regs *regs)
>  {
> -	int i, access;
> -	u32 val, ctrl_reg, alignment_mask;
> +	int i, access, closest_match = 0;
> +	u32 min_dist = -1, dist;
> +	u32 val, ctrl_reg;
>  	struct perf_event *wp, **slots;
>  	struct arch_hw_breakpoint *info;
>  	struct arch_hw_breakpoint_ctrl ctrl;
>  
>  	slots = this_cpu_ptr(wp_on_reg);
>  
> +	/*
> +	 * Find all watchpoints that match the reported address. If no exact
> +	 * match is found. Attribute the hit to the closest watchpoint.
> +	 */
> +	rcu_read_lock();
>  	for (i = 0; i < core_num_wrps; ++i) {
> -		rcu_read_lock();
> -
>  		wp = slots[i];
> -
>  		if (wp == NULL)
> -			goto unlock;
> +			continue;
>  
> -		info = counter_arch_bp(wp);
>  		/*
>  		 * The DFAR is an unknown value on debug architectures prior
>  		 * to 7.1. Since we only allow a single watchpoint on these
> @@ -708,33 +744,31 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  		 */
>  		if (debug_arch < ARM_DEBUG_ARCH_V7_1) {
>  			BUG_ON(i > 0);
> +			info = counter_arch_bp(wp);
>  			info->trigger = wp->attr.bp_addr;
>  		} else {
> -			if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
> -				alignment_mask = 0x7;
> -			else
> -				alignment_mask = 0x3;
> -
> -			/* Check if the watchpoint value matches. */
> -			val = read_wb_reg(ARM_BASE_WVR + i);
> -			if (val != (addr & ~alignment_mask))
> -				goto unlock;
> -
> -			/* Possible match, check the byte address select. */
> -			ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> -			decode_ctrl_reg(ctrl_reg, &ctrl);
> -			if (!((1 << (addr & alignment_mask)) & ctrl.len))
> -				goto unlock;
> -
>  			/* Check that the access type matches. */
>  			if (debug_exception_updates_fsr()) {
>  				access = (fsr & ARM_FSR_ACCESS_MASK) ?
>  					  HW_BREAKPOINT_W : HW_BREAKPOINT_R;
>  				if (!(access & hw_breakpoint_type(wp)))
> -					goto unlock;
> +					continue;
>  			}
>  
> +			val = read_wb_reg(ARM_BASE_WVR + i);
> +			ctrl_reg = read_wb_reg(ARM_BASE_WCR + i);
> +			decode_ctrl_reg(ctrl_reg, &ctrl);
> +			dist = get_distance_from_watchpoint(addr, val, &ctrl);
> +			if (dist < min_dist) {
> +				min_dist = dist;
> +				closest_match = i;
> +			}
> +			/* Is this an exact match? */
> +			if (dist != 0)
> +				continue;
> +
>  			/* We have a winner. */
> +			info = counter_arch_bp(wp);
>  			info->trigger = addr;

Unless we care about using the 'last' watchpoint in case multiple WPs have
the same address I think it would be clearer to change the above to:

	       	       	if (dist == 0) {
				/* We have a winner. */
				info = counter_arch_bp(wp);
				info->trigger = addr;
				break;
			}

>  		}
>  
> @@ -748,10 +782,20 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  		 */
>  		if (is_default_overflow_handler(wp))
>  			enable_single_step(wp, instruction_pointer(regs));
> +	}
>  
> -unlock:
> -		rcu_read_unlock();
> +	if (min_dist > 0 && min_dist != -1) {
> +		/* No exact match found. */
> +		wp = slots[closest_match];
> +		info = counter_arch_bp(wp);
> +		info->trigger = addr;
> +		pr_debug("watchpoint fired: address = 0x%x\n", info->trigger);
> +		perf_bp_event(wp, regs);
> +		if (is_default_overflow_handler(wp))
> +			enable_single_step(wp, instruction_pointer(regs));
>  	}
> +
> +	rcu_read_unlock();
>  }
>  
>  static void watchpoint_single_step_handler(unsigned long pc)
> -- 
> 2.23.0.866.gb869b98d4c-goog
>