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 <[email protected]>
---
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;
+
+ 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);
+ }
+
return 0;
}
@@ -1740,53 +1797,16 @@ long arch_ptrace(struct task_struct *child, long request,
return ptrace_request(child, request, addr, data);
}
-enum ptrace_syscall_dir {
- PTRACE_SYSCALL_ENTER = 0,
- PTRACE_SYSCALL_EXIT,
-};
-
static void tracehook_report_syscall(struct pt_regs *regs,
enum ptrace_syscall_dir dir)
{
- int regno;
- unsigned long saved_reg;
-
- /*
- * 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 a general
- * purpose register (ip/r12 for AArch32, x7 for AArch64) in the tracee
- * and restoring its old value after the stop. 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.
- *
- * - Syscall stops behave differently to seccomp and pseudo-step traps
- * (the latter do not nobble any registers).
- */
- regno = (is_compat_task() ? 12 : 7);
- saved_reg = regs->regs[regno];
- regs->regs[regno] = dir;
-
if (dir == PTRACE_SYSCALL_ENTER) {
if (tracehook_report_syscall_entry(regs))
forget_syscall(regs);
- regs->regs[regno] = saved_reg;
- } else if (!test_thread_flag(TIF_SINGLESTEP)) {
- tracehook_report_syscall_exit(regs, 0);
- regs->regs[regno] = saved_reg;
} else {
- regs->regs[regno] = saved_reg;
+ int singlestep = test_thread_flag(TIF_SINGLESTEP);
- /*
- * Signal a pseudo-step exception since we are stepping but
- * tracer modifications to the registers may have rewound the
- * state machine.
- */
- tracehook_report_syscall_exit(regs, 1);
+ tracehook_report_syscall_exit(regs, singlestep);
}
}
--
2.29.2
On Tue, Jan 19, 2021 at 02:06:35PM -0800, Andrei Vagin wrote:
> 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 <[email protected]>
> ---
> 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;
> +
> + 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