On Mon, Aug 31, 2020 at 12:37 PM Kees Cook <keesc...@chromium.org> wrote: > > On Fri, Aug 28, 2020 at 09:56:13PM -0400, Rich Felker wrote: > > Asynchronous termination of a thread outside of the userspace thread > > library's knowledge is an unsafe operation that leaves the process in > > an inconsistent, corrupt, and possibly unrecoverable state. In order > > to make new actions that may be added in the future safe on kernels > > not aware of them, change the default action from > > SECCOMP_RET_KILL_THREAD to SECCOMP_RET_KILL_PROCESS. > > > > Signed-off-by: Rich Felker <dal...@libc.org> > > --- > > > > This fundamental problem with SECCOMP_RET_KILL_THREAD, and that it > > should be considered unsafe and deprecated, was recently noted/fixed > > seccomp in the man page and its example. Here I've only changed the > > default action for new/unknown action codes. Ideally the behavior for > > strict seccomp mode would be changed too but I think that breaks > > stability policy; in any case it's less likely to be an issue since > > strict mode is hard or impossible to use reasonably in a multithreaded > > process. > > > > Unfortunately changing this now won't help older kernels where unknown > > new actions would still be handled unsafely, but at least it makes it > > so the problem will fade away over time. > > I think this is probably fine to change now. I'd always wanted to > "upgrade" the default to KILL_PROCESS, but wanted to wait for > KILL_PROCESS to exist at all for a while first. :) > > I'm not aware of any filter generators (e.g. libseccomp, Chrome) that > depend on unknown filter return values to cause a KILL_THREAD, and > everything I've seen indicates that they aren't _accidentally_ depending > on it either (i.e. they both produce "valid" filters). It's possible > that something out there doesn't, and in that case, we likely need to > make a special case for whatever bad filter value it chose, but we can > cross that bridge when we come to it. > > I've added Kyle and Robert to CC as well, as they have noticed subtle > changes to seccomp behavior in the past. I *think* this change should be > fine, but perhaps they will see something I don't. :)
I can't think of anything here that would break stuff, though I do believe rr needs some changes to how it handles this (I don't think our current behavior is an accurate emulation of the kernel). - Kyle > > > > kernel/seccomp.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index d653d8426de9..ce1875fa6b39 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -910,10 +910,10 @@ static int __seccomp_filter(int this_syscall, const > > struct seccomp_data *sd, > > seccomp_init_siginfo(&info, this_syscall, data); > > do_coredump(&info); > > } > > - if (action == SECCOMP_RET_KILL_PROCESS) > > - do_group_exit(SIGSYS); > > - else > > + if (action == SECCOMP_RET_KILL_THREAD) > > do_exit(SIGSYS); > > + else > > + do_group_exit(SIGSYS); > > I need to think a little more, but I suspect we should change the coredump > logic (above the quoted code) too... (i.e. "action == > SECCOMP_RET_KILL_PROCESS" > -> "action != SECCOMP_RET_KILL_THREAD") > > > } > > > > unreachable(); > > -- > > 2.21.0 > > > > Thanks! > > -Kees > > -- > Kees Cook
