Re: [PATCH 0/6] per signal_struct coredumps

[Eric W. Biederman]

Kees Cook <keescook@xxxxxxxxxxxx> writes:

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

Please do most of the changes are quite small and straight forward.
Which is why this is several patches to make catching bugs and bisecting
easier.

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

There are no tests that I am aware of.  I am hoping to get this into
linux-next so more people will test just because.

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

There are two changes I can think of with this patchset.
- Tasks killed in the coredump signal path are those that share
  signal_struct, and the coredump will only include those tasks.
- No tasks will reach PTRACE_EVENT_EXIT during a coredump.
  Which actually makes PTRACE_EVENT_EXIT and coredumps more reliable.
  As there is no concern about waiting for each other.


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

Correct.  Seccomp can stay the same.

What changes in practice is that now SECCOMP_RET_KILL_PROCESS only kills
the process and not other processes that share the mm.


I can imagine a future where the seccomp logic to ask if this is the
final thread will move into signal delivery.  That takes some more
exit cleanups.

In fact once we are convinced the final change in the series is correct.
There are a lot of simplifications to the code that are possible.

One I am hoping for is to move the killing up into complete_signal and
then modifying force_sig_info_to_task to need to stomp sa_handler and
SIGNAL_UNKILLABLE but can instead call the relevant parts of send_signal
by hand, like send_sigqueue does.


Eric