PATCH? fix unshare(NEWPID) && vfork()

PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

Hello.

Colin reports that vfork() doesn't work after unshare(PIDNS). The
reason is trivial, copy_process() does:

	/*
	 * 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);

and CLONE_VM obviously nacks vfork(). So perhaps we can relax
this check to CLONE_THREAD? Or should we really nack CLONE_VM
by security reasons?

OTOH. Perhaps we should also deny CLONE_PARENT in this case?

In short. So far I am thinking about the patch below but I got
lost and totally confused. Will try to think more tomorrow, but
I would like to see the fix from someone who still understands
this all.

Oleg.

--- x/kernel/fork.c	2013-08-14 18:34:06.000000000 +0200
+++ x/kernel/fork.c	2013-08-19 19:03:43.848823039 +0200
@@ -1172,14 +1172,6 @@ static struct task_struct *copy_process(
 				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);
-
 	retval = security_task_create(clone_flags);
 	if (retval)
 		goto fork_out;
@@ -1578,8 +1570,9 @@ long do_fork(unsigned long clone_flags,
 	 * Do some preliminary argument and permissions checking before we
 	 * actually start allocating stuff
 	 */
-	if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
-		if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
+	if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
+	    (task_active_pid_ns(current) != current->nsproxy->pid_ns)) {
+		if (clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_NEWPID))
 			return -EINVAL;
 	}
 

Re: PATCH? fix unshare(NEWPID) && vfork()

[Linus Torvalds]

