Messages in this thread | | | From | (Eric W. Biederman) | Date | Sat, 22 Dec 2012 12:16:22 -0800 | Subject | Re: [PATCH review 1/3] pidns: Outlaw thread creation after unshare(CLONE_NEWPID) |
| |
Rob Landley <rob@landley.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@redhat.com> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.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
| |