lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 22 Dec 2012 12:16:22 -0800 From: ebiederm@...ssion.com (Eric W. Biederman) To: Rob Landley <rob@...dley.net> Cc: Oleg Nesterov <oleg@...hat.com>, Linux Containers <containers@...ts.linux-foundation.org>, linux-kernel@...r.kernel.org Subject: Re: [PATCH review 1/3] pidns: Outlaw thread creation after unshare(CLONE_NEWPID) Rob Landley <rob@...dley.net> writes: > On 12/21/2012 10:57:34 PM, Eric W. Biederman wrote: >> >> The sequence: >> unshare(CLONE_NEWPID) >> clone(CLONE_THREAD|CLONE_SIGHAND|CLONE_VM) >> >> Creates a new process in the new pid namespace without setting >> pid_ns->child_reaper. After forking this results in a NULL >> pointer dereference. >> >> Avoid this and other nonsense scenarios that can show up after >> creating a new pid namespace with unshare by adding a new >> check in copy_prodcess. >> >> Pointed-out-by: Oleg Nesterov <oleg@...hat.com> >> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com> >> --- >> kernel/fork.c | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/kernel/fork.c b/kernel/fork.c >> index a31b823..65ca6d2 100644 >> --- a/kernel/fork.c >> +++ b/kernel/fork.c >> @@ -1166,6 +1166,14 @@ static struct task_struct >> *copy_process(unsigned long clone_flags, >> current->signal->flags & >> SIGNAL_UNKILLABLE) >> return ERR_PTR(-EINVAL); >> >> + /* >> + * If the new process will be in a different pid namespace >> + * don't allow the creation of threads. >> + */ >> + if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) && >> + (task_active_pid_ns(current) != current->nsproxy->pid_ns)) >> + return ERR_PTR(-EINVAL); >> + > > Since the first bit will trigger if clone_flags has just CLONE_VM > without CLONE_NEWPID, or vice versa, I'm guessing this is a fast path > optimization? (Otherwise you meant (clone_flags & > (CLONE_VM|CLONE_NEWPID)) == CLONE_VM|CLONE_NEWPID ?) > > (Just trying to wrap my head around it...) Actually I mean (clone_flags & (CLONE_THREAD | CLONE_SIGHAND | CLONE_VM | CLONE_NEWPID)) CLONE_THREAD and CLONE_SIGHAND imply CLONE_VM... and that is enfored above. I don't mean all of those flags must be in place. CLONE_NEWPID is an optimization in that the test is also in copy_pid_ns but there is no point in going to all of the work to get there if we are going to be testing for this scenario anyway. I definitely don't mean (clone_flags & (CLONE_VM|CLONE_NEWPID)) == (CLONE_VM|CLONE_NEWPID)). The task_active_pid_ns(current) != current->nsproxy->pid_ns case is what tests to see if the pid namespace has already been unshared. The sequence "unshare(CLONE_NEWPID); unshare(CLONE_NEWPID);" is nonsense and that is what CLONE_NEWPID is about in that test. Similary the sequence "unshare(CLONE_NEWPID); clone(CLONE_THREAD);" is nonsense and what the CLONE_VM is the test is for. There are also a number of other nonsense thread like states that CLONE_VM also catches and prevents. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists