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

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

[Kees Cook]

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@ocallahan.org>
Reported-by: Kyle Huey <khuey@kylehuey.com>
Fixes: 93e35efb8de4 ("x86/ptrace: run seccomp after ptrace")
Signed-off-by: Kees Cook <keescook@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.
 		 */
 		if (fatal_signal_pending(current))
-			do_exit(SIGSYS);
+			goto skip;
 		/* 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

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

[Kyle Huey]

On Wed, Aug 10, 2016 at 4:37 PM, Kees Cook <keescook@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 <robert@ocallahan.org>
> Reported-by: Kyle Huey <khuey@kylehuey.com>
> Fixes: 93e35efb8de4 ("x86/ptrace: run seccomp after ptrace")
> Signed-off-by: Kees Cook <keescook@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.
>                  */
>                 if (fatal_signal_pending(current))
> -                       do_exit(SIGSYS);
> +                       goto skip;
>                 /* 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

This patch also fixes rr as well.

- Kyle

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

[Andy Lutomirski]

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

Otherwise looks good to me.

--Andy

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

[Oleg Nesterov]

On 08/10, Kees Cook 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.

ACK.

I think this change is fine in any case, but...

> 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

I _never_ understood what PTRACE_EVENT_EXIT should actually do. I mean,
when it should actually stop. This was never defined.

> 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.

And this can happen anyway, with or without this change, with or without
seccomp. Because another fatal signal can be pending. So PTRACE_EVENT_EXIT
actually depends on /dev/random.

Perhaps we should finally define what it should do. Say, it should only
stop if SIGKILL was sent "implicitely" by exit/exec. But as for exec,
there are more (off-topic) complications, not sure we actually want this...

Nevermind, the main problem is that _any_ change in this area can break
something. This code is sooooooo old.

But let me repeat, I think this change is fine anyway.

Acked-by: Oleg Nesterov <oleg@redhat.com>

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

[Kees Cook]

On Thu, Aug 11, 2016 at 12:27 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Aug 10, 2016 at 4:37 PM, Kees Cook <keescook@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 <robert@ocallahan.org>
>> Reported-by: Kyle Huey <khuey@kylehuey.com>
>> Fixes: 93e35efb8de4 ("x86/ptrace: run seccomp after ptrace")
>> Signed-off-by: Kees Cook <keescook@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

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

[Kees Cook]

On Thu, Aug 11, 2016 at 8:12 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/10, Kees Cook 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.
>
> ACK.
>
> I think this change is fine in any case, but...
>
>> 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
>
> I _never_ understood what PTRACE_EVENT_EXIT should actually do. I mean,
> when it should actually stop. This was never defined.

Yeah, agreed. I spent some time reading through what should happen to
__TASK_TRACED during exit and my head spun. :)

>> 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.
>
> And this can happen anyway, with or without this change, with or without
> seccomp. Because another fatal signal can be pending. So PTRACE_EVENT_EXIT
> actually depends on /dev/random.
>
> Perhaps we should finally define what it should do. Say, it should only
> stop if SIGKILL was sent "implicitely" by exit/exec. But as for exec,
> there are more (off-topic) complications, not sure we actually want this...
>
> Nevermind, the main problem is that _any_ change in this area can break
> something. This code is sooooooo old.
>
> But let me repeat, I think this change is fine anyway.
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>

Awesome, thanks!

-Kees

-- 
Kees Cook
Nexus Security

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

[Robert O'Callahan]

Thanks!

On Fri, Aug 12, 2016 at 3:12 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > 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
>
> I _never_ understood what PTRACE_EVENT_EXIT should actually do. I mean,
> when it should actually stop. This was never defined.
>
> > 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.
>
> And this can happen anyway, with or without this change, with or without
> seccomp. Because another fatal signal can be pending. So PTRACE_EVENT_EXIT
> actually depends on /dev/random.

True. But at least (as Kees alluded to later) this patch ensures
PTRACE_EVENT_EXIT delivery when exit is due to exit_group() and no
genuine fatal signals are involved.

> Perhaps we should finally define what it should do. Say, it should only
> stop if SIGKILL was sent "implicitely" by exit/exec. But as for exec,
> there are more (off-topic) complications, not sure we actually want this...

The ptrace man page currently says:
> A SIGKILL signal may still cause a PTRACE_EVENT_EXIT stop before actual signal death.  This may be changed in the future; SIGKILL is meant to always immediately kill tasks even under ptrace.  Last confirmed on Linux 3.13.

In practice, a PTRACE_EVENT_EXIT is almost always observed after
SIGKILL. That's nice for rr, because it lets us observe the process's
final state. But it allows a process to stay alive indefinitely after
receiving SIGKILL, so I can see why you might want to change it.

Rob
-- 
lbir ye,ea yer.tnietoehr  rdn rdsme,anea lurpr  edna e hnysnenh hhe uresyf toD
selthor  stor  edna  siewaoeodm  or v sstvr  esBa  kbvted,t rdsme,aoreseoouoto
o l euetiuruewFa  kbn e hnystoivateweh uresyf tulsa rehr  rdm  or rnea lurpr
.a war hsrer holsa rodvted,t  nenh hneireseoouot.tniesiewaoeivatewt sstvr  esn

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

[Kyle Huey]

On Thu, Aug 11, 2016 at 11:18 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Aug 11, 2016 at 8:12 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 08/10, Kees Cook 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.
>>
>> ACK.
>>
>> I think this change is fine in any case, but...
>>
>>> 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
>>
>> I _never_ understood what PTRACE_EVENT_EXIT should actually do. I mean,
>> when it should actually stop. This was never defined.
>
> Yeah, agreed. I spent some time reading through what should happen to
> __TASK_TRACED during exit and my head spun. :)
>
>>> 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.
>>
>> And this can happen anyway, with or without this change, with or without
>> seccomp. Because another fatal signal can be pending. So PTRACE_EVENT_EXIT
>> actually depends on /dev/random.
>>
>> Perhaps we should finally define what it should do. Say, it should only
>> stop if SIGKILL was sent "implicitely" by exit/exec. But as for exec,
>> there are more (off-topic) complications, not sure we actually want this...
>>
>> Nevermind, the main problem is that _any_ change in this area can break
>> something. This code is sooooooo old.
>>
>> But let me repeat, I think this change is fine anyway.
>>
>> Acked-by: Oleg Nesterov <oleg@redhat.com>
>
> Awesome, thanks!

Hi folks,

Can't help but notice this didn't make it into rc3.  Not sure if it's
bubbling up somewhere I can't see, but we'd really like this to get
into 4.8 so we don't have to work around the regression.

Thanks!

- Kyle

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

[Kees Cook]

On Mon, Aug 22, 2016 at 8:27 PM, Kyle Huey <me@kylehuey.com> wrote:
> On Thu, Aug 11, 2016 at 11:18 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Aug 11, 2016 at 8:12 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 08/10, Kees Cook 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.
>>>
>>> ACK.
>>>
>>> I think this change is fine in any case, but...
>>>
>>>> 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
>>>
>>> I _never_ understood what PTRACE_EVENT_EXIT should actually do. I mean,
>>> when it should actually stop. This was never defined.
>>
>> Yeah, agreed. I spent some time reading through what should happen to
>> __TASK_TRACED during exit and my head spun. :)
>>
>>>> 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.
>>>
>>> And this can happen anyway, with or without this change, with or without
>>> seccomp. Because another fatal signal can be pending. So PTRACE_EVENT_EXIT
>>> actually depends on /dev/random.
>>>
>>> Perhaps we should finally define what it should do. Say, it should only
>>> stop if SIGKILL was sent "implicitely" by exit/exec. But as for exec,
>>> there are more (off-topic) complications, not sure we actually want this...
>>>
>>> Nevermind, the main problem is that _any_ change in this area can break
>>> something. This code is sooooooo old.
>>>
>>> But let me repeat, I think this change is fine anyway.
>>>
>>> Acked-by: Oleg Nesterov <oleg@redhat.com>
>>
>> Awesome, thanks!
>
> Hi folks,
>
> Can't help but notice this didn't make it into rc3.  Not sure if it's
> bubbling up somewhere I can't see, but we'd really like this to get
> into 4.8 so we don't have to work around the regression.
>
> Thanks!

Eek, thanks for the ping. I got distracted by some other bugs fixes to
get landed, but I've sent the seccomp pull request to James now. It
should show up for -rc4.

Thanks!

-Kees

-- 
Kees Cook
Nexus Security