| 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: <20210225160002.GA143918@gmail.com>
Date: Thu, 25 Feb 2021 08:00:02 -0800
From: Andrei Vagin <avagin@...il.com>
To: Will Deacon <will@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
Oleg Nesterov <oleg@...hat.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-api@...r.kernel.org,
Anthony Steinhauser <asteinhauser@...gle.com>,
Dave Martin <Dave.Martin@....com>,
Keno Fischer <keno@...iacomputing.com>
Subject: Re: [PATCH 1/3] arm64/ptrace: don't clobber task registers on
syscall entry/exit traps
On Thu, Feb 04, 2021 at 03:23:34PM +0000, Will Deacon wrote:
> On Mon, Feb 01, 2021 at 11:40:10AM -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 the 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
> > ptrace option to get the full set of registers.
> >
> > Signed-off-by: Andrei Vagin <avagin@...il.com>
> > ---
> > arch/arm64/include/asm/ptrace.h | 5 ++
> > arch/arm64/kernel/ptrace.c | 104 ++++++++++++++++++++------------
> > 2 files changed, 69 insertions(+), 40 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..39da03104528 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,31 @@ 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.
> > + */
>
> When you move a comment, please don't truncate it!
This is my fault. In the previous version, the other part of this
comment was irelevant, because I always allowed to change clobbered
registers, but then I realized that we can't do that.
>
> > + 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;
> > + }
>
> I'm wary of checking target->ptrace_message here, as I seem to recall the
> regset code also being used for coredumps. What guarantees we don't break
> things there?
Registers were clobbered in tracehook_report_syscall,
task->ptrace_message is set in ptrace_report_syscall.
do_coredump() is called from get_signal and secure_computing, so we
always see actuall registers in core dumps with and without these
changes.
>
> > +
> > + ret = membuf_write(&to, uregs, sizeof(*uregs));
> > +
> > + uregs->regs[7] = saved_reg;
> > +
> > + return ret;
> > }
> >
> > static int gpr_set(struct task_struct *target, const struct user_regset *regset,
> > @@ -575,6 +600,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 (!valid_user_regs(&newregs, target))
> > return -EINVAL;
> >
> > @@ -1206,6 +1242,20 @@ static inline compat_ulong_t compat_get_user_reg(struct task_struct *task, int i
> > struct pt_regs *regs = task_pt_regs(task);
> >
> > switch (idx) {
> > + case 12:
> > + /*
> > + * 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 (task->ptrace_message) {
> > + case PTRACE_EVENTMSG_SYSCALL_ENTRY:
> > + return PTRACE_SYSCALL_ENTER;
> > + case PTRACE_EVENTMSG_SYSCALL_EXIT:
> > + return PTRACE_SYSCALL_EXIT;
> > + }
> > + return regs->regs[idx];
> > case 15:
> > return regs->pc;
> > case 16:
> > @@ -1282,6 +1332,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 (valid_user_regs(&newregs.user_regs, target))
> > *task_pt_regs(target) = newregs;
> > else
> > @@ -1740,53 +1801,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);
>
> Again, please preserve the comment in some form (maybe "... if we are
> stepping since tracer ...").
ok
>
> That said, doesn't your change above break the pseudo-step trap? Currently,
> we report the real x7 for those.
No, it doesn't.
In case of singlestep, tracehook_report_syscall_exit calls
user_single_step_report instead of ptrace_report_syscall, so
current->ptace_message will not be set PTRACE_EVENTMSG_SYSCALL_EXIT.
>
> Will
Powered by blists - more mailing lists