| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aIEaeo-tNehdTeci@debug.ba.rivosinc.com>
Date: Wed, 23 Jul 2025 10:23:06 -0700
From: Deepak Gupta <debug@...osinc.com>
To: Jesse Taube <jesse@...osinc.com>
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
Oleg Nesterov <oleg@...hat.com>,
Himanshu Chauhan <hchauhan@...tanamicro.com>,
Charlie Jenkins <charlie@...osinc.com>,
Samuel Holland <samuel.holland@...ive.com>,
Andrew Jones <ajones@...tanamicro.com>,
Atish Patra <atishp@...osinc.com>,
Anup Patel <apatel@...tanamicro.com>,
Mayuresh Chitale <mchitale@...tanamicro.com>,
Conor Dooley <conor.dooley@...rochip.com>,
WangYuli <wangyuli@...ontech.com>,
Huacai Chen <chenhuacai@...nel.org>, Nam Cao <namcao@...utronix.de>,
Andrew Morton <akpm@...ux-foundation.org>,
"Mike Rapoport (Microsoft)" <rppt@...nel.org>,
Luis Chamberlain <mcgrof@...nel.org>,
Yunhui Cui <cuiyunhui@...edance.com>,
Joel Granados <joel.granados@...nel.org>,
Clément Léger <cleger@...osinc.com>,
Celeste Liu <coelacanthushex@...il.com>,
Evan Green <evan@...osinc.com>, Nylon Chen <nylon.chen@...ive.com>
Subject: Re: [RFC PATCH 6/6] riscv: ptrace: Add hw breakpoint support
On Wed, Jul 23, 2025 at 09:55:25AM -0700, Jesse Taube wrote:
>On Tue, Jul 22, 2025 at 9:18 PM Deepak Gupta <debug@...osinc.com> wrote:
>>
>> On Tue, Jul 22, 2025 at 10:38:29AM -0700, Jesse Taube wrote:
>> >Add ability to setup hw breakpoints to ptrace. Call defines a new
>> >structure of (ulong[3]){bp_addr, bp_len, bp_type} with
>> >bp_type being one of HW_BREAKPOINT_LEN_X and
>> >bp_len being one of HW_BREAKPOINT_X with a value of
>> >zero dissabling the breakpoint.
>> >
>> >Signed-off-by: Jesse Taube <jesse@...osinc.com>
>> >---
>> > arch/riscv/include/asm/processor.h | 4 ++
>> > arch/riscv/include/uapi/asm/ptrace.h | 3 +-
>> > arch/riscv/kernel/hw_breakpoint.c | 14 ++++-
>> > arch/riscv/kernel/process.c | 4 ++
>> > arch/riscv/kernel/ptrace.c | 93 ++++++++++++++++++++++++++++
>> > 5 files changed, 116 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
>> >index 5f56eb9d114a..488d956a951f 100644
>> >--- a/arch/riscv/include/asm/processor.h
>> >+++ b/arch/riscv/include/asm/processor.h
>> >@@ -12,6 +12,7 @@
>> >
>> > #include <vdso/processor.h>
>> >
>> >+#include <asm/hw_breakpoint.h>
>> > #include <asm/ptrace.h>
>> >
>> > #define arch_get_mmap_end(addr, len, flags) \
>> >@@ -108,6 +109,9 @@ struct thread_struct {
>> > struct __riscv_v_ext_state vstate;
>> > unsigned long align_ctl;
>> > struct __riscv_v_ext_state kernel_vstate;
>> >+#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> >+ struct perf_event *ptrace_bps[RV_MAX_TRIGGERS];
>> >+#endif
>> > #ifdef CONFIG_SMP
>> > /* Flush the icache on migration */
>> > bool force_icache_flush;
>> >diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
>> >index a38268b19c3d..a7998ed41913 100644
>> >--- a/arch/riscv/include/uapi/asm/ptrace.h
>> >+++ b/arch/riscv/include/uapi/asm/ptrace.h
>> >@@ -14,7 +14,8 @@
>> >
>> > #define PTRACE_GETFDPIC_EXEC 0
>> > #define PTRACE_GETFDPIC_INTERP 1
>> >-
>> >+#define PTRACE_GETHBPREGS 2
>> >+#define PTRACE_SETHBPREGS 3
>>
>> Why not use `PTRACE_GETREGSET` `PTRACE_SETREGSET` ?
>
>Because it was easier to implement this first, and REGSET will be
>another commit ontop of this one.
>Unless there is a reason to not have this version.
>
Yeah I don't have strong feelings on this. Whatever is best for the tools
(gdb, etc) who will consume this interface, lets do that.
>>
>> > /*
>> > * User-mode register state for core dumps, ptrace, sigcontext
>> > *
>> >diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
>> >index 437fd82b9590..c58145464539 100644
>> >--- a/arch/riscv/kernel/hw_breakpoint.c
>> >+++ b/arch/riscv/kernel/hw_breakpoint.c
>> >@@ -633,7 +633,19 @@ void arch_uninstall_hw_breakpoint(struct perf_event *event)
>> > pr_warn("%s: Failed to uninstall trigger %d. error: %ld\n", __func__, i, ret.error);
>> > }
>> >
>> >-void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
>> >+/*
>> >+ * Release the user breakpoints used by ptrace
>> >+ */
>> >+void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>> >+{
>> >+ int i;
>> >+ struct thread_struct *t = &tsk->thread;
>> >+
>> >+ for (i = 0; i < dbtr_total_num; i++) {
>> >+ unregister_hw_breakpoint(t->ptrace_bps[i]);
>> >+ t->ptrace_bps[i] = NULL;
>> >+ }
>> >+}
>> >
>> > void hw_breakpoint_pmu_read(struct perf_event *bp) { }
>> >
>> >diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
>> >index 15d8f75902f8..9cf07ecfb523 100644
>> >--- a/arch/riscv/kernel/process.c
>> >+++ b/arch/riscv/kernel/process.c
>> >@@ -9,6 +9,7 @@
>> >
>> > #include <linux/bitfield.h>
>> > #include <linux/cpu.h>
>> >+#include <linux/hw_breakpoint.h>
>> > #include <linux/kernel.h>
>> > #include <linux/sched.h>
>> > #include <linux/sched/debug.h>
>> >@@ -164,6 +165,7 @@ void start_thread(struct pt_regs *regs, unsigned long pc,
>> >
>> > void flush_thread(void)
>> > {
>> >+ flush_ptrace_hw_breakpoint(current);
>> > #ifdef CONFIG_FPU
>> > /*
>> > * Reset FPU state and context
>> >@@ -218,6 +220,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>> > set_bit(MM_CONTEXT_LOCK_PMLEN, &p->mm->context.flags);
>> >
>> > memset(&p->thread.s, 0, sizeof(p->thread.s));
>> >+ if (IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
>> >+ memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
>> >
>> > /* p->thread holds context to be restored by __switch_to() */
>> > if (unlikely(args->fn)) {
>> >diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
>> >index ea67e9fb7a58..b78cfb0f1c0e 100644
>> >--- a/arch/riscv/kernel/ptrace.c
>> >+++ b/arch/riscv/kernel/ptrace.c
>> >@@ -9,11 +9,13 @@
>> >
>> > #include <asm/vector.h>
>> > #include <asm/ptrace.h>
>> >+#include <asm/hw_breakpoint.h>
>> > #include <asm/syscall.h>
>> > #include <asm/thread_info.h>
>> > #include <asm/switch_to.h>
>> > #include <linux/audit.h>
>> > #include <linux/compat.h>
>> >+#include <linux/hw_breakpoint.h>
>> > #include <linux/ptrace.h>
>> > #include <linux/elf.h>
>> > #include <linux/regset.h>
>> >@@ -336,12 +338,103 @@ void ptrace_disable(struct task_struct *child)
>> > {
>> > }
>> >
>> >+#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> >+static void ptrace_hbptriggered(struct perf_event *bp,
>> >+ struct perf_sample_data *data,
>> >+ struct pt_regs *regs)
>> >+{
>> >+ struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
>> >+ int num = 0;
>> >+
>> >+ force_sig_ptrace_errno_trap(num, (void __user *)bkpt->address);
>> >+}
>> >+
>> >+/*
>> >+ * idx selects the breakpoint index.
>> >+ * Both PTRACE_GETHBPREGS and PTRACE_SETHBPREGS transfer three 32-bit words:
>> >+ * address (0), length (1), type (2).
>> >+ * Instruction breakpoint length is one of HW_BREAKPOINT_LEN_X or 0. 0 will
>> >+ * disable the breakpoint.
>> >+ * Instruction breakpoint type is one of HW_BREAKPOINT_X.
>> >+ */
>> >+
>> >+static long ptrace_gethbpregs(struct task_struct *child, unsigned long idx,
>> >+ unsigned long __user *datap)
>> >+{
>> >+ struct perf_event *bp;
>> >+ unsigned long user_data[3] = {0};
>> >+
>> >+ if (idx >= RV_MAX_TRIGGERS)
>> >+ return -EINVAL;
>> >+
>> >+ bp = child->thread.ptrace_bps[idx];
>> >+
>> >+ if (!IS_ERR_OR_NULL(bp)) {
>> >+ user_data[0] = bp->attr.bp_addr;
>> >+ user_data[1] = bp->attr.disabled ? 0 : bp->attr.bp_len;
>> >+ user_data[2] = bp->attr.bp_type;
>> >+ }
>> >+
>> >+ if (copy_to_user(datap, user_data, sizeof(user_data)))
>> >+ return -EFAULT;
>> >+
>> >+ return 0;
>> >+}
>> >+
>> >+static long ptrace_sethbpregs(struct task_struct *child, unsigned long idx,
>> >+ unsigned long __user *datap)
>> >+{
>> >+ struct perf_event *bp;
>> >+ struct perf_event_attr attr;
>> >+ unsigned long user_data[3];
>> >+
>> >+ if (idx >= RV_MAX_TRIGGERS)
>> >+ return -EINVAL;
>> >+
>> >+ if (copy_from_user(user_data, datap, sizeof(user_data)))
>> >+ return -EFAULT;
>> >+
>> >+ bp = child->thread.ptrace_bps[idx];
>> >+ if (IS_ERR_OR_NULL(bp))
>>
>> Why not only check for NULL?
>> IS_ERR_VALUE will always expand to be true. right?
>
>Because im dumb and thought i was setting bp to an error code, but i'm not.
>Yes if (!bp) is right.
>
>>
>> >+ attr = bp->attr;
>> >+ else
>> >+ ptrace_breakpoint_init(&attr);
>> >+
>> >+ attr.bp_addr = user_data[0];
>> >+ attr.bp_len = user_data[1];
>> >+ attr.bp_type = user_data[2];
>> >+ attr.disabled = !attr.bp_len;
>>
>> Is it okay to not have any sanitization on inputs?
>>
>> Can these inputs be controlled by user to give kernel address and kernel
>> breakpoint?
>
>modify_user_hw_breakpoint calls modify_user_hw_breakpoint_check, which
>eventually checks if we have CAP_SYS_ADMIN.
>Same for register. type and len are also checked by the
>_user_hw_breakpoint_check functions and again in the riscv code.
>
>it would be nice if this could be double checked, but it does seem
>other architectures don't check addr aswell.
I see. That's interesting.
>
>Thanks,
>Jesse Taube
>
>>
>> >+
>> >+ if (IS_ERR_OR_NULL(bp)) {
>> >+ bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL,
>> >+ child);
>> >+ if (IS_ERR(bp))
>> >+ return PTR_ERR(bp);
>> >+
>> >+ child->thread.ptrace_bps[idx] = bp;
>> >+ return 0;
>> >+ } else {
>> >+ return modify_user_hw_breakpoint(bp, &attr);
>> >+ }
>> >+}
>> >+#endif
>> >+
>> > long arch_ptrace(struct task_struct *child, long request,
>> > unsigned long addr, unsigned long data)
>> > {
>> > long ret = -EIO;
>> >+ unsigned long __user *datap = (unsigned long __user *) data;
>> >
>> > switch (request) {
>> >+#ifdef CONFIG_HAVE_HW_BREAKPOINT
>> >+ case PTRACE_GETHBPREGS:
>> >+ ret = ptrace_gethbpregs(child, addr, datap);
>> >+ break;
>> >+
>> >+ case PTRACE_SETHBPREGS:
>> >+ ret = ptrace_sethbpregs(child, addr, datap);
>> >+ break;
>> >+#endif
>> > default:
>> > ret = ptrace_request(child, request, addr, data);
>> > break;
>> >--
>> >2.43.0
>> >
Powered by blists - more mailing lists