On Jun 18, 2016 12:02 AM, "Kees Cook" <keesc...@chromium.org> wrote: > > 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.
Hmm, right, but... > > 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. > Yes for some filters, but others might have different logic for different arches, at which point there's a minor bypass. > Maybe I'm missing something more? > You can also use this to do a 64-bit syscall with in_compat_syscall() returning true, which could cause issues for audit and maybe some ioctl handlers irrespective of this patch series. I'll see about getting it fixed in x86/urgent.
