Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Jun 18, 2016 6:56 AM, "Pedro Alves" <pedro@xxxxxxxxxx> wrote:
>
> On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
> > A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
> >
> > - If the tracee is stopped in a 32-bit syscall, this has no effect
> >   as TS_COMPAT will already be set.
> >
> > - If the tracee is stopped on entry to a 64-bit syscall, this could
> >   cause problems: in_compat_syscall() etc will be out of sync with
> >   the actual syscall table in use.  I can imagine this bre aking
> >   audit.  (It can't meaningfully break seccomp, as a malicious
> >   tracer can already fully bypass seccomp.)  I could also imagine it
> >   subtly confusing the implementations of a few syscalls.
> >
> >  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
> >    will end up set in user mode, which isn't supposed to happen.  The results
> >    are likely to be similar to the 64-bit syscall entry case.
> >
> > Fix it by deleting the code.
> >
> > Here's my understanding of the previous intent.  Suppose a
> > 32-bit tracee makes a syscall that returns one of the -ERESTART
> > codes.  A 32-bit tracer saves away its register state.  The tracee
> > resumes, returns from the system call, and gets stopped again for a
> > non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> > back the tracee's state by writing all of the saved registers back.
> >
> > The result of this sequence of events will be that the tracee's
> > registers' low bits match what they were at the end of the syscall
> > but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> > return a *positive* number, because we zero-extend registers poked
> > by 32-bit tracers instead of sign-extending them.  As a result, with
> > this change, syscall restart won't happen, whereas, historically, it
> > would have happened.
> >
> > As far as I can tell, this corner case isn't very important, and I
>
> I believe it's actually very much very important for gdb, for restoring
> the inferior state when the user calls a function in the inferior, with:
>
>  (gdb) print foo()
>
> Some background here:
>
>  http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0

Yuck.  I should have dug in to the history.  Why not just
unconditionally sign-extend eax when set by a 32-bit tracer?

Do you know how to acquire a copy of erestartsys-trap.c?  The old
links appear to be broken.

Also, while I have your attention: when gdb restores old state like
this, does it do it with individual calls to PTRACE_POKEUSER or does
it use SETREGSET or similar to do it all at once?  I'm asking because
I have some other code (fsgsbase) that's on hold until I can figure
out how to keep it from breaking gdb if and when gdb writes to fs and
fs_base.