| Message ID | 1465506124-21866-7-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Jun 9, 2016 at 2:01 PM, Kees Cook <keescook@chromium.org> wrote: > This moves seccomp after ptrace on x86 to that seccomp can catch changes > made by ptrace. Emulation should skip the rest of processing too. > > We can get rid of test_thread_flag because there's no longer any > opportunity for seccomp to mess with ptrace state before invoking > ptrace. > > Suggested-by: Andy Lutomirski <luto@kernel.org> > Signed-off-by: Kees Cook <keescook@chromium.org> > Cc: x86@kernel.org > Cc: Andy Lutomirski <luto@kernel.org> > --- > arch/x86/entry/common.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c > index df56ca394877..81c0e12d831c 100644 > --- a/arch/x86/entry/common.c > +++ b/arch/x86/entry/common.c > @@ -73,6 +73,7 @@ static long syscall_trace_enter(struct pt_regs *regs) > > struct thread_info *ti = pt_regs_to_thread_info(regs); > unsigned long ret = 0; > + bool emulated = false; > u32 work; > > if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) > @@ -80,11 +81,19 @@ static long syscall_trace_enter(struct pt_regs *regs) > > work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; > > + if (unlikely(work & _TIF_SYSCALL_EMU)) > + emulated = true; > + > + if ((emulated || (work & _TIF_SYSCALL_TRACE)) && > + tracehook_report_syscall_entry(regs)) > + return -1L; > + > + if (emulated) > + return -1L; > + I think that this code will result in ptrace-induced skips calling the audit exit hook but not the audit entry hook. I don't know whether this is a problem. It's also worth making sure that ptracing a seccomp-skipped syscall calls the exit hook with the right regs. I suspect it's fine, but I want to think about it a little bit more. --Andy
On Thu, Jun 9, 2016 at 3:52 PM, Andy Lutomirski <luto@amacapital.net> wrote: > On Thu, Jun 9, 2016 at 2:01 PM, Kees Cook <keescook@chromium.org> wrote: >> This moves seccomp after ptrace on x86 to that seccomp can catch changes >> made by ptrace. Emulation should skip the rest of processing too. >> >> We can get rid of test_thread_flag because there's no longer any >> opportunity for seccomp to mess with ptrace state before invoking >> ptrace. >> >> Suggested-by: Andy Lutomirski <luto@kernel.org> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Cc: x86@kernel.org >> Cc: Andy Lutomirski <luto@kernel.org> >> --- >> arch/x86/entry/common.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c >> index df56ca394877..81c0e12d831c 100644 >> --- a/arch/x86/entry/common.c >> +++ b/arch/x86/entry/common.c >> @@ -73,6 +73,7 @@ static long syscall_trace_enter(struct pt_regs *regs) >> >> struct thread_info *ti = pt_regs_to_thread_info(regs); >> unsigned long ret = 0; >> + bool emulated = false; >> u32 work; >> >> if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) >> @@ -80,11 +81,19 @@ static long syscall_trace_enter(struct pt_regs *regs) >> >> work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; >> >> + if (unlikely(work & _TIF_SYSCALL_EMU)) >> + emulated = true; >> + >> + if ((emulated || (work & _TIF_SYSCALL_TRACE)) && >> + tracehook_report_syscall_entry(regs)) >> + return -1L; >> + >> + if (emulated) >> + return -1L; >> + > > I think that this code will result in ptrace-induced skips calling the > audit exit hook but not the audit entry hook. I don't know whether > this is a problem. It's also worth making sure that ptracing a > seccomp-skipped syscall calls the exit hook with the right regs. > > I suspect it's fine, but I want to think about it a little bit more. I don't think this is true, since all architectures already needed to handle an immediate return from seccomp, so audit shouldn't be touched on the exit path either. -Kees
On Thu, Jun 9, 2016 at 3:52 PM, Andy Lutomirski <luto@amacapital.net> wrote: > On Thu, Jun 9, 2016 at 2:01 PM, Kees Cook <keescook@chromium.org> wrote: >> This moves seccomp after ptrace on x86 to that seccomp can catch changes >> made by ptrace. Emulation should skip the rest of processing too. >> >> We can get rid of test_thread_flag because there's no longer any >> opportunity for seccomp to mess with ptrace state before invoking >> ptrace. >> >> Suggested-by: Andy Lutomirski <luto@kernel.org> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> Cc: x86@kernel.org >> Cc: Andy Lutomirski <luto@kernel.org> >> --- >> arch/x86/entry/common.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c >> index df56ca394877..81c0e12d831c 100644 >> --- a/arch/x86/entry/common.c >> +++ b/arch/x86/entry/common.c >> @@ -73,6 +73,7 @@ static long syscall_trace_enter(struct pt_regs *regs) >> >> struct thread_info *ti = pt_regs_to_thread_info(regs); >> unsigned long ret = 0; >> + bool emulated = false; >> u32 work; >> >> if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) >> @@ -80,11 +81,19 @@ static long syscall_trace_enter(struct pt_regs *regs) >> >> work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; >> >> + if (unlikely(work & _TIF_SYSCALL_EMU)) >> + emulated = true; >> + >> + if ((emulated || (work & _TIF_SYSCALL_TRACE)) && >> + tracehook_report_syscall_entry(regs)) >> + return -1L; >> + >> + if (emulated) >> + return -1L; >> + > > I think that this code will result in ptrace-induced skips calling the > audit exit hook but not the audit entry hook. I don't know whether > this is a problem. It's also worth making sure that ptracing a > seccomp-skipped syscall calls the exit hook with the right regs. > > I suspect it's fine, but I want to think about it a little bit more. I poked at it a bit and this seems to work correctly. selftests/x86/ptrace_syscall.c exercises PTRACE_SYSCALL_EMU pretty well, and it still passes. --Andy
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c index df56ca394877..81c0e12d831c 100644 --- a/arch/x86/entry/common.c +++ b/arch/x86/entry/common.c @@ -73,6 +73,7 @@ static long syscall_trace_enter(struct pt_regs *regs) struct thread_info *ti = pt_regs_to_thread_info(regs); unsigned long ret = 0; + bool emulated = false; u32 work; if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) @@ -80,11 +81,19 @@ static long syscall_trace_enter(struct pt_regs *regs) work = ACCESS_ONCE(ti->flags) & _TIF_WORK_SYSCALL_ENTRY; + if (unlikely(work & _TIF_SYSCALL_EMU)) + emulated = true; + + if ((emulated || (work & _TIF_SYSCALL_TRACE)) && + tracehook_report_syscall_entry(regs)) + return -1L; + + if (emulated) + return -1L; + #ifdef CONFIG_SECCOMP /* - * Do seccomp first -- it should minimize exposure of other - * code, and keeping seccomp fast is probably more valuable - * than the rest of this. + * Do seccomp after ptrace, to catch any tracer changes. */ if (work & _TIF_SECCOMP) { struct seccomp_data sd; @@ -117,13 +126,6 @@ static long syscall_trace_enter(struct pt_regs *regs) } #endif - if (unlikely(work & _TIF_SYSCALL_EMU)) - ret = -1L; - - if ((ret || test_thread_flag(TIF_SYSCALL_TRACE)) && - tracehook_report_syscall_entry(regs)) - ret = -1L; - if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT))) trace_sys_enter(regs, regs->orig_ax);
This moves seccomp after ptrace on x86 to that seccomp can catch changes made by ptrace. Emulation should skip the rest of processing too. We can get rid of test_thread_flag because there's no longer any opportunity for seccomp to mess with ptrace state before invoking ptrace. Suggested-by: Andy Lutomirski <luto@kernel.org> Signed-off-by: Kees Cook <keescook@chromium.org> Cc: x86@kernel.org Cc: Andy Lutomirski <luto@kernel.org> --- arch/x86/entry/common.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)