linux-kernel - Re: [PATCH] seccomp: suppress fatal signals that will never be delivered before seccomp forces an exit because of said signals

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jKb2Q5d6U1UPdQNK-RJQmVY6n=4NT2Rje+ajwPK3tr3vA@mail.gmail.com>
Date:	Wed, 10 Aug 2016 14:35:27 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Kyle Huey <me@...ehuey.com>, Oleg Nesterov <oleg@...hat.com>
Cc:	"Robert O'Callahan" <robert@...llahan.org>,
	Andy Lutomirski <luto@...capital.net>,
	Will Drewry <wad@...omium.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] seccomp: suppress fatal signals that will never be
 delivered before seccomp forces an exit because of said signals

On Wed, Aug 10, 2016 at 12:50 PM, Kyle Huey <me@...ehuey.com> wrote:
> This fixes rr. It doesn't quite fix the provided testcase, because the testcase fails to wait on the tracee after awakening from the nanosleep. Instead the testcase immediately does a PTHREAD_CONT, discarding the PTHREAD_EVENT_EXIT. The slightly modified testcase at https://gist.github.com/khuey/3c43ac247c72cef8c956c does pass.
>
> I don't see any obvious way to dequeue only the fatal signal, so instead I dequeue them all. Since none of these signals will ever be delivered it shouldn't affect the executing task.
>
> Suggested-by: Robert O'Callahan <robert@...llahan.org>
> Signed-off-by: Kyle Huey <khuey@...ehuey.com>

Oh excellent! Thank you for the patch; I've been chasing other things
this week and hadn't had time yet to dig into this. I agree with your
rationale: the signals aren't being delivered anyway, so drop them to
keep ptrace operating as expected.

Oleg, does this pass a quick sanity-check from you? For context, the
original problem description was:

The problem is that 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. Small-ish testcase here:
https://gist.github.com/rocallahan/1344f7d01183c233d08a2c6b93413068.

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.

Thanks!

-Kees

> ---
>  kernel/seccomp.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index ef6c6c3..728074d 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -609,8 +609,20 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>                  * Terminating the task now avoids executing a system
>                  * call that may not be intended.
>                  */
> -               if (fatal_signal_pending(current))
> +               if (fatal_signal_pending(current)) {
> +                       /*
> +                        * Swallow the signals we will never deliver.
> +                        * If we do not do this, the PTRACE_EVENT_EXIT will
> +                        * be suppressed by those signals.
> +                        */
> +                       siginfo_t info;
> +
> +                       spin_lock_irq(&current->sighand->siglock);
> +                       while (dequeue_signal(current, &current->blocked, &info));
> +                       spin_unlock_irq(&current->sighand->siglock);
> +
>                         do_exit(SIGSYS);
> +               }
>                 /* Check if the tracer forced the syscall to be skipped. */
>                 this_syscall = syscall_get_nr(current, task_pt_regs(current));
>                 if (this_syscall < 0)
> --
> 2.7.4
>

-- 
Kees Cook
Nexus Security