Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

[James Hogan]

On Fri, Aug 11, 2017 at 03:23:34PM -0700, Kees Cook wrote:
> On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
> > Add a PTRACE_SET_SYSCALL ptrace operation to allow the system call to be
> > cancelled independently to the value of the v0 system call number
> > register.
> >
> > This is needed for SECCOMP_RET_TRACE when the tracer wants to cancel the
> > system call, since it has to set both the system call number to -1 and
> > the chosen return value, both of which reside in the same register (v0).
> > The tracer should set the return value first, followed by
> > PTRACE_SET_SYSCALL to set the system call number to -1.
> >
> > That is in contrast to the normal ptrace syscall hook which triggers the
> > tracer on both entry and exit, allowing the system call to be cancelled
> > during the entry hook (setting system call number register to -1, or
> > optionally using PTRACE_SET_SYSCALL), separately to setting the return
> > value during the exit hook.
> >
> > Positive values (to change the syscall that should be executed instead
> > of cancelling it entirely) are explicitly disallowed at the moment. The
> > same thing can be done safely already by writing the v0 system call
> > number register and the argument registers, and allowing
> > thread_info::syscall to be changed to a different value independently of
> > the v0 register would potentially allow seccomp or the syscall trace
> > events to be fooled into thinking a different system call was being
> > executed.
> 
> Wouldn't the sycall be reloaded, so no spoofing could occur?

The case I was thinking of was:
- PTRACE_POKEUSR v0 = __NR_some_disallowed_syscall
- PTRACE_SET_SYSCALL __NR_some_allowed_syscall

syscall_get_nr() will return __NR_some_allowed_syscall, so seccomp will
allow, but when syscall_trace_enter() returns to syscall_trace_entry in
arch/mips/kernel/scall32-o32.S, it will reload the syscall number from
v0 (i.e. __NR_some_disallowed_syscall).

> 
> Regardless, can you update
> tools/testing/selftests/seccomp/seccomp_bpf.c to update or eliminate
> the MIPS-only SYSCALL_NUM_RET_SHARE_REG special-case? (Or maybe it
> needs to be further special-cased to split syscall-changing from
> syscall-cancelling?)

Sure, i'll look into that,

Thanks for reviewing,

Cheers
James

> 
> -Kees
> 
> >
> > Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> > Cc: Will Drewry <wad@xxxxxxxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxx
> > ---
> >  arch/mips/include/uapi/asm/ptrace.h |  1 +
> >  arch/mips/kernel/ptrace.c           | 11 +++++++++++
> >  arch/mips/kernel/ptrace32.c         | 11 +++++++++++
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/arch/mips/include/uapi/asm/ptrace.h b/arch/mips/include/uapi/asm/ptrace.h
> > index 91a3d197ede3..23af103c4e8d 100644
> > --- a/arch/mips/include/uapi/asm/ptrace.h
> > +++ b/arch/mips/include/uapi/asm/ptrace.h
> > @@ -58,6 +58,7 @@ struct pt_regs {
> >
> >  #define PTRACE_GET_THREAD_AREA 25
> >  #define PTRACE_SET_THREAD_AREA 26
> > +#define PTRACE_SET_SYSCALL     27
> >
> >  /* Calls to trace a 64bit program from a 32bit program.         */
> >  #define PTRACE_PEEKTEXT_3264   0xc0
> > diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> > index 465fc5633e61..9bf31a990c6e 100644
> > --- a/arch/mips/kernel/ptrace.c
> > +++ b/arch/mips/kernel/ptrace.c
> > @@ -853,6 +853,17 @@ long arch_ptrace(struct task_struct *child, long request,
> >                 ret = put_user(task_thread_info(child)->tp_value, datalp);
> >                 break;
> >
> > +       case PTRACE_SET_SYSCALL:
> > +               /*
> > +                * This is currently only useful to cancel the syscall from a
> > +                * seccomp RET_TRACE tracer.
> > +                */
> > +               if ((long)data >= 0)
> > +                       return -EINVAL;
> > +               task_thread_info(child)->syscall = -1;
> > +               ret = 0;
> > +               break;
> > +
> >         case PTRACE_GET_WATCH_REGS:
> >                 ret = ptrace_get_watch_regs(child, addrp);
> >                 break;
> > diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
> > index 2b9260f92ccd..cca76aec9c10 100644
> > --- a/arch/mips/kernel/ptrace32.c
> > +++ b/arch/mips/kernel/ptrace32.c
> > @@ -287,6 +287,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> >                                 (unsigned int __user *) (unsigned long) data);
> >                 break;
> >
> > +       case PTRACE_SET_SYSCALL:
> > +               /*
> > +                * This is currently only useful to cancel the syscall from a
> > +                * seccomp RET_TRACE tracer.
> > +                */
> > +               if ((long)data >= 0)
> > +                       return -EINVAL;
> > +               task_thread_info(child)->syscall = -1;
> > +               ret = 0;
> > +               break;
> > +
> >         case PTRACE_GET_THREAD_AREA_3264:
> >                 ret = put_user(task_thread_info(child)->tp_value,
> >                                 (unsigned long __user *) (unsigned long) data);
> > --
> > 2.13.2
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

Attachment: signature.asc
Description: Digital signature