| Message ID | 20210201194012.524831-3-avagin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/ptrace: allow to get all registers on syscall traps | expand |
On Mon, Feb 01, 2021 at 11:40:11AM -0800, Andrei Vagin wrote: > We have some ABI weirdness 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 behavior was inherited from ARM and it isn't common for other > architectures. Now, we have PTRACE_GET_SYSCALL_INFO that gives all > required information about system calls, so the hack with clobbering > registers isn't needed anymore. > > This change adds the new ptrace option PTRACE_O_ARM64_RAW_REGS. If it > is set, PTRACE_GETREGSET returns values of all registers without > clobbering r12 or x7 and PTRACE_SETREGSE sets all registers even if a > process has been stopped in syscall-enter or syscall-exit. > > Signed-off-by: Andrei Vagin <avagin@gmail.com> > --- > arch/arm64/include/uapi/asm/ptrace.h | 4 ++ > arch/arm64/kernel/ptrace.c | 70 ++++++++++++++++------------ > include/linux/ptrace.h | 1 + > include/uapi/linux/ptrace.h | 9 +++- > 4 files changed, 52 insertions(+), 32 deletions(-) Please split this up so that the arm64-specific changes are separate to the core changes. > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h > index 83ee45fa634b..bcc8c362ddd9 100644 > --- a/include/uapi/linux/ptrace.h > +++ b/include/uapi/linux/ptrace.h > @@ -7,6 +7,7 @@ > /* has the defines to get at the registers. */ > > #include <linux/types.h> > +#include <asm/ptrace.h> > > #define PTRACE_TRACEME 0 > #define PTRACE_PEEKTEXT 1 > @@ -137,8 +138,14 @@ struct ptrace_syscall_info { > #define PTRACE_O_EXITKILL (1 << 20) > #define PTRACE_O_SUSPEND_SECCOMP (1 << 21) > > +/* (1<<28) is reserved for arch specific options. */ > +#ifndef _PTRACE_O_ARCH_OPTIONS > +#define _PTRACE_O_ARCH_OPTIONS 0 > +#endif It seems a bit fragile to rely on a comment here to define the user ABI; why not define _PTRACE_O_ARCH_OPTIONS to the right value unconditionally? Also, it seems as though we immediately burn this bit on arm64, so if we ever wanted another option we'd have to come back here and allocate another bit. Could we do better, e.g. by calling into an arch hook (arch_ptrace_setoptions()) and passing the 'addr' parameter? How do other architectures manage this sort of thing? I'm wondering whether a separate regset containing just "real x7" and orig_x0 would be preferable after all... Will
On Thu, Feb 04, 2021 at 03:36:15PM +0000, Will Deacon wrote: > On Mon, Feb 01, 2021 at 11:40:11AM -0800, Andrei Vagin wrote: > > We have some ABI weirdness 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 behavior was inherited from ARM and it isn't common for other > > architectures. Now, we have PTRACE_GET_SYSCALL_INFO that gives all > > required information about system calls, so the hack with clobbering > > registers isn't needed anymore. > > > > This change adds the new ptrace option PTRACE_O_ARM64_RAW_REGS. If it > > is set, PTRACE_GETREGSET returns values of all registers without > > clobbering r12 or x7 and PTRACE_SETREGSE sets all registers even if a > > process has been stopped in syscall-enter or syscall-exit. > > > > Signed-off-by: Andrei Vagin <avagin@gmail.com> > > --- > > arch/arm64/include/uapi/asm/ptrace.h | 4 ++ > > arch/arm64/kernel/ptrace.c | 70 ++++++++++++++++------------ > > include/linux/ptrace.h | 1 + > > include/uapi/linux/ptrace.h | 9 +++- > > 4 files changed, 52 insertions(+), 32 deletions(-) > > Please split this up so that the arm64-specific changes are separate to > the core changes. ok > > > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h > > index 83ee45fa634b..bcc8c362ddd9 100644 > > --- a/include/uapi/linux/ptrace.h > > +++ b/include/uapi/linux/ptrace.h > > @@ -7,6 +7,7 @@ > > /* has the defines to get at the registers. */ > > > > #include <linux/types.h> > > +#include <asm/ptrace.h> > > > > #define PTRACE_TRACEME 0 > > #define PTRACE_PEEKTEXT 1 > > @@ -137,8 +138,14 @@ struct ptrace_syscall_info { > > #define PTRACE_O_EXITKILL (1 << 20) > > #define PTRACE_O_SUSPEND_SECCOMP (1 << 21) > > > > +/* (1<<28) is reserved for arch specific options. */ > > +#ifndef _PTRACE_O_ARCH_OPTIONS > > +#define _PTRACE_O_ARCH_OPTIONS 0 > > +#endif > > It seems a bit fragile to rely on a comment here to define the user ABI; > why not define _PTRACE_O_ARCH_OPTIONS to the right value unconditionally? We don't want to allow setting options that are not supported. _PTRACE_O_ARCH_OPTIONS is added to PTRACE_O_MASK and then ptrace_setoptions checks whether all specified options is supported or not. > > Also, it seems as though we immediately burn this bit on arm64, so if we > ever wanted another option we'd have to come back here and allocate another > bit. Could we do better, e.g. by calling into an arch hook > (arch_ptrace_setoptions()) and passing the 'addr' parameter? I am not sure that I understand the idea. Do you suggest to have PTRACE_O_ARCH_OPTION and pass arch-specific options in addr? In this case, I think it could be more cleaner to introduce a new ptrace command. If this is the idea, I think it worth doing this only if we expect to have more than one,two,three options. As for my solution, we need to come back to allocate a new bit to be sure that we don't intersect with non-arch specific options. And those who add a non-arch option should see that they don't use bits of arch-specific options. Let's decide what interface we want to use to solve the problem and then if we will stop on a ptrace option I will figure out how to improve this code. > > How do other architectures manage this sort of thing? I'm wondering whether > a separate regset containing just "real x7" and orig_x0 would be preferable > after all... Yeah, it might be a good idea. We will need to do one extra ptrace system call, but in comparison with ptrace context-switches, this is nothing. Dave, Keno, what do you think about this? > > Will
diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h index 758ae984ff97..465cc9713895 100644 --- a/arch/arm64/include/uapi/asm/ptrace.h +++ b/arch/arm64/include/uapi/asm/ptrace.h @@ -109,6 +109,10 @@ struct user_hwdebug_state { } dbg_regs[16]; }; +#define PTRACE_O_ARM64_RAW_REGS (1 << 28) + +#define _PTRACE_O_ARCH_OPTIONS PTRACE_O_ARM64_RAW_REGS + /* SVE/FP/SIMD state (NT_ARM_SVE) */ struct user_sve_header { diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 39da03104528..591a4478ad76 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -565,21 +565,23 @@ static int gpr_get(struct task_struct *target, 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. - */ saved_reg = uregs->regs[7]; - switch (target->ptrace_message) { - case PTRACE_EVENTMSG_SYSCALL_ENTRY: - uregs->regs[7] = PTRACE_SYSCALL_ENTER; - break; - case PTRACE_EVENTMSG_SYSCALL_EXIT: - uregs->regs[7] = PTRACE_SYSCALL_EXIT; - break; + if (!(target->ptrace & PT_ARM64_RAW_REGS)) { + /* + * 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: + uregs->regs[7] = PTRACE_SYSCALL_ENTER; + break; + case PTRACE_EVENTMSG_SYSCALL_EXIT: + uregs->regs[7] = PTRACE_SYSCALL_EXIT; + break; + } } ret = membuf_write(&to, uregs, sizeof(*uregs)); @@ -600,15 +602,17 @@ static int gpr_set(struct task_struct *target, const struct user_regset *regset, if (ret) return ret; - /* - * Historically, x7 can't be changed if the stop has been signalled - * from syscall-enter of syscall-exit. - */ - switch (target->ptrace_message) { - case PTRACE_EVENTMSG_SYSCALL_ENTRY: - case PTRACE_EVENTMSG_SYSCALL_EXIT: - newregs.regs[7] = task_pt_regs(target)->regs[7]; - break; + if (!(target->ptrace & PT_ARM64_RAW_REGS)) { + /* + * Historically, x7 can't be changed if the stop has been + * signalled from syscall-enter of syscall-exit. + */ + switch (target->ptrace_message) { + case PTRACE_EVENTMSG_SYSCALL_ENTRY: + case PTRACE_EVENTMSG_SYSCALL_EXIT: + newregs.regs[7] = task_pt_regs(target)->regs[7]; + break; + } } if (!valid_user_regs(&newregs, target)) @@ -1243,6 +1247,8 @@ static inline compat_ulong_t compat_get_user_reg(struct task_struct *task, int i switch (idx) { case 12: + if (task->ptrace & PT_ARM64_RAW_REGS) + return regs->regs[idx]; /* * We have some ABI weirdness here in the way that we handle * syscall exit stops because we indicate whether or not the @@ -1332,15 +1338,17 @@ static int compat_gpr_set(struct task_struct *target, } - /* - * Historically, x12 can't be changed if the stop has been signalled - * from syscall-enter of syscall-exit. - */ - switch (target->ptrace_message) { - case PTRACE_EVENTMSG_SYSCALL_ENTRY: - case PTRACE_EVENTMSG_SYSCALL_EXIT: - newregs.regs[12] = task_pt_regs(target)->regs[12]; - break; + if (!(target->ptrace & PT_ARM64_RAW_REGS)) { + /* + * Historically, r12 can't be changed if the stop has been + * signalled from syscall-enter of syscall-exit. + */ + switch (target->ptrace_message) { + case PTRACE_EVENTMSG_SYSCALL_ENTRY: + case PTRACE_EVENTMSG_SYSCALL_EXIT: + newregs.regs[12] = task_pt_regs(target)->regs[12]; + break; + } } if (valid_user_regs(&newregs.user_regs, target)) diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index 2a9df80ea887..987d6ec5f0ce 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -46,6 +46,7 @@ extern int ptrace_access_vm(struct task_struct *tsk, unsigned long addr, #define PT_EXITKILL (PTRACE_O_EXITKILL << PT_OPT_FLAG_SHIFT) #define PT_SUSPEND_SECCOMP (PTRACE_O_SUSPEND_SECCOMP << PT_OPT_FLAG_SHIFT) +#define PT_ARM64_RAW_REGS (PTRACE_O_ARM64_RAW_REGS << PT_OPT_FLAG_SHIFT) /* single stepping state bits (used on ARM and PA-RISC) */ #define PT_SINGLESTEP_BIT 31 diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h index 83ee45fa634b..bcc8c362ddd9 100644 --- a/include/uapi/linux/ptrace.h +++ b/include/uapi/linux/ptrace.h @@ -7,6 +7,7 @@ /* has the defines to get at the registers. */ #include <linux/types.h> +#include <asm/ptrace.h> #define PTRACE_TRACEME 0 #define PTRACE_PEEKTEXT 1 @@ -137,8 +138,14 @@ struct ptrace_syscall_info { #define PTRACE_O_EXITKILL (1 << 20) #define PTRACE_O_SUSPEND_SECCOMP (1 << 21) +/* (1<<28) is reserved for arch specific options. */ +#ifndef _PTRACE_O_ARCH_OPTIONS +#define _PTRACE_O_ARCH_OPTIONS 0 +#endif + #define PTRACE_O_MASK (\ - 0x000000ff | PTRACE_O_EXITKILL | PTRACE_O_SUSPEND_SECCOMP) + 0x000000ff | PTRACE_O_EXITKILL | \ + PTRACE_O_SUSPEND_SECCOMP | _PTRACE_O_ARCH_OPTIONS) #include <asm/ptrace.h>
We have some ABI weirdness 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 behavior was inherited from ARM and it isn't common for other architectures. Now, we have PTRACE_GET_SYSCALL_INFO that gives all required information about system calls, so the hack with clobbering registers isn't needed anymore. This change adds the new ptrace option PTRACE_O_ARM64_RAW_REGS. If it is set, PTRACE_GETREGSET returns values of all registers without clobbering r12 or x7 and PTRACE_SETREGSE sets all registers even if a process has been stopped in syscall-enter or syscall-exit. Signed-off-by: Andrei Vagin <avagin@gmail.com> --- arch/arm64/include/uapi/asm/ptrace.h | 4 ++ arch/arm64/kernel/ptrace.c | 70 ++++++++++++++++------------ include/linux/ptrace.h | 1 + include/uapi/linux/ptrace.h | 9 +++- 4 files changed, 52 insertions(+), 32 deletions(-)