Message-Id: <94bda8cd5f326ae5591c80fb5d7c1c22624accec.1466244711.git.luto@kernel.org>
Date:	Sat, 18 Jun 2016 03:21:40 -0700
From:	Andy Lutomirski <luto@...nel.org>
To:	x86@...nel.org
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Kees Cook <keescook@...omium.org>,
	Borislav Petkov <bp@...en8.de>,
	Oleg Nesterov <oleg@...hat.com>,
	Andy Lutomirski <luto@...nel.org>
Subject: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

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@...nel.org>
---

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.

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

Powered by blists - more mailing lists