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

[Kees Cook]

On Sat, Jun 18, 2016 at 3:21 AM, Andy Lutomirski <luto@xxxxxxxxxx> 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
> also don't see how one would reasonably have triggered it in the
> first place.  In particular, syscall restart happens before ptrace
> hooks in the syscall exit path, so a tracer should never see the
> problematic pre-restart syscall state in the first place.
>
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
> ---
>
> Oleg, you often have good insight into ptrace oddities.  Do you think I'm
> correct that this can safely be deleted?
>
> Kees, I think that either this or a similar fix is important to make the
> seccomp reordering series be fully effective.

For good measure, just to repeat what I said in the other thread:
yeah, this TS_COMPAT injection needs to be fixed, though I don't view
it as critical for seccomp since the major bypass situation will be
fixed already. The TS_COMPAT interaction isn't bad (since the arch
state is saved before the seccomp calls), but it can confuse a filter
intentionally trying to hit this bug via RET_SECCOMP_TRACE, but then
it would only be a problem for a filter that was either ignoring arch
tests or doing biarch filtering, which is extremely uncommon.

-Kees

>
> Ingo, this is intended for x86/urgent.  I deliberately didn't cc stable,
> as the bug this fixes seems minor enough that I think we can wait a while
> to backport it.
>
> arch/x86/kernel/ptrace.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 600edd225e81..bde57caf9aa9 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned regno, u32 value)
>         R32(esp, sp);
>
>         case offsetof(struct user32, regs.orig_eax):
> -               /*
> -                * A 32-bit debugger setting orig_eax means to restore
> -                * the state of the task restarting a 32-bit syscall.
> -                * Make sure we interpret the -ERESTART* codes correctly
> -                * in case the task is not actually still sitting at the
> -                * exit from a 32-bit syscall with TS_COMPAT still set.
> -                */
>                 regs->orig_ax = value;
> -               if (syscall_get_nr(child, regs) >= 0)
> -                       task_thread_info(child)->status |= TS_COMPAT;
>                 break;
>
>         case offsetof(struct user32, regs.eflags):
> --
> 2.7.4
>



-- 
Kees Cook
Chrome OS & Brillo Security