Messages in this thread Patch in this message | | | Date | Mon, 21 Oct 2019 11:46:58 -0700 | From | Matthias Kaehlcke <> | Subject | Re: [PATCH] ARM: hw_breakpoint: Handle inexact watchpoint addresses |
| |
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@chromium.org> > --- > > 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 >
| |