Patch #1 fixes issue for quardword instruction on p10 predecessors.
Patch #2 fixes issue for vector instructions.
Patch #3 fixes a bug about watchpoint not firing when created with
ptrace PPC_PTRACE_SETHWDEBUG and CONFIG_HAVE_HW_BREAKPOINT=N.
The fix uses HW_BRK_TYPE_PRIV_ALL for ptrace user which, I
guess, should be fine because we don't leak any kernel
addresses and PRIV_ALL will also help to cover scenarios when
kernel accesses user memory.
Patch #4,#5 fixes infinite exception bug, again the bug happens only
with CONFIG_HAVE_HW_BREAKPOINT=N.
Patch #6 fixes two places where we are missing to set hw_len.
Patch #7 introduce new feature bit PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
which will be set when running on ISA 3.1 compliant machine.
Patch #8 finally adds selftest to test scenarios fixed by patch#2,#3
and also moves MODE_EXACT tests outside of BP_RANGE condition.
Christophe, let me know if this series breaks something for 8xx.
v5: https://lore.kernel.org/r/[email protected]
v5->v6:
- Fix build faulure reported by kernel test robot
- patch #5. Use more compact if condition, suggested by Christophe
Ravi Bangoria (8):
powerpc/watchpoint: Fix quarword instruction handling on p10
predecessors
powerpc/watchpoint: Fix handling of vector instructions
powerpc/watchpoint/ptrace: Fix SETHWDEBUG when
CONFIG_HAVE_HW_BREAKPOINT=N
powerpc/watchpoint: Move DAWR detection logic outside of
hw_breakpoint.c
powerpc/watchpoint: Fix exception handling for
CONFIG_HAVE_HW_BREAKPOINT=N
powerpc/watchpoint: Add hw_len wherever missing
powerpc/watchpoint/ptrace: Introduce PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
powerpc/watchpoint/selftests: Tests for kernel accessing user memory
Documentation/powerpc/ptrace.rst | 1 +
arch/powerpc/include/asm/hw_breakpoint.h | 12 ++
arch/powerpc/include/uapi/asm/ptrace.h | 1 +
arch/powerpc/kernel/Makefile | 3 +-
arch/powerpc/kernel/hw_breakpoint.c | 149 +---------------
.../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
arch/powerpc/kernel/process.c | 48 ++++++
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 9 +-
arch/powerpc/xmon/xmon.c | 1 +
.../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 +++++-
10 files changed, 282 insertions(+), 152 deletions(-)
create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
--
2.26.2
Vector load/store instructions are special because they are always
aligned. Thus unaligned EA needs to be aligned down before comparing
it with watch ranges. Otherwise we might consider valid event as
invalid.
Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/kernel/hw_breakpoint.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 9f7df1c37233..f6b24838ca3c 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -644,6 +644,8 @@ static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
if (*type == CACHEOP) {
*size = cache_op_size();
*ea &= ~(*size - 1);
+ } else if (*type == LOAD_VMX || *type == STORE_VMX) {
+ *ea &= ~(*size - 1);
}
}
--
2.26.2
Power10 hw has multiple DAWRs but hw doesn't tell which DAWR caused
the exception. So we have a sw logic to detect that in hw_breakpoint.c.
But hw_breakpoint.c gets compiled only with CONFIG_HAVE_HW_BREAKPOINT=Y.
Move DAWR detection logic outside of hw_breakpoint.c so that it can be
reused when CONFIG_HAVE_HW_BREAKPOINT is not set.
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/include/asm/hw_breakpoint.h | 8 +
arch/powerpc/kernel/Makefile | 3 +-
arch/powerpc/kernel/hw_breakpoint.c | 159 +----------------
.../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
4 files changed, 174 insertions(+), 158 deletions(-)
create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 9b68eafebf43..81872c420476 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -10,6 +10,7 @@
#define _PPC_BOOK3S_64_HW_BREAKPOINT_H
#include <asm/cpu_has_feature.h>
+#include <asm/inst.h>
#ifdef __KERNEL__
struct arch_hw_breakpoint {
@@ -52,6 +53,13 @@ static inline int nr_wp_slots(void)
return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1;
}
+bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
+ unsigned long ea, int type, int size,
+ struct arch_hw_breakpoint *info);
+
+void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
+ int *type, int *size, unsigned long *ea);
+
#ifdef CONFIG_HAVE_HW_BREAKPOINT
#include <linux/kdebug.h>
#include <asm/reg.h>
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index cbf41fb4ee89..a5550c2b24c4 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -45,7 +45,8 @@ obj-y := cputable.o syscalls.o \
signal.o sysfs.o cacheinfo.o time.o \
prom.o traps.o setup-common.o \
udbg.o misc.o io.o misc_$(BITS).o \
- of_platform.o prom_parse.o firmware.o
+ of_platform.o prom_parse.o firmware.o \
+ hw_breakpoint_constraints.o
obj-y += ptrace/
obj-$(CONFIG_PPC64) += setup_64.o \
paca.o nvram_64.o note.o syscall_64.o
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index f6b24838ca3c..f4e8f21046f5 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -494,161 +494,6 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
}
}
-static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
-{
- return ((info->address <= dar) && (dar - info->address < info->len));
-}
-
-static bool ea_user_range_overlaps(unsigned long ea, int size,
- struct arch_hw_breakpoint *info)
-{
- return ((ea < info->address + info->len) &&
- (ea + size > info->address));
-}
-
-static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
-{
- unsigned long hw_start_addr, hw_end_addr;
-
- hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
- hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
-
- return ((hw_start_addr <= dar) && (hw_end_addr > dar));
-}
-
-static bool ea_hw_range_overlaps(unsigned long ea, int size,
- struct arch_hw_breakpoint *info)
-{
- unsigned long hw_start_addr, hw_end_addr;
- unsigned long align_size = HW_BREAKPOINT_SIZE;
-
- /*
- * On p10 predecessors, quadword is handle differently then
- * other instructions.
- */
- if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
- align_size = HW_BREAKPOINT_SIZE_QUADWORD;
-
- hw_start_addr = ALIGN_DOWN(info->address, align_size);
- hw_end_addr = ALIGN(info->address + info->len, align_size);
-
- return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
-}
-
-/*
- * If hw has multiple DAWR registers, we also need to check all
- * dawrx constraint bits to confirm this is _really_ a valid event.
- * If type is UNKNOWN, but privilege level matches, consider it as
- * a positive match.
- */
-static bool check_dawrx_constraints(struct pt_regs *regs, int type,
- struct arch_hw_breakpoint *info)
-{
- if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
- return false;
-
- /*
- * The Cache Management instructions other than dcbz never
- * cause a match. i.e. if type is CACHEOP, the instruction
- * is dcbz, and dcbz is treated as Store.
- */
- if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
- return false;
-
- if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
- return false;
-
- if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
- return false;
-
- return true;
-}
-
-/*
- * Return true if the event is valid wrt dawr configuration,
- * including extraneous exception. Otherwise return false.
- */
-static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
- unsigned long ea, int type, int size,
- struct arch_hw_breakpoint *info)
-{
- bool in_user_range = dar_in_user_range(regs->dar, info);
- bool dawrx_constraints;
-
- /*
- * 8xx supports only one breakpoint and thus we can
- * unconditionally return true.
- */
- if (IS_ENABLED(CONFIG_PPC_8xx)) {
- if (!in_user_range)
- info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
- return true;
- }
-
- if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
- if (cpu_has_feature(CPU_FTR_ARCH_31) &&
- !dar_in_hw_range(regs->dar, info))
- return false;
-
- return true;
- }
-
- dawrx_constraints = check_dawrx_constraints(regs, type, info);
-
- if (type == UNKNOWN) {
- if (cpu_has_feature(CPU_FTR_ARCH_31) &&
- !dar_in_hw_range(regs->dar, info))
- return false;
-
- return dawrx_constraints;
- }
-
- if (ea_user_range_overlaps(ea, size, info))
- return dawrx_constraints;
-
- if (ea_hw_range_overlaps(ea, size, info)) {
- if (dawrx_constraints) {
- info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
- return true;
- }
- }
- return false;
-}
-
-static int cache_op_size(void)
-{
-#ifdef __powerpc64__
- return ppc64_caches.l1d.block_size;
-#else
- return L1_CACHE_BYTES;
-#endif
-}
-
-static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
- int *type, int *size, unsigned long *ea)
-{
- struct instruction_op op;
-
- if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
- return;
-
- analyse_instr(&op, regs, *instr);
- *type = GETTYPE(op.type);
- *ea = op.ea;
-#ifdef __powerpc64__
- if (!(regs->msr & MSR_64BIT))
- *ea &= 0xffffffffUL;
-#endif
-
- *size = GETSIZE(op.type);
- if (*type == CACHEOP) {
- *size = cache_op_size();
- *ea &= ~(*size - 1);
- } else if (*type == LOAD_VMX || *type == STORE_VMX) {
- *ea &= ~(*size - 1);
- }
-}
-
static bool is_larx_stcx_instr(int type)
{
return type == LARX || type == STCX;
@@ -732,7 +577,7 @@ int hw_breakpoint_handler(struct die_args *args)
rcu_read_lock();
if (!IS_ENABLED(CONFIG_PPC_8xx))
- get_instr_detail(regs, &instr, &type, &size, &ea);
+ wp_get_instr_detail(regs, &instr, &type, &size, &ea);
for (i = 0; i < nr_wp_slots(); i++) {
bp[i] = __this_cpu_read(bp_per_reg[i]);
@@ -742,7 +587,7 @@ int hw_breakpoint_handler(struct die_args *args)
info[i] = counter_arch_bp(bp[i]);
info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
- if (check_constraints(regs, instr, ea, type, size, info[i])) {
+ if (wp_check_constraints(regs, instr, ea, type, size, info[i])) {
if (!IS_ENABLED(CONFIG_PPC_8xx) &&
ppc_inst_equal(instr, ppc_inst(0))) {
handler_error(bp[i], info[i]);
diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
new file mode 100644
index 000000000000..867ee4aa026a
--- /dev/null
+++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/kernel.h>
+#include <linux/uaccess.h>
+#include <linux/sched.h>
+#include <asm/hw_breakpoint.h>
+#include <asm/sstep.h>
+#include <asm/cache.h>
+
+static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
+{
+ return ((info->address <= dar) && (dar - info->address < info->len));
+}
+
+static bool ea_user_range_overlaps(unsigned long ea, int size,
+ struct arch_hw_breakpoint *info)
+{
+ return ((ea < info->address + info->len) &&
+ (ea + size > info->address));
+}
+
+static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
+{
+ unsigned long hw_start_addr, hw_end_addr;
+
+ hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
+ hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
+
+ return ((hw_start_addr <= dar) && (hw_end_addr > dar));
+}
+
+static bool ea_hw_range_overlaps(unsigned long ea, int size,
+ struct arch_hw_breakpoint *info)
+{
+ unsigned long hw_start_addr, hw_end_addr;
+ unsigned long align_size = HW_BREAKPOINT_SIZE;
+
+ /*
+ * On p10 predecessors, quadword is handle differently then
+ * other instructions.
+ */
+ if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
+ align_size = HW_BREAKPOINT_SIZE_QUADWORD;
+
+ hw_start_addr = ALIGN_DOWN(info->address, align_size);
+ hw_end_addr = ALIGN(info->address + info->len, align_size);
+
+ return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
+}
+
+/*
+ * If hw has multiple DAWR registers, we also need to check all
+ * dawrx constraint bits to confirm this is _really_ a valid event.
+ * If type is UNKNOWN, but privilege level matches, consider it as
+ * a positive match.
+ */
+static bool check_dawrx_constraints(struct pt_regs *regs, int type,
+ struct arch_hw_breakpoint *info)
+{
+ if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
+ return false;
+
+ /*
+ * The Cache Management instructions other than dcbz never
+ * cause a match. i.e. if type is CACHEOP, the instruction
+ * is dcbz, and dcbz is treated as Store.
+ */
+ if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
+ return false;
+
+ if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
+ return false;
+
+ if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
+ return false;
+
+ return true;
+}
+
+/*
+ * Return true if the event is valid wrt dawr configuration,
+ * including extraneous exception. Otherwise return false.
+ */
+bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
+ unsigned long ea, int type, int size,
+ struct arch_hw_breakpoint *info)
+{
+ bool in_user_range = dar_in_user_range(regs->dar, info);
+ bool dawrx_constraints;
+
+ /*
+ * 8xx supports only one breakpoint and thus we can
+ * unconditionally return true.
+ */
+ if (IS_ENABLED(CONFIG_PPC_8xx)) {
+ if (!in_user_range)
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
+
+ if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
+ if (cpu_has_feature(CPU_FTR_ARCH_31) &&
+ !dar_in_hw_range(regs->dar, info))
+ return false;
+
+ return true;
+ }
+
+ dawrx_constraints = check_dawrx_constraints(regs, type, info);
+
+ if (type == UNKNOWN) {
+ if (cpu_has_feature(CPU_FTR_ARCH_31) &&
+ !dar_in_hw_range(regs->dar, info))
+ return false;
+
+ return dawrx_constraints;
+ }
+
+ if (ea_user_range_overlaps(ea, size, info))
+ return dawrx_constraints;
+
+ if (ea_hw_range_overlaps(ea, size, info)) {
+ if (dawrx_constraints) {
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
+ }
+ return false;
+}
+
+static int cache_op_size(void)
+{
+#ifdef __powerpc64__
+ return ppc64_caches.l1d.block_size;
+#else
+ return L1_CACHE_BYTES;
+#endif
+}
+
+void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
+ int *type, int *size, unsigned long *ea)
+{
+ struct instruction_op op;
+
+ if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
+ return;
+
+ analyse_instr(&op, regs, *instr);
+ *type = GETTYPE(op.type);
+ *ea = op.ea;
+#ifdef __powerpc64__
+ if (!(regs->msr & MSR_64BIT))
+ *ea &= 0xffffffffUL;
+#endif
+
+ *size = GETSIZE(op.type);
+ if (*type == CACHEOP) {
+ *size = cache_op_size();
+ *ea &= ~(*size - 1);
+ } else if (*type == LOAD_VMX || *type == STORE_VMX) {
+ *ea &= ~(*size - 1);
+ }
+}
--
2.26.2
There are couple of places where we set len but not hw_len. For
ptrace/perf watchpoints, when CONFIG_HAVE_HW_BREAKPOINT=Y, hw_len
will be calculated and set internally while parsing watchpoint.
But when CONFIG_HAVE_HW_BREAKPOINT=N, we need to manually set
'hw_len'. Similarly for xmon as well, hw_len needs to be set
directly.
Fixes: b57aeab811db ("powerpc/watchpoint: Fix length calculation for unaligned target")
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 1 +
arch/powerpc/xmon/xmon.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index c9122ed91340..48c52426af80 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -219,6 +219,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
brk.type = HW_BRK_TYPE_TRANSLATE | HW_BRK_TYPE_PRIV_ALL;
brk.len = DABR_MAX_LEN;
+ brk.hw_len = DABR_MAX_LEN;
if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
brk.type |= HW_BRK_TYPE_READ;
if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index df7bca00f5ec..55c43a6c9111 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -969,6 +969,7 @@ static void insert_cpu_bpts(void)
brk.address = dabr[i].address;
brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
brk.len = 8;
+ brk.hw_len = 8;
__set_breakpoint(i, &brk);
}
}
--
2.26.2
Introduce tests to cover simple scenarios where user is watching
memory which can be accessed by kernel as well. We also support
_MODE_EXACT with _SETHWDEBUG interface. Move those testcases out-
side of _BP_RANGE condition. This will help to test _MODE_EXACT
scenarios when CONFIG_HAVE_HW_BREAKPOINT is not set, eg:
$ ./ptrace-hwbreak
...
PTRACE_SET_DEBUGREG, Kernel Access Userspace, len: 8: Ok
PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO, len: 1: Ok
PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RO, len: 1: Ok
PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RW, len: 1: Ok
PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace, len: 1: Ok
success: ptrace-hwbreak
Suggested-by: Pedro Miraglia Franco de Carvalho <[email protected]>
Signed-off-by: Ravi Bangoria <[email protected]>
---
.../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 ++++++++++++++++++-
1 file changed, 46 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
index fc477dfe86a2..2e0d86e0687e 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
@@ -20,6 +20,8 @@
#include <signal.h>
#include <sys/types.h>
#include <sys/wait.h>
+#include <sys/syscall.h>
+#include <linux/limits.h>
#include "ptrace.h"
#define SPRN_PVR 0x11F
@@ -44,6 +46,7 @@ struct gstruct {
};
static volatile struct gstruct gstruct __attribute__((aligned(512)));
+static volatile char cwd[PATH_MAX] __attribute__((aligned(8)));
static void get_dbginfo(pid_t child_pid, struct ppc_debug_info *dbginfo)
{
@@ -138,6 +141,9 @@ static void test_workload(void)
write_var(len);
}
+ /* PTRACE_SET_DEBUGREG, Kernel Access Userspace test */
+ syscall(__NR_getcwd, &cwd, PATH_MAX);
+
/* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO test */
write_var(1);
@@ -150,6 +156,9 @@ static void test_workload(void)
else
read_var(1);
+ /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace test */
+ syscall(__NR_getcwd, &cwd, PATH_MAX);
+
/* PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW ALIGNED, WO test */
gstruct.a[rand() % A_LEN] = 'a';
@@ -293,6 +302,24 @@ static int test_set_debugreg(pid_t child_pid)
return 0;
}
+static int test_set_debugreg_kernel_userspace(pid_t child_pid)
+{
+ unsigned long wp_addr = (unsigned long)cwd;
+ char *name = "PTRACE_SET_DEBUGREG";
+
+ /* PTRACE_SET_DEBUGREG, Kernel Access Userspace test */
+ wp_addr &= ~0x7UL;
+ wp_addr |= (1Ul << DABR_READ_SHIFT);
+ wp_addr |= (1UL << DABR_WRITE_SHIFT);
+ wp_addr |= (1UL << DABR_TRANSLATION_SHIFT);
+ ptrace_set_debugreg(child_pid, wp_addr);
+ ptrace(PTRACE_CONT, child_pid, NULL, 0);
+ check_success(child_pid, name, "Kernel Access Userspace", wp_addr, 8);
+
+ ptrace_set_debugreg(child_pid, 0);
+ return 0;
+}
+
static void get_ppc_hw_breakpoint(struct ppc_hw_breakpoint *info, int type,
unsigned long addr, int len)
{
@@ -338,6 +365,22 @@ static void test_sethwdebug_exact(pid_t child_pid)
ptrace_delhwdebug(child_pid, wh);
}
+static void test_sethwdebug_exact_kernel_userspace(pid_t child_pid)
+{
+ struct ppc_hw_breakpoint info;
+ unsigned long wp_addr = (unsigned long)&cwd;
+ char *name = "PPC_PTRACE_SETHWDEBUG, MODE_EXACT";
+ int len = 1; /* hardcoded in kernel */
+ int wh;
+
+ /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace test */
+ get_ppc_hw_breakpoint(&info, PPC_BREAKPOINT_TRIGGER_WRITE, wp_addr, 0);
+ wh = ptrace_sethwdebug(child_pid, &info);
+ ptrace(PTRACE_CONT, child_pid, NULL, 0);
+ check_success(child_pid, name, "Kernel Access Userspace", wp_addr, len);
+ ptrace_delhwdebug(child_pid, wh);
+}
+
static void test_sethwdebug_range_aligned(pid_t child_pid)
{
struct ppc_hw_breakpoint info;
@@ -452,9 +495,10 @@ static void
run_tests(pid_t child_pid, struct ppc_debug_info *dbginfo, bool dawr)
{
test_set_debugreg(child_pid);
+ test_set_debugreg_kernel_userspace(child_pid);
+ test_sethwdebug_exact(child_pid);
+ test_sethwdebug_exact_kernel_userspace(child_pid);
if (dbginfo->features & PPC_DEBUG_FEATURE_DATA_BP_RANGE) {
- test_sethwdebug_exact(child_pid);
-
test_sethwdebug_range_aligned(child_pid);
if (dawr || is_8xx) {
test_sethwdebug_range_unaligned(child_pid);
--
2.26.2
On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel
disables event every time it fires and user has to re-enable it.
Also, in case of ptrace watchpoint, kernel notifies ptrace user
before executing instruction.
With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable
ptrace event and thus it's causing infinite loop of exceptions.
This is especially harmful when user watches on a data which is
also read/written by kernel, eg syscall parameters. In such case,
infinite exceptions happens in kernel mode which causes soft-lockup.
Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers")
Reported-by: Pedro Miraglia Franco de Carvalho <[email protected]>
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/include/asm/hw_breakpoint.h | 3 ++
arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 4 +-
3 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 81872c420476..abebfbee5b1c 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -18,6 +18,7 @@ struct arch_hw_breakpoint {
u16 type;
u16 len; /* length of the target data symbol */
u16 hw_len; /* length programmed in hw */
+ u8 flags;
};
/* Note: Don't change the first 6 bits below as they are in the same order
@@ -37,6 +38,8 @@ struct arch_hw_breakpoint {
#define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
HW_BRK_TYPE_HYP)
+#define HW_BRK_FLAG_DISABLED 0x1
+
/* Minimum granularity */
#ifdef CONFIG_PPC_8xx
#define HW_BREAKPOINT_SIZE 0x4
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 016bd831908e..160fbbf41d40 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address,
(void __user *)address);
}
#else /* !CONFIG_PPC_ADV_DEBUG_REGS */
+
+static void do_break_handler(struct pt_regs *regs)
+{
+ struct arch_hw_breakpoint null_brk = {0};
+ struct arch_hw_breakpoint *info;
+ struct ppc_inst instr = ppc_inst(0);
+ int type = 0;
+ int size = 0;
+ unsigned long ea;
+ int i;
+
+ /*
+ * If underneath hw supports only one watchpoint, we know it
+ * caused exception. 8xx also falls into this category.
+ */
+ if (nr_wp_slots() == 1) {
+ __set_breakpoint(0, &null_brk);
+ current->thread.hw_brk[0] = null_brk;
+ current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED;
+ return;
+ }
+
+ /* Otherwise findout which DAWR caused exception and disable it. */
+ wp_get_instr_detail(regs, &instr, &type, &size, &ea);
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ info = ¤t->thread.hw_brk[i];
+ if (!info->address)
+ continue;
+
+ if (wp_check_constraints(regs, instr, ea, type, size, info)) {
+ __set_breakpoint(i, &null_brk);
+ current->thread.hw_brk[i] = null_brk;
+ current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED;
+ }
+ }
+}
+
void do_break (struct pt_regs *regs, unsigned long address,
unsigned long error_code)
{
@@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address,
if (debugger_break_match(regs))
return;
+ /*
+ * We reach here only when watchpoint exception is generated by ptrace
+ * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set,
+ * watchpoint is already handled by hw_breakpoint_handler() so we don't
+ * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set,
+ * we need to manually handle the watchpoint here.
+ */
+ if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
+ do_break_handler(regs);
+
/* Deliver the signal to userspace */
force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 57a0ab822334..c9122ed91340 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -286,11 +286,13 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
}
return ret;
#else /* CONFIG_HAVE_HW_BREAKPOINT */
- if (child->thread.hw_brk[data - 1].address == 0)
+ if (!(child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) &&
+ child->thread.hw_brk[data - 1].address == 0)
return -ENOENT;
child->thread.hw_brk[data - 1].address = 0;
child->thread.hw_brk[data - 1].type = 0;
+ child->thread.hw_brk[data - 1].flags = 0;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
return 0;
--
2.26.2
PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 can be used to determine whether
we are running on an ISA 3.1 compliant machine. Which is needed to
determine DAR behaviour, 512 byte boundary limit etc. This was
requested by Pedro Miraglia Franco de Carvalho for extending
watchpoint features in gdb. Note that availability of 2nd DAWR is
independent of this flag and should be checked using
ppc_debug_info->num_data_bps.
Signed-off-by: Ravi Bangoria <[email protected]>
---
Documentation/powerpc/ptrace.rst | 1 +
arch/powerpc/include/uapi/asm/ptrace.h | 1 +
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 ++
3 files changed, 4 insertions(+)
diff --git a/Documentation/powerpc/ptrace.rst b/Documentation/powerpc/ptrace.rst
index 864d4b6dddd1..77725d69eb4a 100644
--- a/Documentation/powerpc/ptrace.rst
+++ b/Documentation/powerpc/ptrace.rst
@@ -46,6 +46,7 @@ features will have bits indicating whether there is support for::
#define PPC_DEBUG_FEATURE_DATA_BP_RANGE 0x4
#define PPC_DEBUG_FEATURE_DATA_BP_MASK 0x8
#define PPC_DEBUG_FEATURE_DATA_BP_DAWR 0x10
+ #define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x20
2. PTRACE_SETHWDEBUG
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
index f5f1ccc740fc..7004cfea3f5f 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -222,6 +222,7 @@ struct ppc_debug_info {
#define PPC_DEBUG_FEATURE_DATA_BP_RANGE 0x0000000000000004
#define PPC_DEBUG_FEATURE_DATA_BP_MASK 0x0000000000000008
#define PPC_DEBUG_FEATURE_DATA_BP_DAWR 0x0000000000000010
+#define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x0000000000000020
#ifndef __ASSEMBLY__
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 48c52426af80..aa36fcad36cd 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -57,6 +57,8 @@ void ppc_gethwdinfo(struct ppc_debug_info *dbginfo)
} else {
dbginfo->features = 0;
}
+ if (cpu_has_feature(CPU_FTR_ARCH_31))
+ dbginfo->features |= PPC_DEBUG_FEATURE_DATA_BP_ARCH_31;
}
int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
--
2.26.2
On p10 predecessors, watchpoint with quarword access is compared at
quardword length. If the watch range is doubleword or less than that
in a first half of quarword aligned 16 bytes, and if there is any
unaligned quadword access which will access only the 2nd half, the
handler should consider it as extraneous and emulate/single-step it
before continuing.
Reported-by: Pedro Miraglia Franco de Carvalho <[email protected]>
Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/include/asm/hw_breakpoint.h | 1 +
arch/powerpc/kernel/hw_breakpoint.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index db206a7f38e2..9b68eafebf43 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -42,6 +42,7 @@ struct arch_hw_breakpoint {
#else
#define HW_BREAKPOINT_SIZE 0x8
#endif
+#define HW_BREAKPOINT_SIZE_QUADWORD 0x10
#define DABR_MAX_LEN 8
#define DAWR_MAX_LEN 512
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 1f4a1efa0074..9f7df1c37233 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -520,9 +520,17 @@ static bool ea_hw_range_overlaps(unsigned long ea, int size,
struct arch_hw_breakpoint *info)
{
unsigned long hw_start_addr, hw_end_addr;
+ unsigned long align_size = HW_BREAKPOINT_SIZE;
- hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
- hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
+ /*
+ * On p10 predecessors, quadword is handle differently then
+ * other instructions.
+ */
+ if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
+ align_size = HW_BREAKPOINT_SIZE_QUADWORD;
+
+ hw_start_addr = ALIGN_DOWN(info->address, align_size);
+ hw_end_addr = ALIGN(info->address + info->len, align_size);
return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
}
--
2.26.2
When kernel is compiled with CONFIG_HAVE_HW_BREAKPOINT=N, user can
still create watchpoint using PPC_PTRACE_SETHWDEBUG, with limited
functionalities. But, such watchpoints are never firing because of
the missing privilege settings. Fix that.
It's safe to set HW_BRK_TYPE_PRIV_ALL because we don't really leak
any kernel address in signal info. Setting HW_BRK_TYPE_PRIV_ALL will
also help to find scenarios when kernel accesses user memory.
Reported-by: Pedro Miraglia Franco de Carvalho <[email protected]>
Suggested-by: Pedro Miraglia Franco de Carvalho <[email protected]>
Signed-off-by: Ravi Bangoria <[email protected]>
---
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 697c7e4b5877..57a0ab822334 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -217,7 +217,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
return -EIO;
brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
- brk.type = HW_BRK_TYPE_TRANSLATE;
+ brk.type = HW_BRK_TYPE_TRANSLATE | HW_BRK_TYPE_PRIV_ALL;
brk.len = DABR_MAX_LEN;
if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
brk.type |= HW_BRK_TYPE_READ;
--
2.26.2
On Wed, 2 Sep 2020 09:59:37 +0530, Ravi Bangoria wrote:
> Patch #1 fixes issue for quardword instruction on p10 predecessors.
> Patch #2 fixes issue for vector instructions.
> Patch #3 fixes a bug about watchpoint not firing when created with
> ptrace PPC_PTRACE_SETHWDEBUG and CONFIG_HAVE_HW_BREAKPOINT=N.
> The fix uses HW_BRK_TYPE_PRIV_ALL for ptrace user which, I
> guess, should be fine because we don't leak any kernel
> addresses and PRIV_ALL will also help to cover scenarios when
> kernel accesses user memory.
> Patch #4,#5 fixes infinite exception bug, again the bug happens only
> with CONFIG_HAVE_HW_BREAKPOINT=N.
> Patch #6 fixes two places where we are missing to set hw_len.
> Patch #7 introduce new feature bit PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
> which will be set when running on ISA 3.1 compliant machine.
> Patch #8 finally adds selftest to test scenarios fixed by patch#2,#3
> and also moves MODE_EXACT tests outside of BP_RANGE condition.
>
> [...]
Applied to powerpc/next.
[1/8] powerpc/watchpoint: Fix quadword instruction handling on p10 predecessors
https://git.kernel.org/powerpc/c/4759c11ed20454b7b36db4ec15f7d5aa1519af4a
[2/8] powerpc/watchpoint: Fix handling of vector instructions
https://git.kernel.org/powerpc/c/4441eb02333a9b46a0d919aa7a6d3b137b5f2562
[3/8] powerpc/watchpoint/ptrace: Fix SETHWDEBUG when CONFIG_HAVE_HW_BREAKPOINT=N
https://git.kernel.org/powerpc/c/9b6b7c680cc20971444d9f836e49fc98848bcd0a
[4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
https://git.kernel.org/powerpc/c/edc8dd99b29e4d705c45e2a3a6c01b096ee056db
[5/8] powerpc/watchpoint: Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N
https://git.kernel.org/powerpc/c/5b905d77987de065bdd3a2906816b5f143df087b
[6/8] powerpc/watchpoint: Add hw_len wherever missing
https://git.kernel.org/powerpc/c/58da5984d2ea6d95f3f9d9e8dd9f7e1b0dddfb3c
[7/8] powerpc/watchpoint/ptrace: Introduce PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
https://git.kernel.org/powerpc/c/fa725cc53d353110f39a9e5b9f60d6acb2c7ff49
[8/8] selftests/powerpc: Tests for kernel accessing user memory
https://git.kernel.org/powerpc/c/ac234524056da4e0c081f682da3ea25cdaab737a
cheers
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Introduce tests to cover simple scenarios where user is watching
> memory which can be accessed by kernel as well. We also support
> _MODE_EXACT with _SETHWDEBUG interface. Move those testcases out-
> side of _BP_RANGE condition. This will help to test _MODE_EXACT
> scenarios when CONFIG_HAVE_HW_BREAKPOINT is not set, eg:
>
> $ ./ptrace-hwbreak
> ...
> PTRACE_SET_DEBUGREG, Kernel Access Userspace, len: 8: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO, len: 1: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RO, len: 1: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RW, len: 1: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace, len: 1: Ok
> success: ptrace-hwbreak
>
> Suggested-by: Pedro Miraglia Franco de Carvalho <[email protected]>
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> .../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 ++++++++++++++++++-
> 1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> index fc477dfe86a2..2e0d86e0687e 100644
> --- a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> +++ b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> @@ -20,6 +20,8 @@
> #include <signal.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> +#include <sys/syscall.h>
> +#include <linux/limits.h>
> #include "ptrace.h"
>
> #define SPRN_PVR 0x11F
> @@ -44,6 +46,7 @@ struct gstruct {
> };
> static volatile struct gstruct gstruct __attribute__((aligned(512)));
>
> +static volatile char cwd[PATH_MAX] __attribute__((aligned(8)));
>
> static void get_dbginfo(pid_t child_pid, struct ppc_debug_info *dbginfo)
> {
> @@ -138,6 +141,9 @@ static void test_workload(void)
> write_var(len);
> }
>
> + /* PTRACE_SET_DEBUGREG, Kernel Access Userspace test */
> + syscall(__NR_getcwd, &cwd, PATH_MAX);
> +
> /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO test */
> write_var(1);
>
> @@ -150,6 +156,9 @@ static void test_workload(void)
> else
> read_var(1);
>
> + /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace test */
> + syscall(__NR_getcwd, &cwd, PATH_MAX);
> +
> /* PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW ALIGNED, WO test */
> gstruct.a[rand() % A_LEN] = 'a';
>
> @@ -293,6 +302,24 @@ static int test_set_debugreg(pid_t child_pid)
> return 0;
> }
>
> +static int test_set_debugreg_kernel_userspace(pid_t child_pid)
> +{
> + unsigned long wp_addr = (unsigned long)cwd;
> + char *name = "PTRACE_SET_DEBUGREG";
> +
> + /* PTRACE_SET_DEBUGREG, Kernel Access Userspace test */
> + wp_addr &= ~0x7UL;
> + wp_addr |= (1Ul << DABR_READ_SHIFT);
> + wp_addr |= (1UL << DABR_WRITE_SHIFT);
> + wp_addr |= (1UL << DABR_TRANSLATION_SHIFT);
> + ptrace_set_debugreg(child_pid, wp_addr);
> + ptrace(PTRACE_CONT, child_pid, NULL, 0);
> + check_success(child_pid, name, "Kernel Access Userspace", wp_addr, 8);
> +
> + ptrace_set_debugreg(child_pid, 0);
> + return 0;
> +}
> +
> static void get_ppc_hw_breakpoint(struct ppc_hw_breakpoint *info, int type,
> unsigned long addr, int len)
> {
> @@ -338,6 +365,22 @@ static void test_sethwdebug_exact(pid_t child_pid)
> ptrace_delhwdebug(child_pid, wh);
> }
>
> +static void test_sethwdebug_exact_kernel_userspace(pid_t child_pid)
> +{
> + struct ppc_hw_breakpoint info;
> + unsigned long wp_addr = (unsigned long)&cwd;
> + char *name = "PPC_PTRACE_SETHWDEBUG, MODE_EXACT";
> + int len = 1; /* hardcoded in kernel */
> + int wh;
> +
> + /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace test */
> + get_ppc_hw_breakpoint(&info, PPC_BREAKPOINT_TRIGGER_WRITE, wp_addr, 0);
> + wh = ptrace_sethwdebug(child_pid, &info);
> + ptrace(PTRACE_CONT, child_pid, NULL, 0);
> + check_success(child_pid, name, "Kernel Access Userspace", wp_addr, len);
> + ptrace_delhwdebug(child_pid, wh);
> +}
> +
> static void test_sethwdebug_range_aligned(pid_t child_pid)
> {
> struct ppc_hw_breakpoint info;
> @@ -452,9 +495,10 @@ static void
> run_tests(pid_t child_pid, struct ppc_debug_info *dbginfo, bool dawr)
> {
> test_set_debugreg(child_pid);
> + test_set_debugreg_kernel_userspace(child_pid);
> + test_sethwdebug_exact(child_pid);
> + test_sethwdebug_exact_kernel_userspace(child_pid);
> if (dbginfo->features & PPC_DEBUG_FEATURE_DATA_BP_RANGE) {
> - test_sethwdebug_exact(child_pid);
> -
> test_sethwdebug_range_aligned(child_pid);
> if (dawr || is_8xx) {
> test_sethwdebug_range_unaligned(child_pid);
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Power10 hw has multiple DAWRs but hw doesn't tell which DAWR caused
> the exception. So we have a sw logic to detect that in hw_breakpoint.c.
> But hw_breakpoint.c gets compiled only with CONFIG_HAVE_HW_BREAKPOINT=Y.
> Move DAWR detection logic outside of hw_breakpoint.c so that it can be
> reused when CONFIG_HAVE_HW_BREAKPOINT is not set.
>
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 8 +
> arch/powerpc/kernel/Makefile | 3 +-
> arch/powerpc/kernel/hw_breakpoint.c | 159 +----------------
> .../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
> 4 files changed, 174 insertions(+), 158 deletions(-)
> create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 9b68eafebf43..81872c420476 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -10,6 +10,7 @@
> #define _PPC_BOOK3S_64_HW_BREAKPOINT_H
>
> #include <asm/cpu_has_feature.h>
> +#include <asm/inst.h>
>
> #ifdef __KERNEL__
> struct arch_hw_breakpoint {
> @@ -52,6 +53,13 @@ static inline int nr_wp_slots(void)
> return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1;
> }
>
> +bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
> + unsigned long ea, int type, int size,
> + struct arch_hw_breakpoint *info);
> +
> +void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> + int *type, int *size, unsigned long *ea);
> +
> #ifdef CONFIG_HAVE_HW_BREAKPOINT
> #include <linux/kdebug.h>
> #include <asm/reg.h>
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index cbf41fb4ee89..a5550c2b24c4 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -45,7 +45,8 @@ obj-y := cputable.o syscalls.o \
> signal.o sysfs.o cacheinfo.o time.o \
> prom.o traps.o setup-common.o \
> udbg.o misc.o io.o misc_$(BITS).o \
> - of_platform.o prom_parse.o firmware.o
> + of_platform.o prom_parse.o firmware.o \
> + hw_breakpoint_constraints.o
> obj-y += ptrace/
> obj-$(CONFIG_PPC64) += setup_64.o \
> paca.o nvram_64.o note.o syscall_64.o
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index f6b24838ca3c..f4e8f21046f5 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -494,161 +494,6 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
> }
> }
>
> -static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
> -{
> - return ((info->address <= dar) && (dar - info->address < info->len));
> -}
> -
> -static bool ea_user_range_overlaps(unsigned long ea, int size,
> - struct arch_hw_breakpoint *info)
> -{
> - return ((ea < info->address + info->len) &&
> - (ea + size > info->address));
> -}
> -
> -static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
> -{
> - unsigned long hw_start_addr, hw_end_addr;
> -
> - hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
> - hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
> -
> - return ((hw_start_addr <= dar) && (hw_end_addr > dar));
> -}
> -
> -static bool ea_hw_range_overlaps(unsigned long ea, int size,
> - struct arch_hw_breakpoint *info)
> -{
> - unsigned long hw_start_addr, hw_end_addr;
> - unsigned long align_size = HW_BREAKPOINT_SIZE;
> -
> - /*
> - * On p10 predecessors, quadword is handle differently then
> - * other instructions.
> - */
> - if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> - align_size = HW_BREAKPOINT_SIZE_QUADWORD;
> -
> - hw_start_addr = ALIGN_DOWN(info->address, align_size);
> - hw_end_addr = ALIGN(info->address + info->len, align_size);
> -
> - return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
> -}
> -
> -/*
> - * If hw has multiple DAWR registers, we also need to check all
> - * dawrx constraint bits to confirm this is _really_ a valid event.
> - * If type is UNKNOWN, but privilege level matches, consider it as
> - * a positive match.
> - */
> -static bool check_dawrx_constraints(struct pt_regs *regs, int type,
> - struct arch_hw_breakpoint *info)
> -{
> - if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
> - return false;
> -
> - /*
> - * The Cache Management instructions other than dcbz never
> - * cause a match. i.e. if type is CACHEOP, the instruction
> - * is dcbz, and dcbz is treated as Store.
> - */
> - if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
> - return false;
> -
> - if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
> - return false;
> -
> - if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
> - return false;
> -
> - return true;
> -}
> -
> -/*
> - * Return true if the event is valid wrt dawr configuration,
> - * including extraneous exception. Otherwise return false.
> - */
> -static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
> - unsigned long ea, int type, int size,
> - struct arch_hw_breakpoint *info)
> -{
> - bool in_user_range = dar_in_user_range(regs->dar, info);
> - bool dawrx_constraints;
> -
> - /*
> - * 8xx supports only one breakpoint and thus we can
> - * unconditionally return true.
> - */
> - if (IS_ENABLED(CONFIG_PPC_8xx)) {
> - if (!in_user_range)
> - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> - return true;
> - }
> -
> - if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
> - if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> - !dar_in_hw_range(regs->dar, info))
> - return false;
> -
> - return true;
> - }
> -
> - dawrx_constraints = check_dawrx_constraints(regs, type, info);
> -
> - if (type == UNKNOWN) {
> - if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> - !dar_in_hw_range(regs->dar, info))
> - return false;
> -
> - return dawrx_constraints;
> - }
> -
> - if (ea_user_range_overlaps(ea, size, info))
> - return dawrx_constraints;
> -
> - if (ea_hw_range_overlaps(ea, size, info)) {
> - if (dawrx_constraints) {
> - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> - return true;
> - }
> - }
> - return false;
> -}
> -
> -static int cache_op_size(void)
> -{
> -#ifdef __powerpc64__
> - return ppc64_caches.l1d.block_size;
> -#else
> - return L1_CACHE_BYTES;
> -#endif
> -}
> -
> -static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> - int *type, int *size, unsigned long *ea)
> -{
> - struct instruction_op op;
> -
> - if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
> - return;
> -
> - analyse_instr(&op, regs, *instr);
> - *type = GETTYPE(op.type);
> - *ea = op.ea;
> -#ifdef __powerpc64__
> - if (!(regs->msr & MSR_64BIT))
> - *ea &= 0xffffffffUL;
> -#endif
> -
> - *size = GETSIZE(op.type);
> - if (*type == CACHEOP) {
> - *size = cache_op_size();
> - *ea &= ~(*size - 1);
> - } else if (*type == LOAD_VMX || *type == STORE_VMX) {
> - *ea &= ~(*size - 1);
> - }
> -}
> -
> static bool is_larx_stcx_instr(int type)
> {
> return type == LARX || type == STCX;
> @@ -732,7 +577,7 @@ int hw_breakpoint_handler(struct die_args *args)
> rcu_read_lock();
>
> if (!IS_ENABLED(CONFIG_PPC_8xx))
> - get_instr_detail(regs, &instr, &type, &size, &ea);
> + wp_get_instr_detail(regs, &instr, &type, &size, &ea);
>
> for (i = 0; i < nr_wp_slots(); i++) {
> bp[i] = __this_cpu_read(bp_per_reg[i]);
> @@ -742,7 +587,7 @@ int hw_breakpoint_handler(struct die_args *args)
> info[i] = counter_arch_bp(bp[i]);
> info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
>
> - if (check_constraints(regs, instr, ea, type, size, info[i])) {
> + if (wp_check_constraints(regs, instr, ea, type, size, info[i])) {
> if (!IS_ENABLED(CONFIG_PPC_8xx) &&
> ppc_inst_equal(instr, ppc_inst(0))) {
> handler_error(bp[i], info[i]);
> diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
> new file mode 100644
> index 000000000000..867ee4aa026a
> --- /dev/null
> +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/kernel.h>
> +#include <linux/uaccess.h>
> +#include <linux/sched.h>
> +#include <asm/hw_breakpoint.h>
> +#include <asm/sstep.h>
> +#include <asm/cache.h>
> +
> +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
> +{
> + return ((info->address <= dar) && (dar - info->address < info->len));
> +}
> +
> +static bool ea_user_range_overlaps(unsigned long ea, int size,
> + struct arch_hw_breakpoint *info)
> +{
> + return ((ea < info->address + info->len) &&
> + (ea + size > info->address));
> +}
> +
> +static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
> +{
> + unsigned long hw_start_addr, hw_end_addr;
> +
> + hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
> + hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
> +
> + return ((hw_start_addr <= dar) && (hw_end_addr > dar));
> +}
> +
> +static bool ea_hw_range_overlaps(unsigned long ea, int size,
> + struct arch_hw_breakpoint *info)
> +{
> + unsigned long hw_start_addr, hw_end_addr;
> + unsigned long align_size = HW_BREAKPOINT_SIZE;
> +
> + /*
> + * On p10 predecessors, quadword is handle differently then
> + * other instructions.
> + */
> + if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> + align_size = HW_BREAKPOINT_SIZE_QUADWORD;
> +
> + hw_start_addr = ALIGN_DOWN(info->address, align_size);
> + hw_end_addr = ALIGN(info->address + info->len, align_size);
> +
> + return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
> +}
> +
> +/*
> + * If hw has multiple DAWR registers, we also need to check all
> + * dawrx constraint bits to confirm this is _really_ a valid event.
> + * If type is UNKNOWN, but privilege level matches, consider it as
> + * a positive match.
> + */
> +static bool check_dawrx_constraints(struct pt_regs *regs, int type,
> + struct arch_hw_breakpoint *info)
> +{
> + if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
> + return false;
> +
> + /*
> + * The Cache Management instructions other than dcbz never
> + * cause a match. i.e. if type is CACHEOP, the instruction
> + * is dcbz, and dcbz is treated as Store.
> + */
> + if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
> + return false;
> +
> + if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
> + return false;
> +
> + if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
> + return false;
> +
> + return true;
> +}
> +
> +/*
> + * Return true if the event is valid wrt dawr configuration,
> + * including extraneous exception. Otherwise return false.
> + */
> +bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
> + unsigned long ea, int type, int size,
> + struct arch_hw_breakpoint *info)
> +{
> + bool in_user_range = dar_in_user_range(regs->dar, info);
> + bool dawrx_constraints;
> +
> + /*
> + * 8xx supports only one breakpoint and thus we can
> + * unconditionally return true.
> + */
> + if (IS_ENABLED(CONFIG_PPC_8xx)) {
> + if (!in_user_range)
> + info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> + return true;
> + }
> +
> + if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
> + if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> + !dar_in_hw_range(regs->dar, info))
> + return false;
> +
> + return true;
> + }
> +
> + dawrx_constraints = check_dawrx_constraints(regs, type, info);
> +
> + if (type == UNKNOWN) {
> + if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> + !dar_in_hw_range(regs->dar, info))
> + return false;
> +
> + return dawrx_constraints;
> + }
> +
> + if (ea_user_range_overlaps(ea, size, info))
> + return dawrx_constraints;
> +
> + if (ea_hw_range_overlaps(ea, size, info)) {
> + if (dawrx_constraints) {
> + info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static int cache_op_size(void)
> +{
> +#ifdef __powerpc64__
> + return ppc64_caches.l1d.block_size;
> +#else
> + return L1_CACHE_BYTES;
> +#endif
> +}
> +
> +void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> + int *type, int *size, unsigned long *ea)
> +{
> + struct instruction_op op;
> +
> + if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
> + return;
> +
> + analyse_instr(&op, regs, *instr);
> + *type = GETTYPE(op.type);
> + *ea = op.ea;
> +#ifdef __powerpc64__
> + if (!(regs->msr & MSR_64BIT))
> + *ea &= 0xffffffffUL;
> +#endif
> +
> + *size = GETSIZE(op.type);
> + if (*type == CACHEOP) {
> + *size = cache_op_size();
> + *ea &= ~(*size - 1);
> + } else if (*type == LOAD_VMX || *type == STORE_VMX) {
> + *ea &= ~(*size - 1);
> + }
> +}
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> There are couple of places where we set len but not hw_len. For
> ptrace/perf watchpoints, when CONFIG_HAVE_HW_BREAKPOINT=Y, hw_len
> will be calculated and set internally while parsing watchpoint.
> But when CONFIG_HAVE_HW_BREAKPOINT=N, we need to manually set
> 'hw_len'. Similarly for xmon as well, hw_len needs to be set
> directly.
>
> Fixes: b57aeab811db ("powerpc/watchpoint: Fix length calculation for unaligned target")
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 1 +
> arch/powerpc/xmon/xmon.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index c9122ed91340..48c52426af80 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -219,6 +219,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
> brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
> brk.type = HW_BRK_TYPE_TRANSLATE | HW_BRK_TYPE_PRIV_ALL;
> brk.len = DABR_MAX_LEN;
> + brk.hw_len = DABR_MAX_LEN;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> brk.type |= HW_BRK_TYPE_READ;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index df7bca00f5ec..55c43a6c9111 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -969,6 +969,7 @@ static void insert_cpu_bpts(void)
> brk.address = dabr[i].address;
> brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
> brk.len = 8;
> + brk.hw_len = 8;
> __set_breakpoint(i, &brk);
> }
> }
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 can be used to determine whether
> we are running on an ISA 3.1 compliant machine. Which is needed to
> determine DAR behaviour, 512 byte boundary limit etc. This was
> requested by Pedro Miraglia Franco de Carvalho for extending
> watchpoint features in gdb. Note that availability of 2nd DAWR is
> independent of this flag and should be checked using
> ppc_debug_info->num_data_bps.
>
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> Documentation/powerpc/ptrace.rst | 1 +
> arch/powerpc/include/uapi/asm/ptrace.h | 1 +
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 ++
> 3 files changed, 4 insertions(+)
>
> diff --git a/Documentation/powerpc/ptrace.rst b/Documentation/powerpc/ptrace.rst
> index 864d4b6dddd1..77725d69eb4a 100644
> --- a/Documentation/powerpc/ptrace.rst
> +++ b/Documentation/powerpc/ptrace.rst
> @@ -46,6 +46,7 @@ features will have bits indicating whether there is support for::
> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE 0x4
> #define PPC_DEBUG_FEATURE_DATA_BP_MASK 0x8
> #define PPC_DEBUG_FEATURE_DATA_BP_DAWR 0x10
> + #define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x20
>
> 2. PTRACE_SETHWDEBUG
>
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index f5f1ccc740fc..7004cfea3f5f 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -222,6 +222,7 @@ struct ppc_debug_info {
> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE 0x0000000000000004
> #define PPC_DEBUG_FEATURE_DATA_BP_MASK 0x0000000000000008
> #define PPC_DEBUG_FEATURE_DATA_BP_DAWR 0x0000000000000010
> +#define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x0000000000000020
>
> #ifndef __ASSEMBLY__
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 48c52426af80..aa36fcad36cd 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -57,6 +57,8 @@ void ppc_gethwdinfo(struct ppc_debug_info *dbginfo)
> } else {
> dbginfo->features = 0;
> }
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + dbginfo->features |= PPC_DEBUG_FEATURE_DATA_BP_ARCH_31;
> }
>
> int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Vector load/store instructions are special because they are always
> aligned. Thus unaligned EA needs to be aligned down before comparing
> it with watch ranges. Otherwise we might consider valid event as
> invalid.
>
> Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> arch/powerpc/kernel/hw_breakpoint.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 9f7df1c37233..f6b24838ca3c 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -644,6 +644,8 @@ static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> if (*type == CACHEOP) {
> *size = cache_op_size();
> *ea &= ~(*size - 1);
> + } else if (*type == LOAD_VMX || *type == STORE_VMX) {
> + *ea &= ~(*size - 1);
> }
> }
>
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Patch #1 fixes issue for quardword instruction on p10 predecessors.
> Patch #2 fixes issue for vector instructions.
> Patch #3 fixes a bug about watchpoint not firing when created with
> ptrace PPC_PTRACE_SETHWDEBUG and CONFIG_HAVE_HW_BREAKPOINT=N.
> The fix uses HW_BRK_TYPE_PRIV_ALL for ptrace user which, I
> guess, should be fine because we don't leak any kernel
> addresses and PRIV_ALL will also help to cover scenarios when
> kernel accesses user memory.
> Patch #4,#5 fixes infinite exception bug, again the bug happens only
> with CONFIG_HAVE_HW_BREAKPOINT=N.
> Patch #6 fixes two places where we are missing to set hw_len.
> Patch #7 introduce new feature bit PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
> which will be set when running on ISA 3.1 compliant machine.
> Patch #8 finally adds selftest to test scenarios fixed by patch#2,#3
> and also moves MODE_EXACT tests outside of BP_RANGE condition.
>
> Christophe, let me know if this series breaks something for 8xx.
>
> v5: https://lore.kernel.org/r/[email protected]
>
> v5->v6:
> - Fix build faulure reported by kernel test robot
> - patch #5. Use more compact if condition, suggested by Christophe
>
>
> Ravi Bangoria (8):
> powerpc/watchpoint: Fix quarword instruction handling on p10
> predecessors
> powerpc/watchpoint: Fix handling of vector instructions
> powerpc/watchpoint/ptrace: Fix SETHWDEBUG when
> CONFIG_HAVE_HW_BREAKPOINT=N
> powerpc/watchpoint: Move DAWR detection logic outside of
> hw_breakpoint.c
> powerpc/watchpoint: Fix exception handling for
> CONFIG_HAVE_HW_BREAKPOINT=N
> powerpc/watchpoint: Add hw_len wherever missing
> powerpc/watchpoint/ptrace: Introduce PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
> powerpc/watchpoint/selftests: Tests for kernel accessing user memory
>
> Documentation/powerpc/ptrace.rst | 1 +
> arch/powerpc/include/asm/hw_breakpoint.h | 12 ++
> arch/powerpc/include/uapi/asm/ptrace.h | 1 +
> arch/powerpc/kernel/Makefile | 3 +-
> arch/powerpc/kernel/hw_breakpoint.c | 149 +---------------
> .../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
> arch/powerpc/kernel/process.c | 48 ++++++
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 9 +-
> arch/powerpc/xmon/xmon.c | 1 +
> .../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 +++++-
> 10 files changed, 282 insertions(+), 152 deletions(-)
> create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
>
Tested this patch set for:
- SETHWDEBUG when CONFIG_HAVE_HW_BREAKPOINT=N = OK
- Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N = OK
- Check for PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 = OK
- Fix quarword instruction handling on p10 predecessors = OK
- Fix handling of vector instructions = OK
Also tested for:
- Set second watchpoint (P10 Mambo) = OK
- Infinity loop on sc instruction = OK
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel
> disables event every time it fires and user has to re-enable it.
> Also, in case of ptrace watchpoint, kernel notifies ptrace user
> before executing instruction.
>
> With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable
> ptrace event and thus it's causing infinite loop of exceptions.
> This is especially harmful when user watches on a data which is
> also read/written by kernel, eg syscall parameters. In such case,
> infinite exceptions happens in kernel mode which causes soft-lockup.
>
> Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers")
> Reported-by: Pedro Miraglia Franco de Carvalho <[email protected]>
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 3 ++
> arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 4 +-
> 3 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 81872c420476..abebfbee5b1c 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -18,6 +18,7 @@ struct arch_hw_breakpoint {
> u16 type;
> u16 len; /* length of the target data symbol */
> u16 hw_len; /* length programmed in hw */
> + u8 flags;
> };
>
> /* Note: Don't change the first 6 bits below as they are in the same order
> @@ -37,6 +38,8 @@ struct arch_hw_breakpoint {
> #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
> HW_BRK_TYPE_HYP)
>
> +#define HW_BRK_FLAG_DISABLED 0x1
> +
> /* Minimum granularity */
> #ifdef CONFIG_PPC_8xx
> #define HW_BREAKPOINT_SIZE 0x4
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 016bd831908e..160fbbf41d40 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address,
> (void __user *)address);
> }
> #else /* !CONFIG_PPC_ADV_DEBUG_REGS */
> +
> +static void do_break_handler(struct pt_regs *regs)
> +{
> + struct arch_hw_breakpoint null_brk = {0};
> + struct arch_hw_breakpoint *info;
> + struct ppc_inst instr = ppc_inst(0);
> + int type = 0;
> + int size = 0;
> + unsigned long ea;
> + int i;
> +
> + /*
> + * If underneath hw supports only one watchpoint, we know it
> + * caused exception. 8xx also falls into this category.
> + */
> + if (nr_wp_slots() == 1) {
> + __set_breakpoint(0, &null_brk);
> + current->thread.hw_brk[0] = null_brk;
> + current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED;
> + return;
> + }
> +
> + /* Otherwise findout which DAWR caused exception and disable it. */
> + wp_get_instr_detail(regs, &instr, &type, &size, &ea);
> +
> + for (i = 0; i < nr_wp_slots(); i++) {
> + info = ¤t->thread.hw_brk[i];
> + if (!info->address)
> + continue;
> +
> + if (wp_check_constraints(regs, instr, ea, type, size, info)) {
> + __set_breakpoint(i, &null_brk);
> + current->thread.hw_brk[i] = null_brk;
> + current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED;
> + }
> + }
> +}
> +
> void do_break (struct pt_regs *regs, unsigned long address,
> unsigned long error_code)
> {
> @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address,
> if (debugger_break_match(regs))
> return;
>
> + /*
> + * We reach here only when watchpoint exception is generated by ptrace
> + * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set,
> + * watchpoint is already handled by hw_breakpoint_handler() so we don't
> + * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set,
> + * we need to manually handle the watchpoint here.
> + */
> + if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
> + do_break_handler(regs);
> +
> /* Deliver the signal to userspace */
> force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 57a0ab822334..c9122ed91340 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -286,11 +286,13 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
> }
> return ret;
> #else /* CONFIG_HAVE_HW_BREAKPOINT */
> - if (child->thread.hw_brk[data - 1].address == 0)
> + if (!(child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) &&
> + child->thread.hw_brk[data - 1].address == 0)
> return -ENOENT;
>
> child->thread.hw_brk[data - 1].address = 0;
> child->thread.hw_brk[data - 1].type = 0;
> + child->thread.hw_brk[data - 1].flags = 0;
> #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>
> return 0;
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> When kernel is compiled with CONFIG_HAVE_HW_BREAKPOINT=N, user can
> still create watchpoint using PPC_PTRACE_SETHWDEBUG, with limited
> functionalities. But, such watchpoints are never firing because of
> the missing privilege settings. Fix that.
>
> It's safe to set HW_BRK_TYPE_PRIV_ALL because we don't really leak
> any kernel address in signal info. Setting HW_BRK_TYPE_PRIV_ALL will
> also help to find scenarios when kernel accesses user memory.
>
> Reported-by: Pedro Miraglia Franco de Carvalho <[email protected]>
> Suggested-by: Pedro Miraglia Franco de Carvalho <[email protected]>
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 697c7e4b5877..57a0ab822334 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -217,7 +217,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
> return -EIO;
>
> brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
> - brk.type = HW_BRK_TYPE_TRANSLATE;
> + brk.type = HW_BRK_TYPE_TRANSLATE | HW_BRK_TYPE_PRIV_ALL;
> brk.len = DABR_MAX_LEN;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> brk.type |= HW_BRK_TYPE_READ;
>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> On p10 predecessors, watchpoint with quarword access is compared at
> quardword length. If the watch range is doubleword or less than that
> in a first half of quarword aligned 16 bytes, and if there is any
> unaligned quadword access which will access only the 2nd half, the
> handler should consider it as extraneous and emulate/single-step it
> before continuing.
>
> Reported-by: Pedro Miraglia Franco de Carvalho <[email protected]>
> Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
> Signed-off-by: Ravi Bangoria <[email protected]>
Tested-by: Rogerio Alves <[email protected]>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 1 +
> arch/powerpc/kernel/hw_breakpoint.c | 12 ++++++++++--
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index db206a7f38e2..9b68eafebf43 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -42,6 +42,7 @@ struct arch_hw_breakpoint {
> #else
> #define HW_BREAKPOINT_SIZE 0x8
> #endif
> +#define HW_BREAKPOINT_SIZE_QUADWORD 0x10
>
> #define DABR_MAX_LEN 8
> #define DAWR_MAX_LEN 512
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 1f4a1efa0074..9f7df1c37233 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -520,9 +520,17 @@ static bool ea_hw_range_overlaps(unsigned long ea, int size,
> struct arch_hw_breakpoint *info)
> {
> unsigned long hw_start_addr, hw_end_addr;
> + unsigned long align_size = HW_BREAKPOINT_SIZE;
>
> - hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
> - hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
> + /*
> + * On p10 predecessors, quadword is handle differently then
> + * other instructions.
> + */
> + if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> + align_size = HW_BREAKPOINT_SIZE_QUADWORD;
> +
> + hw_start_addr = ALIGN_DOWN(info->address, align_size);
> + hw_end_addr = ALIGN(info->address + info->len, align_size);
>
> return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
> }
>
On 9/17/20 6:54 PM, Rogerio Alves wrote:
> On 9/2/20 1:29 AM, Ravi Bangoria wrote:
>> Patch #1 fixes issue for quardword instruction on p10 predecessors.
>> Patch #2 fixes issue for vector instructions.
>> Patch #3 fixes a bug about watchpoint not firing when created with
>> ptrace PPC_PTRACE_SETHWDEBUG and CONFIG_HAVE_HW_BREAKPOINT=N.
>> The fix uses HW_BRK_TYPE_PRIV_ALL for ptrace user which, I
>> guess, should be fine because we don't leak any kernel
>> addresses and PRIV_ALL will also help to cover scenarios when
>> kernel accesses user memory.
>> Patch #4,#5 fixes infinite exception bug, again the bug happens only
>> with CONFIG_HAVE_HW_BREAKPOINT=N.
>> Patch #6 fixes two places where we are missing to set hw_len.
>> Patch #7 introduce new feature bit PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
>> which will be set when running on ISA 3.1 compliant machine.
>> Patch #8 finally adds selftest to test scenarios fixed by patch#2,#3
>> and also moves MODE_EXACT tests outside of BP_RANGE condition.
>>
[...]
>
> Tested this patch set for:
> - SETHWDEBUG when CONFIG_HAVE_HW_BREAKPOINT=N = OK
> - Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N = OK
> - Check for PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 = OK
> - Fix quarword instruction handling on p10 predecessors = OK
> - Fix handling of vector instructions = OK
>
> Also tested for:
> - Set second watchpoint (P10 Mambo) = OK
> - Infinity loop on sc instruction = OK
Thanks Rogerio!
Ravi
Rogerio Alves <[email protected]> writes:
> On 9/2/20 1:29 AM, Ravi Bangoria wrote:
>> Patch #1 fixes issue for quardword instruction on p10 predecessors.
>> Patch #2 fixes issue for vector instructions.
>> Patch #3 fixes a bug about watchpoint not firing when created with
>> ptrace PPC_PTRACE_SETHWDEBUG and CONFIG_HAVE_HW_BREAKPOINT=N.
>> The fix uses HW_BRK_TYPE_PRIV_ALL for ptrace user which, I
>> guess, should be fine because we don't leak any kernel
>> addresses and PRIV_ALL will also help to cover scenarios when
>> kernel accesses user memory.
>> Patch #4,#5 fixes infinite exception bug, again the bug happens only
>> with CONFIG_HAVE_HW_BREAKPOINT=N.
>> Patch #6 fixes two places where we are missing to set hw_len.
>> Patch #7 introduce new feature bit PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
>> which will be set when running on ISA 3.1 compliant machine.
>> Patch #8 finally adds selftest to test scenarios fixed by patch#2,#3
>> and also moves MODE_EXACT tests outside of BP_RANGE condition.
>>
>> Christophe, let me know if this series breaks something for 8xx.
>>
>> v5: https://lore.kernel.org/r/[email protected]
>>
>> v5->v6:
>> - Fix build faulure reported by kernel test robot
>> - patch #5. Use more compact if condition, suggested by Christophe
>>
>>
>> Ravi Bangoria (8):
>> powerpc/watchpoint: Fix quarword instruction handling on p10
>> predecessors
>> powerpc/watchpoint: Fix handling of vector instructions
>> powerpc/watchpoint/ptrace: Fix SETHWDEBUG when
>> CONFIG_HAVE_HW_BREAKPOINT=N
>> powerpc/watchpoint: Move DAWR detection logic outside of
>> hw_breakpoint.c
>> powerpc/watchpoint: Fix exception handling for
>> CONFIG_HAVE_HW_BREAKPOINT=N
>> powerpc/watchpoint: Add hw_len wherever missing
>> powerpc/watchpoint/ptrace: Introduce PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
>> powerpc/watchpoint/selftests: Tests for kernel accessing user memory
>>
>> Documentation/powerpc/ptrace.rst | 1 +
>> arch/powerpc/include/asm/hw_breakpoint.h | 12 ++
>> arch/powerpc/include/uapi/asm/ptrace.h | 1 +
>> arch/powerpc/kernel/Makefile | 3 +-
>> arch/powerpc/kernel/hw_breakpoint.c | 149 +---------------
>> .../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
>> arch/powerpc/kernel/process.c | 48 ++++++
>> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 9 +-
>> arch/powerpc/xmon/xmon.c | 1 +
>> .../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 +++++-
>> 10 files changed, 282 insertions(+), 152 deletions(-)
>> create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
>>
>
> Tested this patch set for:
> - SETHWDEBUG when CONFIG_HAVE_HW_BREAKPOINT=N = OK
> - Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N = OK
> - Check for PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 = OK
> - Fix quarword instruction handling on p10 predecessors = OK
> - Fix handling of vector instructions = OK
>
> Also tested for:
> - Set second watchpoint (P10 Mambo) = OK
> - Infinity loop on sc instruction = OK
Thanks.
I wasn't able to pick up your Tested-by tags as I'd already applied the
patches, but thanks for sending them anyway, they will live on in the
mailing list archives for eternity.
cheers