| 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: <CALSpo=Y+hLsVC+w942Bhg628HuDqG=MU9+f87R5X616rhG11Mw@mail.gmail.com>
Date: Wed, 23 Jul 2025 09:55:25 -0700
From: Jesse Taube <jesse@...osinc.com>
To: Deepak Gupta <debug@...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 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.
>
> > /*
> > * 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.
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