From: ebiederm@xmission.com (Eric W. Biederman)
To: Kees Cook <keescook@chromium.org>
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 6/6] coredump: Limit coredumps to a single thread group
Date: Wed, 06 Oct 2021 12:03:59 -0500 [thread overview]
Message-ID: <87r1cynkzk.fsf@disp2133> (raw)
In-Reply-To: <202109241154.A915C488E2@keescook> (Kees Cook's message of "Fri, 24 Sep 2021 11:56:00 -0700")
[-- Attachment #1: Type: text/plain, Size: 2688 bytes --]
Kees Cook <keescook@chromium.org> writes:
> On Thu, Sep 23, 2021 at 07:12:33PM -0500, Eric W. Biederman wrote:
>>
>> Today when a signal is delivered with a handler of SIG_DFL whose
>> default behavior is to generate a core dump not only that process but
>> every process that shares the mm is killed.
>>
>> In the case of vfork this looks like a real world problem. Consider
>> the following well defined sequence.
>>
>> if (vfork() == 0) {
>> execve(...);
>> _exit(EXIT_FAILURE);
>> }
>>
>> If a signal that generates a core dump is received after vfork but
>> before the execve changes the mm the process that called vfork will
>> also be killed (as the mm is shared).
>>
>> Similarly if the execve fails after the point of no return the kernel
>> delivers SIGSEGV which will kill both the exec'ing process and because
>> the mm is shared the process that called vfork as well.
>>
>> As far as I can tell this behavior is a violation of people's
>> reasonable expectations, POSIX, and is unnecessarily fragile when the
>> system is low on memory.
>>
>> Solve this by making a userspace visible change to only kill a single
>> process/thread group. This is possible because Jann Horn recently
>> modified[1] the coredump code so that the mm can safely be modified
>> while the coredump is happening. With LinuxThreads long gone I don't
>> expect anyone to have a notice this behavior change in practice.
>>
>> To accomplish this move the core_state pointer from mm_struct to
>> signal_struct, which allows different thread groups to coredump
>> simultatenously.
>>
>> In zap_threads remove the work to kill anything except for the current
>> thread group.
>>
>> [1] a07279c9a8cd ("binfmt_elf, binfmt_elf_fdpic: use a VMA list snapshot")
>> Fixes: d89f3847def4 ("[PATCH] thread-aware coredumps, 2.5.43-C3")
>> History-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> This looks correct to me, but depends on the 5/6 not introducing any
> races. So, to that end:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> If you have some local tools you've been using for testing this series,
> can you toss them into tools/testing/selftests/ptrace/ ? I can help
> clean them up if want.
I just have a program that goes multi-thread and calls abort, and a
slight variant of it that calls vfork before calling abort.
It is enough to exercise the code and verify I didn't make any typos.
I have attached the code below. If you can help make it into a proper
test that would be great. I have just been manually running gdb
and the like to verify the kernel works as expected.
Eric
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: threaded-coredump.c --]
[-- Type: text/x-csrc, Size: 1346 bytes --]
#include <stdio.h>
#include <pthread.h>
#include <sys/ptrace.h>
#include <sys/types.h>
#include <signal.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>
struct params {
int argc;
char **argv;
char **envp;
pthread_t parent;
pthread_t sibling[30];
};
static void *dump_thread(void *arg)
{
struct params *params = arg;
void *retval;
int i;
pthread_join(params->parent, &retval);
fprintf(stdout, "Waiting for 5s\n");
sleep(5);
fprintf(stdout, "Dumping\n");
abort();
fprintf(stdout, "Abort Failed: %d %s\n", errno, strerror(errno));
for (i = 0; i <= 29; i++) {
pthread_join(params->sibling[i], &retval);
}
fprintf(stdout, "All Done!\n");
_exit(EXIT_FAILURE);
return NULL;
}
static void *idle_thread(void *arg)
{
unsigned long i = (unsigned long)arg;
sleep(10);
fprintf(stdout, "Done %lu\n", i);
fflush(stdout);
return NULL;
}
int main(int argc, char **argv, char **envp)
{
struct params *params;
pthread_t pt;
unsigned long i;
params = malloc(sizeof(struct params));
params->argc = argc - 1;
params->argv = argv = argv + 1;
params->envp = envp;
params->parent = pthread_self();
pthread_create(&pt, NULL, dump_thread, params);
for (i = 0; i <= 29; i++)
pthread_create(¶ms->sibling[i], NULL, idle_thread, (void *)i);
pthread_exit(NULL);
return 0;
}
next prev parent reply other threads:[~2021-10-06 17:04 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-24 0:08 [PATCH 0/6] per signal_struct coredumps Eric W. Biederman
2021-09-24 0:09 ` [PATCH 1/6] signal: Remove the bogus sigkill_pending in ptrace_stop Eric W. Biederman
2021-09-24 15:22 ` Kees Cook
2021-09-24 15:48 ` Eric W. Biederman
2021-09-24 19:06 ` Kees Cook
2021-09-24 0:10 ` [PATCH 2/6] ptrace: Remove the unnecessary arguments from arch_ptrace_stop Eric W. Biederman
2021-09-24 15:26 ` Kees Cook
2021-09-24 0:10 ` [PATCH 3/6] exec: Check for a pending fatal signal instead of core_state Eric W. Biederman
2021-09-24 15:38 ` Kees Cook
2021-09-24 0:11 ` [PATCH 4/6] exit: Factor coredump_exit_mm out of exit_mm Eric W. Biederman
2021-09-24 18:28 ` Kees Cook
2021-09-24 0:11 ` [PATCH 5/6] coredump: Don't perform any cleanups before dumping core Eric W. Biederman
2021-09-24 18:51 ` Kees Cook
2021-09-24 21:28 ` Eric W. Biederman
2021-09-24 21:41 ` Kees Cook
2021-09-24 0:12 ` [PATCH 6/6] coredump: Limit coredumps to a single thread group Eric W. Biederman
2021-09-24 18:56 ` Kees Cook
2021-10-06 17:03 ` Eric W. Biederman [this message]
2021-11-19 16:03 ` Kyle Huey
2021-11-19 17:38 ` Eric W. Biederman
2021-09-24 5:59 ` [PATCH 0/6] per signal_struct coredumps Kees Cook
2021-09-24 14:00 ` Eric W. Biederman
2021-09-24 15:22 ` [PATCH 2/6] ptrace: Remove the unnecessary arguments from arch_ptrace_stop Eric W. Biederman
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=87r1cynkzk.fsf@disp2133 \
--to=ebiederm@xmission.com \
--cc=keescook@chromium.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).