On Mon, Aug 19, 2013 at 10:25 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Colin reports that vfork() doesn't work after unshare(PIDNS). The
> reason is trivial, copy_process() does:
>
>         /*
>          * 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);
>
> and CLONE_VM obviously nacks vfork(). So perhaps we can relax
> this check to CLONE_THREAD? Or should we really nack CLONE_VM
> by security reasons?
>
> OTOH. Perhaps we should also deny CLONE_PARENT in this case?

I agree that we should probably deny CLONE_PARENT, which makes more
sense paired with CLONE_NEWPID. I think we should also disallow
CLONE_THREAD, which is the thread goup.

And I *think* we can drop CLONE_VM. I suspect that snuck in as a
(misguided) attempt at CLONE_THREAD, as implied by the comment.

In fact, if you go look at the history of that CLONE_VM test, it came from

    unshare(CLONE_NEWPID)
    clone(CLONE_THREAD|CLONE_SIGHAND|CLONE_VM)

and the commit message talks about not setting pid_ns->child_reaper.
Which is very much about the PID, not about the shared VM space.

So I think your patch is correct, although I'm not sure why you move
the test. The new test you have look complicated as hell, so I think
you're actually making things worse by making them unreadable.

                  Linus

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/19, Linus Torvalds wrote:
>
> So I think your patch is correct, although I'm not sure why you move
> the test.

I didn't really, we have 2 tests, in do_fork() and copy_process().
I think it would be better to do the similar checks in one place.

This is off-topic and needs a separate change anyway, but perhaps
we should add the new helper which checks clone_flags. copy_process()
is huge.

> The new test you have look complicated as hell, so I think
> you're actually making things worse by making them unreadable.

Oh I agree. I'll wait for the comments on intent, then try to
cleanup it somehow. Unless a better fix comes.

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Andy Lutomirski]

On Mon, Aug 19, 2013 at 10:25 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Hello.
>
> Colin reports that vfork() doesn't work after unshare(PIDNS). The
> reason is trivial, copy_process() does:
>
>         /*
>          * 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);
>
> and CLONE_VM obviously nacks vfork(). So perhaps we can relax
> this check to CLONE_THREAD? Or should we really nack CLONE_VM
> by security reasons?
>
> OTOH. Perhaps we should also deny CLONE_PARENT in this case?
>
> In short. So far I am thinking about the patch below but I got
> lost and totally confused. Will try to think more tomorrow, but
> I would like to see the fix from someone who still understands
> this all.
>
> Oleg.

By way of (partial) explanation:

http://marc.info/?l=linux-kernel&m=135545831607095

(tl;dr: I think that CLONE_VM is irrelevant here, but there may be
other issues lurking around.)

--Andy

>
> --- x/kernel/fork.c     2013-08-14 18:34:06.000000000 +0200
> +++ x/kernel/fork.c     2013-08-19 19:03:43.848823039 +0200
> @@ -1172,14 +1172,6 @@ static struct task_struct *copy_process(
>                                 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);
> -
>         retval = security_task_create(clone_flags);
>         if (retval)
>                 goto fork_out;
> @@ -1578,8 +1570,9 @@ long do_fork(unsigned long clone_flags,
>          * Do some preliminary argument and permissions checking before we
>          * actually start allocating stuff
>          */
> -       if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
> -               if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
> +       if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
> +           (task_active_pid_ns(current) != current->nsproxy->pid_ns)) {
> +               if (clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_NEWPID))
>                         return -EINVAL;
>         }
>
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/19, Andy Lutomirski wrote:
>
> On Mon, Aug 19, 2013 at 10:25 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Hello.
> >
> > Colin reports that vfork() doesn't work after unshare(PIDNS). The
> > reason is trivial, copy_process() does:
> >
> >         /*
> >          * 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);
> >
> > and CLONE_VM obviously nacks vfork(). So perhaps we can relax
> > this check to CLONE_THREAD? Or should we really nack CLONE_VM
> > by security reasons?
> >
> > OTOH. Perhaps we should also deny CLONE_PARENT in this case?
> >
> > In short. So far I am thinking about the patch below but I got
> > lost and totally confused. Will try to think more tomorrow, but
> > I would like to see the fix from someone who still understands
> > this all.
> >
> > Oleg.
>
> By way of (partial) explanation:
>
> http://marc.info/?l=linux-kernel&m=135545831607095

Thanks... too late for me to even try to read this discussion today.

and I am a bit confused,

> (tl;dr: I think that CLONE_VM is irrelevant here, but there may be
> other issues lurking around.)

So do you think this change is fine or not (ignoring the fact it needs
cleanups) ?

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Andy Lutomirski]

On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/19, Andy Lutomirski wrote:
>>
>> On Mon, Aug 19, 2013 at 10:25 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > Hello.
>> >
>> > Colin reports that vfork() doesn't work after unshare(PIDNS). The
>> > reason is trivial, copy_process() does:
>> >
>> >         /*
>> >          * 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);
>> >
>> > and CLONE_VM obviously nacks vfork(). So perhaps we can relax
>> > this check to CLONE_THREAD? Or should we really nack CLONE_VM
>> > by security reasons?
>> >
>> > OTOH. Perhaps we should also deny CLONE_PARENT in this case?
>> >
>> > In short. So far I am thinking about the patch below but I got
>> > lost and totally confused. Will try to think more tomorrow, but
>> > I would like to see the fix from someone who still understands
>> > this all.
>> >
>> > Oleg.
>>
>> By way of (partial) explanation:
>>
>> http://marc.info/?l=linux-kernel&m=135545831607095
>
> Thanks... too late for me to even try to read this discussion today.
>
> and I am a bit confused,
>
>> (tl;dr: I think that CLONE_VM is irrelevant here, but there may be
>> other issues lurking around.)
>
> 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.

--Andy

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/19, Andy Lutomirski wrote:
>
> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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().

Oleg.

--- x/kernel/fork.c
+++ x/kernel/fork.c
@@ -1176,7 +1176,7 @@ static struct task_struct *copy_process(
 	 * 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)) &&
+	if ((clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_NEWPID)) &&
 	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
 		return ERR_PTR(-EINVAL);
 

Re: PATCH? fix unshare(NEWPID) && vfork()

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 08/19, Andy Lutomirski wrote:
>>
>> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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().

The patch below also needs CLONE_SIGHAND.  You can't meaningfully share
signal handlers if you can't represent the pid in the siginfo.  pids and
signals are too interconnected.

Eric

> Oleg.
>
> --- x/kernel/fork.c
> +++ x/kernel/fork.c
> @@ -1176,7 +1176,7 @@ static struct task_struct *copy_process(
>  	 * 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)) &&
> +	if ((clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_NEWPID)) &&
>  	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
>  		return ERR_PTR(-EINVAL);
>  

Re: PATCH? fix unshare(NEWPID) && vfork()

[Andy Lutomirski]

On Mon, Aug 19, 2013 at 11:43 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/19, Andy Lutomirski wrote:
>>
>> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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.

Currently (with or without your patch), vfork() followed by
unshare(CLONE_NEWUSER) or unshare(CLONE_NEWPID) will unshare the VM.
This may be surprising, but at least it will work.

--Andy

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/20, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 08/19, Andy Lutomirski wrote:
> >>
> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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().
>
> The patch below also needs CLONE_SIGHAND.  You can't meaningfully share
> signal handlers if you can't represent the pid in the siginfo.  pids and
> signals are too interconnected.

I don't really understand. If we allow to share ->mm (with this patch),
why it is bad to share sighand_struct->action[] ? This only shares the
pointers to the code which handles a signal.

However I agree it probably makes sense to deny it "just in case",
I do not think CLONE_SIGHAND can be useful in this case.

But then we should also deny CLONE_SIGHAND if CLONE_NEWUSER|CLONE_NEWPID
(another check in do_fork()). Which makes me think again we should unify
these 2 checks.

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/20, Andy Lutomirski wrote:
>
> On Mon, Aug 19, 2013 at 11:43 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 08/19, Andy Lutomirski wrote:
> >>
> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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".

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Andy Lutomirski]

On Tue, Aug 20, 2013 at 11:50 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/20, Andy Lutomirski wrote:
>>
>> On Mon, Aug 19, 2013 at 11:43 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 08/19, Andy Lutomirski wrote:
>> >>
>> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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?

--Andy



> Oleg.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/20, Andy Lutomirski wrote:
>
> On Tue, Aug 20, 2013 at 11:50 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> >> 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,

I don't think so. Just we can't really unshare ->mm or implement
unshare(CLONE_THREAD). We simply pretend it works if there is nothing
to unshare.

> sys_unshare will see CLONE_NEWPID or CLONE_NEWUSER and set
> CLONE_THREAD.  Then it will see CLONE_THREAD and set CLONE_VM.

This matches copy_process() to some degree... but looks confusing,
I agree.

> 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?

Not sure...

but at least we can probably simplify this a little bit. Say, we can
just kill

	if (unshare_flags & CLONE_THREAD)
		unshare_flags |= CLONE_VM;

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Andy Lutomirski]

On Tue, Aug 20, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/20, Andy Lutomirski wrote:
>>
>> On Tue, Aug 20, 2013 at 11:50 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> >> 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,
>
> I don't think so. Just we can't really unshare ->mm or implement
> unshare(CLONE_THREAD). We simply pretend it works if there is nothing
> to unshare.
>
>> sys_unshare will see CLONE_NEWPID or CLONE_NEWUSER and set
>> CLONE_THREAD.  Then it will see CLONE_THREAD and set CLONE_VM.
>
> This matches copy_process() to some degree... but looks confusing,
> I agree.

Huh?  Doesn't this mean that unshare(CLONE_NEWPID); vfork() will work
with your patches, but vfork(); unshare(CLONE_NEWPID) will fail?  (I
admit I haven't tested it.)

--Andy

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/20, Andy Lutomirski wrote:
>
> On Tue, Aug 20, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 08/20, Andy Lutomirski wrote:
> >>
> >> On Tue, Aug 20, 2013 at 11:50 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >> >
> >> >> 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,
> >
> > I don't think so. Just we can't really unshare ->mm

this looks confusing, sorry. Afaics it is possible to implement
unshare(CLONE_VM), but

> or implement
> > unshare(CLONE_THREAD).

this is unlikely.

but this doesn't matter,

> We simply pretend it works if there is nothing
> > to unshare.
> >
> >> sys_unshare will see CLONE_NEWPID or CLONE_NEWUSER and set
> >> CLONE_THREAD.  Then it will see CLONE_THREAD and set CLONE_VM.
> >
> > This matches copy_process() to some degree... but looks confusing,
> > I agree.

I only mean that copy_process() requires CLONE_VM if CLONE_THREAD.
But, unlike unshare(), it fails if CLONE_VM is not set.

> Huh?  Doesn't this mean that unshare(CLONE_NEWPID); vfork() will work
> with your patches,

I hope,

> but vfork(); unshare(CLONE_NEWPID) will fail?  (I
> admit I haven't tested it.)

Do you mean that the child does unshare(CLONE_NEWPID) before exec?
It should fail with or without this patch.

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Andy Lutomirski]

On Tue, Aug 20, 2013 at 12:23 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/20, Andy Lutomirski wrote:
>>
>> On Tue, Aug 20, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 08/20, Andy Lutomirski wrote:
>> Huh?  Doesn't this mean that unshare(CLONE_NEWPID); vfork() will work
>> with your patches,
>
> I hope,
>
>> but vfork(); unshare(CLONE_NEWPID) will fail?  (I
>> admit I haven't tested it.)
>
> Do you mean that the child does unshare(CLONE_NEWPID) before exec?
> It should fail with or without this patch.
>

Exactly. Shouldn't it succeed?

--Andy

Re: PATCH? fix unshare(NEWPID) && vfork()

[Eric W. Biederman]

Andy Lutomirski <luto@amacapital.net> writes:

> On Tue, Aug 20, 2013 at 11:50 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 08/20, Andy Lutomirski wrote:
>>>
>>> On Mon, Aug 19, 2013 at 11:43 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> > On 08/19, Andy Lutomirski wrote:
>>> >>
>>> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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

Re: PATCH? fix unshare(NEWPID) && vfork()

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 08/20, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > On 08/19, Andy Lutomirski wrote:
>> >>
>> >> On Mon, Aug 19, 2013 at 11:33 AM, Oleg Nesterov <oleg@redhat.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().
>>
>> The patch below also needs CLONE_SIGHAND.  You can't meaningfully share
>> signal handlers if you can't represent the pid in the siginfo.  pids and
>> signals are too interconnected.
>
> I don't really understand. If we allow to share ->mm (with this patch),
> why it is bad to share sighand_struct->action[] ? This only shares the
> pointers to the code which handles a signal.

Not the signal queues?   I guess it is only CLONE_THREAD that shares the
signal queues between tasks.

The practical problem I am worried about with signals is in
__send_signal and the lead up to send signal, pids and uids are stored
in the struct siginfo in the namespace of the destination task.  If that
destination task shares the signal queues we can not possibly store the
proper information.

I believe that sharing just the signal handlers between tasks is also a
problem because while in principle you could distinguish the signals.
In practice it will require at least an extra system call to do so.

> However I agree it probably makes sense to deny it "just in case",
> I do not think CLONE_SIGHAND can be useful in this case.

I agree.  The only CLONE_VM case I can imagine being useful is vfork().

> But then we should also deny CLONE_SIGHAND if CLONE_NEWUSER|CLONE_NEWPID
> (another check in do_fork()). Which makes me think again we should unify
> these 2 checks.

I agree CLONE_SIGHAND if CLONE_NEWUSER|CLONE_NEWPID.

As for unifying them.  I can see colocating the related checks to keep
everything together.  I don't know about unifying them.

I know I get a little dizzy when trying to think of all of the possible
orderings of unshare and clone that can result in problem cases.

Hmm.  Thinking a little more I am half sold on unification.  If we just
handle the pid namespace by itself.  And we handle the user namespace by
itself I think things make sense.

So I am thinking something like the diff below.  CLONE_SIGHAND as in
theory you can figure out which task you are in and sort it out,
although I don't expect that to happen in practice.

Eric



diff --git a/kernel/fork.c b/kernel/fork.c
index 66635c8..0356852 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1172,12 +1172,21 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 				current->signal->flags & SIGNAL_UNKILLABLE)
 		return ERR_PTR(-EINVAL);
 
+	/* Dissallow unshare(CLONE_NEWPID) ; clone(CLONE_NEWPID).
+	 * That can result in a possibly empty parent pid namespace
+	 * which makes no sense.
+	 */
+	if ((clone_flags & CLONE_NEWPID) &&
+	    task_active_pid_ns(current) != current->nsproxy->pid_ns)
+		return ERR_PTR(-EINVAL);
+
 	/*
 	 * If the new process will be in a different pid namespace
-	 * don't allow the creation of threads.
+	 * don't allow the problematic sharing.
 	 */
