Thread (8 messages) 8 messages, 2 authors, 2021-01-29

Re: [PATCH 1/3] arm64/ptrace: don't clobber task registers on syscall entry/exit traps

From: Dave Martin <Dave.Martin@arm.com>
Date: 2021-01-27 15:22:35
Also in: lkml

On Tue, Jan 19, 2021 at 02:06:35PM -0800, Andrei Vagin wrote:
quoted hunk ↗ jump to hunk
ip/r12 for AArch32 and x7 for AArch64 is used to indicate whether or not
the stop has been signalled from syscall entry or syscall exit. This
means that:

- Any writes by the tracer to this register during the stop are
  ignored/discarded.

- The actual value of the register is not available during the stop,
  so the tracer cannot save it and restore it later.

Right now, these registers are clobbered in tracehook_report_syscall.
This change moves this logic to gpr_get and compat_gpr_get where
registers are copied into a user-space buffer.

This will allow to change these registers and to introduce a new
NT_ARM_PRSTATUS command to get the full set of registers.

Signed-off-by: Andrei Vagin <redacted>
---
 arch/arm64/include/asm/ptrace.h |   5 ++
 arch/arm64/kernel/ptrace.c      | 104 +++++++++++++++++++-------------
 2 files changed, 67 insertions(+), 42 deletions(-)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index e58bca832dff..0a9552b4f61e 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -170,6 +170,11 @@ static inline unsigned long pstate_to_compat_psr(const unsigned long pstate)
 	return psr;
 }
 
+enum ptrace_syscall_dir {
+	PTRACE_SYSCALL_ENTER = 0,
+	PTRACE_SYSCALL_EXIT,
+};
+
 /*
  * This struct defines the way the registers are stored on the stack during an
  * exception. Note that sizeof(struct pt_regs) has to be a multiple of 16 (for
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 8ac487c84e37..1863f080cb07 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -40,6 +40,7 @@
 #include <asm/syscall.h>
 #include <asm/traps.h>
 #include <asm/system_misc.h>
+#include <asm/ptrace.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
@@ -561,7 +562,33 @@ static int gpr_get(struct task_struct *target,
 		   struct membuf to)
 {
 	struct user_pt_regs *uregs = &task_pt_regs(target)->user_regs;
-	return membuf_write(&to, uregs, sizeof(*uregs));
+	unsigned long saved_reg;
+	int ret;
+
+	/*
+	 * We have some ABI weirdness here in the way that we handle syscall
+	 * exit stops because we indicate whether or not the stop has been
+	 * signalled from syscall entry or syscall exit by clobbering the general
+	 * purpose register x7.
+	 */
+	switch (target->ptrace_message) {
+	case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+		saved_reg = uregs->regs[7];
+		uregs->regs[7] = PTRACE_SYSCALL_ENTER;
+		break;
+	case PTRACE_EVENTMSG_SYSCALL_EXIT:
+		saved_reg = uregs->regs[7];
+		uregs->regs[7] = PTRACE_SYSCALL_EXIT;
+		break;
+	}
+
+	ret =  membuf_write(&to, uregs, sizeof(*uregs));
+
+	if (target->ptrace_message == PTRACE_EVENTMSG_SYSCALL_ENTRY ||
+	    target->ptrace_message == PTRACE_EVENTMSG_SYSCALL_EXIT)
+		uregs->regs[7] = saved_reg;
This might be a reasonable cleanup even if the extra regset isn't
introduced: it makes it clear that we're not changing the user registers
here, just the tracer's view of them.

I'm assuming it doesn't break tracing anywhere else.  I can't think of
anything it would break just now, but I haven't spent much time looking
into it.


Can you not just unconditionally back up and restore regs[7] here?  e.g.

	saved_reg = uregs->regs[7];

	switch (target->ptrace_message) {
	case PTRACE_EVENTMSG_SYSCALL_ENTRY:
	case PTRACE_EVENTMSG_SYSCALL_EXIT:
		uregs->regs[7] = target->ptrace_message;
	}

	ret = membuf_write(...);

	uregs->regs[7] = saved_reg;

quoted hunk ↗ jump to hunk
+
+	return ret;
 }
 
 static int gpr_set(struct task_struct *target, const struct user_regset *regset,
@@ -1221,10 +1248,40 @@ static int compat_gpr_get(struct task_struct *target,
 			  const struct user_regset *regset,
 			  struct membuf to)
 {
+	compat_ulong_t r12;
+	bool overwrite_r12;
 	int i = 0;
 
-	while (to.left)
-		membuf_store(&to, compat_get_user_reg(target, i++));
+	/*
+	 * We have some ABI weirdness here in the way that we handle syscall
+	 * exit stops because we indicate whether or not the stop has been
+	 * signalled from syscall entry or syscall exit by clobbering the
+	 * general purpose register r12.
+	 */
+	switch (target->ptrace_message) {
+	case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+		r12 = PTRACE_SYSCALL_ENTER;
+		overwrite_r12 = true;
+		break;
+	case PTRACE_EVENTMSG_SYSCALL_EXIT:
+		r12 = PTRACE_SYSCALL_EXIT;
+		overwrite_r12 = true;
+		break;
+	default:
+		overwrite_r12 = false;
+		break;
+	}
+
+	while (to.left) {
+		compat_ulong_t val;
+
+		if (!overwrite_r12 || i != 12)
+			val = compat_get_user_reg(target, i++);
+		else
+			val = r12;
+		membuf_store(&to, val);
+	}
+
Can this be condensed too, say by introducing a wrapper for
compat_get_user_reg() that does the fudging on r12?

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help