On Thu, Aug 11, 2016 at 12:27 AM, Andy Lutomirski <l...@amacapital.net> wrote: > On Wed, Aug 10, 2016 at 4:37 PM, Kees Cook <keesc...@chromium.org> wrote: >> This fixes a ptrace vs fatal pending signals bug as manifested in seccomp >> now that ptrace was reordered to happen after ptrace. The short version is >> that seccomp should not attempt to call do_exit() while fatal signals are >> pending under a tracer. This was needlessly paranoid. Instead, the syscall >> can just be skipped and normal signal handling, tracer notification, and >> process death can happen. >> >> Slightly edited original bug report: >> >> If a tracee task is in a PTRACE_EVENT_SECCOMP trap, or has been resumed >> after such a trap but not yet been scheduled, and another task in the >> thread-group calls exit_group(), then the tracee task exits without the >> ptracer receiving a PTRACE_EVENT_EXIT notification. Test case here: >> https://gist.github.com/khuey/3c43ac247c72cef8c956ca73281c9be7 >> >> The bug happens because when __seccomp_filter() detects >> fatal_signal_pending(), it calls do_exit() without dequeuing the fatal >> signal. When do_exit() sends the PTRACE_EVENT_EXIT notification and >> that task is descheduled, __schedule() notices that there is a fatal >> signal pending and changes its state from TASK_TRACED to TASK_RUNNING. >> That prevents the ptracer's waitpid() from returning the ptrace event. >> A more detailed analysis is here: >> https://github.com/mozilla/rr/issues/1762#issuecomment-237396255. >> >> Reported-by: Robert O'Callahan <rob...@ocallahan.org> >> Reported-by: Kyle Huey <kh...@kylehuey.com> >> Fixes: 93e35efb8de4 ("x86/ptrace: run seccomp after ptrace") >> Signed-off-by: Kees Cook <keesc...@chromium.org> >> --- >> kernel/seccomp.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> index ef6c6c3f9d8a..0db7c8a2afe2 100644 >> --- a/kernel/seccomp.c >> +++ b/kernel/seccomp.c >> @@ -605,12 +605,16 @@ static int __seccomp_filter(int this_syscall, const >> struct seccomp_data *sd, >> ptrace_event(PTRACE_EVENT_SECCOMP, data); >> /* >> * The delivery of a fatal signal during event >> - * notification may silently skip tracer notification. >> - * Terminating the task now avoids executing a system >> - * call that may not be intended. >> + * notification may silently skip tracer notification, >> + * which could leave us with a potentially unmodified >> + * syscall that the tracer would have liked to have >> + * changed. Since the process is about to die, we just >> + * force the syscall to be skipped and let the signal >> + * kill the process and correctly handle any tracer exit >> + * notifications. >> */ > > What does "The delivery of a fatal signal during event notification > may silently skip tracer notification" mean? Is that describing > exactly the issue you're fixing? If so, maybe that sentence should be > deleted.
Well, it's related. The fatal signal delivery may skip notifications, but we could deterministically get into the case of skipping PTRACE_EVENT_EXIT due to the do_exit(), which is much more confusing. So changing this restores the pre-existing (hard to hit) race. > Otherwise looks good to me. Thanks! -Kees -- Kees Cook Nexus Security