-	if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
-	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
+	if ((clone_flags & (CLONE_THREAD|CLONE_SIGHAND|CLONE_PARENT)) &&
+	    ((clone_flags & CLONE_NEWPID) ||
+	     (task_active_pid_ns(current) != current->nsproxy->pid_ns))
 		return ERR_PTR(-EINVAL);
 
 	retval = security_task_create(clone_flags);
@@ -1578,8 +1587,8 @@ long do_fork(unsigned long clone_flags,
 	 * Do some preliminary argument and permissions checking before we
 	 * actually start allocating stuff
 	 */
-	if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
-		if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
+	if (clone_flags & (CLONE_NEWUSER)) {
+		if (clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_SIGHAND))
 			return -EINVAL;
 	}
 
@@ -1816,12 +1825,12 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
 	 * If unsharing a user namespace must also unshare the thread.
 	 */
 	if (unshare_flags & CLONE_NEWUSER)
-		unshare_flags |= CLONE_THREAD | CLONE_FS;
+		unshare_flags |= CLONE_THREAD | CLONE_FS | CLONE_SIGHAND;
 	/*
 	 * If unsharing a pid namespace must also unshare the thread.
 	 */
 	if (unshare_flags & CLONE_NEWPID)
-		unshare_flags |= CLONE_THREAD;
+		unshare_flags |= CLONE_THREAD | CLONE_SIGHAND;
 	/*
 	 * If unsharing a thread from a thread group, must also unshare vm.
 	 */

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/20, Andy Lutomirski wrote:
>
> On Tue, Aug 20, 2013 at 12:23 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> >> but vfork(); unshare(CLONE_NEWPID) will fail?  (I
> >> admit I haven't tested it.)
> >
> > Do you mean that the child does unshare(CLONE_NEWPID) before exec?
> > It should fail with or without this patch.
> >
>
> Exactly. Shouldn't it succeed?

Ah, I am starting to understand... Sorry, I thought that you were
arguing with the patch I sent.

Yes, perhaps you are right... But this is another isssue, and needs
another change and another discussion. Afaics, we only need to change
the "clone_flags" checks in sys_unshare().

Oleg.

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/20, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> >>
> >> The patch below also needs CLONE_SIGHAND.  You can't meaningfully share
> >> signal handlers if you can't represent the pid in the siginfo.  pids and
> >> signals are too interconnected.
> >
> > I don't really understand. If we allow to share ->mm (with this patch),
> > why it is bad to share sighand_struct->action[] ? This only shares the
> > pointers to the code which handles a signal.
>
> Not the signal queues?   I guess it is only CLONE_THREAD that shares the
> signal queues between tasks.

Yes, and we should nack CLONE_THREAD anyway.

> I believe that sharing just the signal handlers between tasks is also a
> problem because while in principle you could distinguish the signals.
> In practice it will require at least an extra system call to do so.

I still do not think this is a problem. If nothing else they share
the code, the fact that they also share the entry point for the signal
handler is not relevant, I think. And they share it anyway until the
child or parent does sigaction().

But this doesn't matter, we both agree that it would be better to deny
CLONE_SIGHAND anyway.

> So I am thinking something like the diff below.  CLONE_SIGHAND as in
> theory you can figure out which task you are in and sort it out,
> although I don't expect that to happen in practice.

Well, I do not really mind. And I won't argue if you submit this patch.

But can't we at least move this CLONE_NEWUSER to copy_process? closer
to other clone_flags checks.

As for your patch,


> +	/* Dissallow unshare(CLONE_NEWPID) ; clone(CLONE_NEWPID).
> +	 * That can result in a possibly empty parent pid namespace
> +	 * which makes no sense.
> +	 */
> +	if ((clone_flags & CLONE_NEWPID) &&
> +	    task_active_pid_ns(current) != current->nsproxy->pid_ns)
> +		return ERR_PTR(-EINVAL);

This looks unneeded... copy_pid_ns() should fail in this case, no?


> +	if ((clone_flags & (CLONE_THREAD|CLONE_SIGHAND|CLONE_PARENT)) &&
> ...
> +		if (clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_SIGHAND))

This doesn't really matter, but CLONE_THREAD can be omitted.


Still. Can't we make a single check? Like the initial patch I sent, but
this one moves the check into copy_process() and checks CLONE_* first.
Looks a bit simpler. And more understandable to me but this is subjective.

But once again, I won't argue, it's up to you.

Oleg.

--- x/kernel/fork.c
+++ x/kernel/fork.c
@@ -1173,12 +1173,13 @@ static struct task_struct *copy_process(
 		return ERR_PTR(-EINVAL);
 
 	/*
-	 * If the new process will be in a different pid namespace
-	 * don't allow the creation of threads.
+	 * --------------  COMMENT -----------------
 	 */
-	if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
-	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
-		return ERR_PTR(-EINVAL);
+	if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) {
+		if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
+		    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
+			return -EINVAL;
+	}
 
 	retval = security_task_create(clone_flags);
 	if (retval)
@@ -1575,15 +1576,6 @@ long do_fork(unsigned long clone_flags,
 	long nr;
 
 	/*
-	 * Do some preliminary argument and permissions checking before we
-	 * actually start allocating stuff
-	 */
-	if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
-		if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
-			return -EINVAL;
-	}
-
-	/*
 	 * Determine whether and which event to report to ptracer.  When
 	 * called from kernel_thread or CLONE_UNTRACED is explicitly
 	 * requested, no event is reported; otherwise, report if the event

Re: PATCH? fix unshare(NEWPID) && vfork()

[Oleg Nesterov]

On 08/21, Oleg Nesterov wrote:
>
> Still. Can't we make a single check? Like the initial patch I sent, but
> this one moves the check into copy_process() and checks CLONE_* first.
> Looks a bit simpler. And more understandable to me but this is subjective.

Seriously. CLONE_NEWPID and task_active_pid_ns() != nsproxy->pid_ns are
the same thing in this respect. Let me send the patches for review, I
decided to split this change.

> --- x/kernel/fork.c
> +++ x/kernel/fork.c
> @@ -1173,12 +1173,13 @@ static struct task_struct *copy_process(
>  		return ERR_PTR(-EINVAL);
>  
>  	/*
> -	 * If the new process will be in a different pid namespace
> -	 * don't allow the creation of threads.
> +	 * --------------  COMMENT -----------------
>  	 */
> -	if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
> -	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
> -		return ERR_PTR(-EINVAL);
> +	if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) {
> +		if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
> +		    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
> +			return -EINVAL;
                                ^^^^^^

should be ERR_PTR(-EINVAL)

Oleg.