linux-kernel - Re: [REVIEW][PATCH 3/5] pidns: Don't have unshare(CLONE_NEWPID) imply CLONE

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ppsuviwb.fsf@xmission.com>
Date:	Fri, 30 Aug 2013 16:49:40 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	Linux Containers <containers@...ts.linux-foundation.org>,
	linux-kernel@...r.kernel.org, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [REVIEW][PATCH 3/5] pidns: Don't have unshare(CLONE_NEWPID) imply CLONE_THREAD

"Serge E. Hallyn" <serge@...lyn.com> writes:

> Quoting Eric W. Biederman (ebiederm@...ssion.com):
>> 
>> I goofed when I made unshare(CLONE_NEWPID) only work in a
>> single-threaded process.  There is no need for that requirement and in
>> fact I analyzied things right for setns.  The hard requirement
>> is for tasks that share a VM to all be in the pid namespace and
>> we properly prevent that in do_fork.
>
> I don't understand though - copy_process does have the right test:
>
>    1176          * If the new process will be in a different pid namespace
>    1177          * don't allow the creation of threads.
>    1178          */
>    1179         if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
>    1180             (task_active_pid_ns(current) != current->nsproxy->pid_ns))
>    1181                 return ERR_PTR(-EINVAL);
>
> but why is it ok for sys_unshare not to do that?  Note that
> in order for check_unshare_flags() to bail on &current->mm->mm_users > 1
> you do have to set CLONE_VM (for inverse interpretation).
>
> So it seems to me this isn't safe as is, and we need to at least
> set CLONE_VM if CLONE_PID is set.

Partly this is the difference in the meaning of the flags between
unshare and clone.

Basically in unshare all othat gets changed is
current->nsproxy->pid_ns_for_children (the rename is in the net tree).

So because unshare of the pid namespace does not actually effect the
current processes, just the pid namespace the children of the current
thread will be in this is safe.

And frankly having the checks be obviously different is a good thing
because it means that people will ask why in the world this is so and
realize the difference in meaning.

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/