Re: [PATCH 0/6] per signal_struct coredumps

From: Kees Cook <keescook@chromium.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH 0/6] per signal_struct coredumps
Date: Thu, 23 Sep 2021 22:59:44 -0700	[thread overview]
Message-ID: <202109232234.135BF1325@keescook> (raw)
In-Reply-To: <87v92qx2c6.fsf@disp2133>

On Thu, Sep 23, 2021 at 07:08:09PM -0500, Eric W. Biederman wrote:
> Current coredumps are mixed up with the exit code, the signal handling
> code and with the ptrace code in was they are much more complicated than
> necessary and difficult to follow.
> 
> This series of changes starts with ptrace_stop and cleans it up,
> making it easier to follow what is happening in ptrace_stop.
> Then cleans up the exec interactions with coredumps.
> Then cleans up the coredump interactions with exit.
> Then the coredump interactions with the signal handling code is clean
> up.
> 
> The first and last changes are bug fixes for minor bugs.

I haven't had a chance to carefully look through this yet, but I like
the sound of it. :)

Do we have any behavioral tests around this? The ptrace tests in seccomp
don't explicitly exercise the exit handling. Are there regression tests
for "rr"? They're usually the first to notice subtle changes in ptrace.

What I couldn't tell from my quick skim: does this further change the
behavior around force_sig_seccomp()? Specifically the "am I single
threaded?" check:

        case SECCOMP_RET_KILL_THREAD:
        case SECCOMP_RET_KILL_PROCESS:
        default:
                seccomp_log(this_syscall, SIGSYS, action, true);
                /* Dump core only if this is the last remaining thread. */
                if (action != SECCOMP_RET_KILL_THREAD ||
                    (atomic_read(&current->signal->live) == 1)) {
                        /* Show the original registers in the dump. */
                        syscall_rollback(current, current_pt_regs());
                        /* Trigger a coredump with SIGSYS */
                        force_sig_seccomp(this_syscall, data, true);
                } else {
                        do_exit(SIGSYS);
                }
                return -1; /* skip the syscall go directly to signal handling */

I *think* the answer is "no", in the sense that coredump_wait() is still
calling zap_threads() which calls zap_process(). Which now seem like
should have opposite names. :) And therefore inducing a coredump will
still take out all threads. (i.e. after your series, no changes need to
be made to seccomp for it.)

-Kees

-- 
Kees Cook