From: Oleg Nesterov <oleg@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org, Kyle Huey <khuey@kylehuey.com>,
"Robert O'Callahan" <robert@ocallahan.org>,
Andy Lutomirski <luto@amacapital.net>,
Will Drewry <wad@chromium.org>
Subject: Re: [PATCH] seccomp: Fix tracer exit notifications during fatal signals
Date: Thu, 11 Aug 2016 17:12:51 +0200 [thread overview]
Message-ID: <20160811151250.GA21519@redhat.com> (raw)
In-Reply-To: <20160810233725.GA23704@www.outflux.net>
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>
next prev parent reply other threads:[~2016-08-11 15:13 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-10 23:37 [PATCH] seccomp: Fix tracer exit notifications during fatal signals Kees Cook
2016-08-11 0:16 ` Kyle Huey
2016-08-11 7:27 ` Andy Lutomirski
2016-08-11 18:06 ` Kees Cook
2016-08-11 15:12 ` Oleg Nesterov [this message]
2016-08-11 18:18 ` Kees Cook
2016-08-23 3:27 ` Kyle Huey
2016-08-23 19:41 ` Kees Cook
2016-08-12 3:21 ` Robert O'Callahan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160811151250.GA21519@redhat.com \
--to=oleg@redhat.com \
--cc=keescook@chromium.org \
--cc=khuey@kylehuey.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=robert@ocallahan.org \
--cc=wad@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.