On Fri, Jun 17, 2016 at 11:55 AM, Andy Lutomirski <l...@amacapital.net> wrote: > On Fri, Jun 17, 2016 at 12:15 AM, James Morris <jmor...@namei.org> wrote: >> On Tue, 14 Jun 2016, Kees Cook wrote: >> >>> Hi, >>> >>> Please pull these seccomp changes for next. These have been tested by >>> myself and Andy, and close a long-standing issue with seccomp where tracers >>> could change the syscall out from under seccomp. >> >> Pulled to security -next. > > As a heads up: I think this doesn't quite close the hole on x86. Consider: > > 64-bit task arranges to be traced by a 32-bit task (or presumably a > 64-bit task that calls ptrace via int80). > > Tracer does PTRACE_SYSCALL. > > Tracee does a normal syscall. > > Tracer writes tracee's orig_ax, thus invoking this thing in > arch/x86/kernel/ptrace.c: > > if (syscall_get_nr(child, regs) >= 0) > child->thread.status |= TS_COMPAT; > > Tracer resumes and gets confused. > > I think the right fix is to just delete: > > if (syscall_get_nr(child, regs) >= 0) > child->thread.status |= TS_COMPAT; > > from ptrace.c. The comment above it is garbage, too.
I'm perfectly happy to see it removed. I can't make sense of the comment. :) That said, the only confusion I see is pretty minor. The arch is saved before the tracer could force TS_COMPAT, so nothing confused is handed to seccomp (the first time). And the syscall will continue to be looked up on sys_call_table not ia32_sys_call_table. The only thing I see is if the tracer has also added a SECCOMP_RET_TRACE filter, after which the recheck will reload all the seccomp info, including the arch. And at this point, a sensible filter will reject a non-matching architecture. Maybe I'm missing something more? -Kees -- Kees Cook Chrome OS & Brillo Security