Jann Horn jannh@google.com writes:
On Thu, Feb 10, 2022 at 6:37 PM Kees Cook keescook@chromium.org wrote:
On Thu, Feb 10, 2022 at 05:18:39PM +0100, Jann Horn wrote:
On Thu, Feb 10, 2022 at 3:53 AM Kees Cook keescook@chromium.org wrote:
Fatal SIGSYS signals were not being delivered to pid namespace init processes. Make sure the SIGNAL_UNKILLABLE doesn't get set for these cases.
Reported-by: Robert Święcki robert@swiecki.net Suggested-by: "Eric W. Biederman" ebiederm@xmission.com Fixes: 00b06da29cf9 ("signal: Add SA_IMMUTABLE to ensure forced siganls do not get changed") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org
kernel/signal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c index 38602738866e..33e3ee4f3383 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1342,9 +1342,10 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, } /* * Don't clear SIGNAL_UNKILLABLE for traced tasks, users won't expect
* debugging to leave init killable.
* debugging to leave init killable, unless it is intended to exit. */
if (action->sa.sa_handler == SIG_DFL && !t->ptrace)
if (action->sa.sa_handler == SIG_DFL &&(!t->ptrace || (handler == HANDLER_EXIT))) t->signal->flags &= ~SIGNAL_UNKILLABLE;You're changing the subclause:
!t->ptrace
to:
(!t->ptrace || (handler == HANDLER_EXIT))
which means that the change only affects cases where the process has a ptracer, right? That's not the scenario the commit message is talking about...
Sorry, yes, I was not as accurate as I should have been in the commit log. I have changed it to:
Fatal SIGSYS signals (i.e. seccomp RET_KILL_* syscall filter actions) were not being delivered to ptraced pid namespace init processes. Make sure the SIGNAL_UNKILLABLE doesn't get set for these cases.
So basically force_sig_info() is trying to figure out whether get_signal() will later on check for SIGNAL_UNKILLABLE (the SIG_DFL case), and if so, it clears the flag from the target's signal_struct that marks the process as unkillable?
This used to be:
if (action->sa.sa_handler == SIG_DFL) t->signal->flags &= ~SIGNAL_UNKILLABLE;
Then someone noticed that in the ptrace case, the signal might not actually end up being consumed by the target process, and added the "&& !t->ptrace" clause in commit eb61b5911bdc923875cde99eb25203a0e2b06d43.
And now Robert Swiecki noticed that that still didn't accurately model what'll happen in get_signal().
This seems hacky to me, and also racy: What if, while you're going through a SECCOMP_RET_KILL_PROCESS in an unkillable process, some other thread e.g. concurrently changes the disposition of SIGSYS from a custom handler to SIG_DFL?
Instead of trying to figure out whether the signal would have been fatal without SIGNAL_UNKILLABLE, I think it would be better to find a way to tell the signal-handling code that SIGNAL_UNKILLABLE should be bypassed for this specific signal, or something along those lines... but of course that's also kind of messy because the signal-sending code might fall back to just using the pending signal mask on allocation failure IIRC?
I am actively working on this. I think I know how to get there but it requires cleanups elsewhere as well, so it is not really an approach that is appropriate for backporting.
The big bottleneck is that we need to make signals that trigger coredumps eligible for short circuit delivery, and that takes a little doing.
Eric