Re: [PATCH] seccomp: Fix tracer exit notifications during fatal signals

[Andy Lutomirski]

On Wed, Aug 10, 2016 at 4:37 PM, Kees Cook <keescook@xxxxxxxxxxxx> 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 <robert@xxxxxxxxxxxxx>
> Reported-by: Kyle Huey <khuey@xxxxxxxxxxxx>
> Fixes: 93e35efb8de4 ("x86/ptrace: run seccomp after ptrace")
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  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.

Otherwise looks good to me.

--Andy