linux-kernel - Re: PATCH? fix unshare(NEWPID) && vfork()

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]

Date:	Tue, 20 Aug 2013 13:25:16 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	Brad Spengler <spender@...ecurity.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Colin Walters <walters@...hat.com>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: PATCH? fix unshare(NEWPID) && vfork()

Andy Lutomirski <luto@...capital.net> writes:

> On Tue, Aug 20, 2013 at 11:50 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>> On 08/20, Andy Lutomirski wrote:
>>>
>>> On Mon, Aug 19, 2013 at 11:43 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>>> > On 08/19, Andy Lutomirski wrote:
>>> >>
>>> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>>> >> >
>>> >> > So do you think this change is fine or not (ignoring the fact it needs
>>> >> > cleanups) ?
>>> >>
>>> >> I think that removing the CLONE_VM check is fine (although there are
>>> >> some other ones that should probably be removed as well), but I'm not
>>> >> sure if that check needs replacing with something else.
>>> >
>>> > OK, thanks... but I still can't understand.
>>> >
>>> > The patch I sent is equivalent to the new one below. I just tried to
>>> > unify it with another check in do_fork().
>>>
>>> I was confused.
>>
>> Andy, I do not know how much you were confused, but I bet I am confused
>> much more ;)
>>
>>> Currently (with or without your patch), vfork() followed by
>>> unshare(CLONE_NEWUSER) or unshare(CLONE_NEWPID) will unshare the VM.
>>
>> Could you spell please?
>>
>> We never unshare the VM. CLONE_VM in sys_unshare() paths just means
>> "fail unless ->mm is not shared".
>>
>
> Argh.  In that case this is probably buggy, and I am just as confused
> as you.  This stuff is serious spaghetti code.
>
> sys_unshare will see CLONE_NEWPID or CLONE_NEWUSER and set
> CLONE_THREAD.  Then it will see CLONE_THREAD and set CLONE_VM.  Then
> check_unshare_flags will see CLONE_VM and fail if we just called
> vfork.
>
> Could this be made much more comprehensible by having a single list of
> shareable things are allowed to be shared across namespaces and
> enforcing the *same* list in clone and unshare?

Having a single function would be nice.

Unforutunately the logic is different (unshare attempts to silently add
the flags you need to make it work) and clone only unshares what you
tell it too.

Even more than that the meaning of half of the bits changes meaning
between unshare and clone.

For clone CLONE_VM means don't generate a new VM.
For unshare CLONE_VM means generate a new VM.

As for reorganizing shrug.  It is always possible to make things better
but clone doesn't look too scary to me.

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/