On Sat, Jun 18, 2016 at 3:21 AM, Andy Lutomirski <l...@kernel.org> 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 <l...@kernel.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.
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
