[PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint

[PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint

[Catalin Udma]

When setting hw breakpoints/watchpoints, GDB reports the error
"Unexpected error setting hardware debug registers". The problem is
reproducible on A53/A57 models where the supported number of
breakpoints/watchpoints (6 or 4 read from ID_AA64DFR0_EL1) it is
less than the maximum number of debug registers from user_hwdebug_state

This patch fixes the problem by restricting the registers access to the
maximum number of supported breakpoints/watchpoints

Signed-off-by: Catalin Udma <catalin.udma@freescale.com>
---
 arch/arm64/kernel/ptrace.c |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index c484d56..dea4b28 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -241,21 +241,30 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 	return 0;
 }
 
-static int ptrace_hbp_get_resource_info(unsigned int note_type, u32 *info)
+static int ptrace_hbp_get_breakpoint_slots(unsigned int note_type, u8 *num)
 {
-	u8 num;
-	u32 reg = 0;
-
 	switch (note_type) {
 	case NT_ARM_HW_BREAK:
-		num = hw_breakpoint_slots(TYPE_INST);
+		*num = hw_breakpoint_slots(TYPE_INST);
 		break;
 	case NT_ARM_HW_WATCH:
-		num = hw_breakpoint_slots(TYPE_DATA);
+		*num = hw_breakpoint_slots(TYPE_DATA);
 		break;
 	default:
 		return -EINVAL;
 	}
+	return 0;
+}
+
+static int ptrace_hbp_get_resource_info(unsigned int note_type, u32 *info)
+{
+	u8 num;
+	u32 reg = 0;
+	int ret;
+
+	ret = ptrace_hbp_get_breakpoint_slots(note_type, &num);
+	if (ret)
+		return ret;
 
 	reg |= debug_monitors_arch();
 	reg <<= 8;
@@ -425,6 +434,7 @@ static int hw_break_set(struct task_struct *target,
 	int ret, idx = 0, offset, limit;
 	u32 ctrl;
 	u64 addr;
+	u8 num_slots;
 
 	/* Resource info and pad */
 	offset = offsetof(struct user_hwdebug_state, dbg_regs);
@@ -432,9 +442,13 @@ static int hw_break_set(struct task_struct *target,
 	if (ret)
 		return ret;
 
+	ret = ptrace_hbp_get_breakpoint_slots(note_type, &num_slots);
+	if (ret)
+		return ret;
+
 	/* (address, ctrl) registers */
 	limit = regset->n * regset->size;
-	while (count && offset < limit) {
+	while (count && offset < limit && idx < num_slots) {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
 					 offset, offset + PTRACE_HBP_ADDR_SZ);
 		if (ret)
-- 
1.7.8

[PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint

[Will Deacon]

On Mon, May 05, 2014 at 12:51:25PM +0100, Catalin Udma wrote:
> When setting hw breakpoints/watchpoints, GDB reports the error
> "Unexpected error setting hardware debug registers". The problem is
> reproducible on A53/A57 models where the supported number of
> breakpoints/watchpoints (6 or 4 read from ID_AA64DFR0_EL1) it is
> less than the maximum number of debug registers from user_hwdebug_state
> 
> This patch fixes the problem by restricting the registers access to the
> maximum number of supported breakpoints/watchpoints

Actually, I consider this to be a bug in GDB. It shouldn't be trying to
access registers that don't exist (we tell it the number of registers
available as part of the 32-bit info register).

I also think this has been fixed in GDB upstream:


  commit f45c82da381e0ce5ce51b7fb24d0d28611d266b8
  Author: Yufeng Zhang <yufeng.zhang@arm.com>
  Date:   Wed Dec 18 16:47:33 2013 +0000

      gdb/

          * aarch64-linux-nat.c (aarch64_linux_set_debug_regs): Set
          iov.iov_len with the real length in use.

      gdb/gdbserver/

          * linux-aarch64-low.c (aarch64_linux_set_debug_regs): Set
          iov.iov_len with the real length in use.


so there's no need to change the kernel for this.

Will

[PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint

[catalin.udma at freescale.com]

Thank you, Will. The gdb patch seems to solve the problem.

As long as the kernel can rely on the registers number sent from GDB,
then there is no need for this kernel patch.

Regards,
Catalin


> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Friday, May 16, 2014 4:44 PM
> To: Udma Catalin-Dan-B32721
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint
> 
> On Mon, May 05, 2014 at 12:51:25PM +0100, Catalin Udma wrote:
> > When setting hw breakpoints/watchpoints, GDB reports the error
> > "Unexpected error setting hardware debug registers". The problem is
> > reproducible on A53/A57 models where the supported number of
> > breakpoints/watchpoints (6 or 4 read from ID_AA64DFR0_EL1) it is
> > less than the maximum number of debug registers from user_hwdebug_state
> >
> > This patch fixes the problem by restricting the registers access to the
> > maximum number of supported breakpoints/watchpoints
> 
> Actually, I consider this to be a bug in GDB. It shouldn't be trying to
> access registers that don't exist (we tell it the number of registers
> available as part of the 32-bit info register).
> 
> I also think this has been fixed in GDB upstream:
> 
> 
>   commit f45c82da381e0ce5ce51b7fb24d0d28611d266b8
>   Author: Yufeng Zhang <yufeng.zhang@arm.com>
>   Date:   Wed Dec 18 16:47:33 2013 +0000
> 
>       gdb/
> 
>           * aarch64-linux-nat.c (aarch64_linux_set_debug_regs): Set
>           iov.iov_len with the real length in use.
> 
>       gdb/gdbserver/
> 
>           * linux-aarch64-low.c (aarch64_linux_set_debug_regs): Set
>           iov.iov_len with the real length in use.
> 
> 
> so there's no need to change the kernel for this.
> 
> Will

[PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint

[Will Deacon]

On Fri, May 16, 2014 at 03:35:45PM +0100, catalin.udma at freescale.com wrote:
> Thank you, Will. The gdb patch seems to solve the problem.
> 
> As long as the kernel can rely on the registers number sent from GDB,
> then there is no need for this kernel patch.

What do you mean? Userspace can pass whatever it likes and the regset
accessors will do the right thing.

Will

[PATCH] arm64: ptrace: fix setting hw breakpoint/watchpoint

[catalin.udma at freescale.com]

> > As long as the kernel can rely on the registers number sent from GDB,
> > then there is no need for this kernel patch.
> 
> What do you mean? Userspace can pass whatever it likes and the regset
> accessors will do the right thing.
> 
> Will

I meant I agree with you that there is no need to change the kernel with this patch. 
Thank you,
Catalin