[PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Oleg Nesterov]

clone(CLONE_PARENT | SIGXXX) blindly sets ->exit_signal = SIGXXX. It is not
clear to me what was the supposed behaviour but this does not look right.
The parent of the forking task will receive this signal which bypasses all
security checks.

With this patch CLONE_PARENT re-uses both ->real_parent and ->exit_signal,
this looks at least logical.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reported-by: Chris Evans <scarybeasts@gmail.com>

--- 6.29-rc3/kernel/fork.c~1_EXIT_SIGNAL	2009-02-09 01:03:48.000000000 +0100
+++ 6.29-rc3/kernel/fork.c	2009-02-25 18:09:56.000000000 +0100
@@ -1217,10 +1217,16 @@ static struct task_struct *copy_process(
 			!cpu_online(task_cpu(p))))
 		set_task_cpu(p, smp_processor_id());
 
-	/* CLONE_PARENT re-uses the old parent */
-	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
+	/* CLONE_PARENT re-uses the old parent and exit_signal */
+	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
+		/*
+		 * Do this under tasklist_lock to avoid the race with
+		 * re-parenting to init.
+		 */
+		if (!(clone_flags & CLONE_THREAD))
+			p->exit_signal = current->group_leader->exit_signal;
 		p->real_parent = current->real_parent;
-	else
+	} else
 		p->real_parent = current;
 
 	spin_lock(&current->sighand->siglock);

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Roland McGrath]

As I think I said before, I don't really know what the actual use case is
for CLONE_PARENT without CLONE_THREAD.  So it's easy to approve changing
its behavior, but I do vaguely worry about who expected what behavior before.

Just for readability, I would prefer that you just change the first line
above that sets p->exit_signal initially.  i.e. (but with some comments):

diff --git a/kernel/fork.c b/kernel/fork.c
index a66fbde..f56793b 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1184,7 +1184,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	p->parent_exec_id = p->self_exec_id;
 
 	/* ok, now we should be set up.. */
-	p->exit_signal = (clone_flags & CLONE_THREAD) ? -1 : (clone_flags & CSIGNAL);
+	p->exit_signal = ((clone_flags & CLONE_THREAD) ? -1 :
+			  (clone_flags & CLONE_PARENT) ?
+			  current->group_leader->exit_signal :
+			  (clone_flags & CSIGNAL));
 	p->pdeath_signal = 0;
 	p->exit_state = 0;
 


Thanks,
Roland

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Oleg Nesterov]

On 02/25, Roland McGrath wrote:
>
> As I think I said before, I don't really know what the actual use case is
> for CLONE_PARENT without CLONE_THREAD.  So it's easy to approve changing
> its behavior, but I do vaguely worry about who expected what behavior before.

Me too ;)

Hopefully CC is wide enough...

> Just for readability, I would prefer that you just change the first line
> above that sets p->exit_signal initially.  i.e. (but with some comments):
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index a66fbde..f56793b 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1184,7 +1184,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	p->parent_exec_id = p->self_exec_id;
>
>  	/* ok, now we should be set up.. */
> -	p->exit_signal = (clone_flags & CLONE_THREAD) ? -1 : (clone_flags & CSIGNAL);
> +	p->exit_signal = ((clone_flags & CLONE_THREAD) ? -1 :
> +			  (clone_flags & CLONE_PARENT) ?
> +			  current->group_leader->exit_signal :
> +			  (clone_flags & CSIGNAL));

Yes, I sent this patch to rhel list because it is most simple.

But afaics this is not exactly right.

The task T has ->exit_signal != SIGCHLD, say, SIGHUP. It does
clone(CLONE_PARENT), copy_process() sets ->exit_signal = SIGHUP.

T->parent exits, T is re-parented to /sbin/init, and its ->exit_signal
is changed to SIGCHLD.

copy_process() continues, takes tasklist, notices CLONE_PARENT and
sets ->real_parent = init. But child's ->exit_signal is still SIGHUP.

Oleg.

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Roland McGrath]

> The task T has ->exit_signal != SIGCHLD, say, SIGHUP. It does
> clone(CLONE_PARENT), copy_process() sets ->exit_signal = SIGHUP.
> 
> T->parent exits, T is re-parented to /sbin/init, and its ->exit_signal
> is changed to SIGCHLD.

I see.  It's tempting to call this a reason to just use SIGCHLD instead of
current->group_leader->exit_signal.  Of course, this all really begs the
question of who uses the case and what they want.


Thanks,
Roland

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Linus Torvalds]

On Wed, 25 Feb 2009, Roland McGrath wrote:
>
> As I think I said before, I don't really know what the actual use case is
> for CLONE_PARENT without CLONE_THREAD.  So it's easy to approve changing
> its behavior, but I do vaguely worry about who expected what behavior before.

I think changing it is wrong. 

I can easily see somebody using CLONE_PARENT to get the correct getppid 
semantics in the thread, and then setting the signal to zero to not make 
the parent see the thread go away. I think both yours and Oleg's patch 
break that.

And there _are_ programs that have used clone() directly - not using any 
pthreads emulation layer. Some game used it for audio threads etc.

So at the very least it should accept zero for "no signal". And quite 
frankly, it would be good to try to see if there are other alternatives.

		Linus

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Oleg Nesterov]

On 02/25, Linus Torvalds wrote:
>
> > As I think I said before, I don't really know what the actual use case is
> > for CLONE_PARENT without CLONE_THREAD.  So it's easy to approve changing
> > its behavior, but I do vaguely worry about who expected what behavior before.
>
> I think changing it is wrong.

Perhaps. As I said, I don't know what is the expected behaviour. And in fact
I can't think of the "obviously good" behaviour.

> I can easily see somebody using CLONE_PARENT to get the correct getppid
> semantics in the thread, and then setting the signal to zero to not make
> the parent see the thread go away.

->exit_signal == 0 doesn't mean the thread silently goes away, it becomes
a zombie (even if ->parent ignores SIGCHLD). We don't send the signal, but
that is all.

And if ->parent execs, we reset ->exit_signal to SIGCHLD anyway.

> So at the very least it should accept zero for "no signal".

perhaps. I don't know. But I am not sure this is always right.

> And quite
> frankly, it would be good to try to see if there are other alternatives.

Agreed. I thought about checking ->xxx_exec_id's in copy_process(),
but doesn't look very nice...


(can't resist... hopefully now it is clear this should have beeen discussed
 outside of the closed lists from the very beginning ;)

Oleg.

Re: [stable] [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Greg KH]

On Wed, Feb 25, 2009 at 10:20:39PM +0100, Oleg Nesterov wrote:
> (can't resist... hopefully now it is clear this should have beeen discussed
>  outside of the closed lists from the very beginning ;)

I don't understand why it wasn't in the first place, holding this back
helped nothing except delay this by a few months :(

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Oleg Nesterov]

On 02/25, Oleg Nesterov wrote:
>
> On 02/25, Linus Torvalds wrote:
> >
> > > As I think I said before, I don't really know what the actual use case is
> > > for CLONE_PARENT without CLONE_THREAD.  So it's easy to approve changing
> > > its behavior, but I do vaguely worry about who expected what behavior before.
> >
> > I think changing it is wrong.
>
> Perhaps. As I said, I don't know what is the expected behaviour. And in fact
> I can't think of the "obviously good" behaviour.
>
> > I can easily see somebody using CLONE_PARENT to get the correct getppid
> > semantics in the thread, and then setting the signal to zero to not make
> > the parent see the thread go away.
>
> ->exit_signal == 0 doesn't mean the thread silently goes away, it becomes
> a zombie (even if ->parent ignores SIGCHLD). We don't send the signal, but
> that is all.
>
> And if ->parent execs, we reset ->exit_signal to SIGCHLD anyway.

Really, how CLONE_PARENT + exit_signal==0 can be useful?

I still think this patch does the right change. Not because it fixes the
security problem, as I said I do not think the ability to send SIGKILL to
->parent is wrong from the security pov, even if child does setuid/etc,
the problem is that CLONE_PARENT can fool ->xxx_exec_id logic and we can
send SIGKILL after parent/child exec.

But because the current behaviour is just silly. Imho.

But of course, if this change can break the user-space applications, then
it should not be applied.

> > And quite
> > frankly, it would be good to try to see if there are other alternatives.
>
> Agreed. I thought about checking ->xxx_exec_id's in copy_process(),
> but doesn't look very nice...

I meant something like the patch below. But I don't like it.

Anybody has other ideas?

Oleg.

--- kernel/fork.c
+++ kernel/fork.c
@@ -1218,9 +1218,15 @@ static struct task_struct *copy_process(
 		set_task_cpu(p, smp_processor_id());
 
 	/* CLONE_PARENT re-uses the old parent */
-	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
+	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
 		p->real_parent = current->real_parent;
-	else
+		p->self_exec_id = p->parent_exec_id =
+				p->real_parent->self_exec_id;
+
+		if (current->parent_exec_id != current->real_parent->self_exec_id ||
+		    current->self_exec_id  != current->parent_exec_id)
+			p->exit_signal = SIGCHLD;
+	} else
 		p->real_parent = current;
 
 	spin_lock(&current->sighand->siglock);

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Linus Torvalds]

On Thu, 26 Feb 2009, Oleg Nesterov wrote:
> 
> Really, how CLONE_PARENT + exit_signal==0 can be useful?

Umm? For the reason I already stated?

> But because the current behaviour is just silly. Imho.

So? If there are people using it, you calling it silly is immaterial.

The fact is, it's an existing user space interface. We don't break them. 

> But of course, if this change can break the user-space applications, then
> it should not be applied.

That's the point. Nobody has shown that there aren't any users.

		Linus

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Oleg Nesterov]

On 02/26, Linus Torvalds wrote:
>
> On Thu, 26 Feb 2009, Oleg Nesterov wrote:
> >
> > Really, how CLONE_PARENT + exit_signal==0 can be useful?
>
> Umm? For the reason I already stated?

But I can not understand that reason... As I said, the thread does not
go away if exit_signal == 0. Yes, the parent does not see it has a new
zombie, but why this is useful?

> > But because the current behaviour is just silly. Imho.
>
> So? If there are people using it, you calling it silly is immaterial.
>
> The fact is, it's an existing user space interface. We don't break them.
>
> > But of course, if this change can break the user-space applications, then
> > it should not be applied.
>
> That's the point. Nobody has shown that there aren't any users.

Yes, yes, sure...

I sent another patch, perhaps someone can comment. Or suggest something
else.

Oleg.

Re: [PATCH 1/2] copy_process: fix CLONE_PARENT && ->exit_signal interaction

[Linus Torvalds]

On Thu, 26 Feb 2009, Oleg Nesterov wrote:
> 
> But I can not understand that reason... As I said, the thread does not
> go away if exit_signal == 0. Yes, the parent does not see it has a new
> zombie, but why this is useful?

Well, I know that people actually do this, because I found it in google 
code search. Maybe it was just a test program or something. So I didn't 
really care about "useful vs not", because I only care about "is it done 
vs not".

People do tons of useless things. That's the only way to explain Dr Phil. 

		Linus

[PATCH] copy_process: fix CLONE_PARENT && parent_exec_id interaction

[Oleg Nesterov]

On 02/26, Oleg Nesterov wrote:
>
> I sent another patch, perhaps someone can comment. Or suggest something
> else.

Nobody commented ;)

I am re-sending this patch simplified to one-liner. If this patch is
accepted, I think it makes sense to move the first
"p->parent_exec_id = p->self_exec_id" in copy_process() down, under
the "else" branch. Just for readability.

Note! This patch doesn't even try to address the original CVE. Let me
repeat, I am not the security expert, please correct me. But, unless
parent or child change security context (via exec), it is OK to send
any ->exit_signal when the child exits.

Comments?

----------------------------------------------------------------------
[PATCH] copy_process: fix CLONE_PARENT && parent_exec_id interaction

CLONE_PARENT can fool the ->self_exec_id/parent_exec_id logic. If we
re-use the old parent, we must also re-use ->parent_exec_id to make
sure exit_notify() sees the right ->xxx_exec_id's when the CLONE_PARENT'ed
task exits.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/fork.c~3_CLONE_PARENT	2009-03-02 20:24:59.000000000 +0100
+++ 6.29-rc3/kernel/fork.c	2009-03-02 21:45:45.000000000 +0100
@@ -1218,9 +1218,10 @@ static struct task_struct *copy_process(
 		set_task_cpu(p, smp_processor_id());
 
 	/* CLONE_PARENT re-uses the old parent */
-	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
+	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
 		p->real_parent = current->real_parent;
-	else
+		p->parent_exec_id = current->parent_exec_id;
+	} else
 		p->real_parent = current;
 
 	spin_lock(&current->sighand->siglock);

Re: [PATCH] copy_process: fix CLONE_PARENT && parent_exec_id interaction

[Linus Torvalds]

On Mon, 2 Mar 2009, Oleg Nesterov wrote:
> 
> I am re-sending this patch simplified to one-liner. If this patch is
> accepted, I think it makes sense to move the first
> "p->parent_exec_id = p->self_exec_id" in copy_process() down, under
> the "else" branch. Just for readability.
> 
> Note! This patch doesn't even try to address the original CVE. Let me
> repeat, I am not the security expert, please correct me. But, unless
> parent or child change security context (via exec), it is OK to send
> any ->exit_signal when the child exits.
> 
> Comments?

I think this looks correct and sane. And I agree with your "also move down 
the "p->parent_exec_id = p->self_exec_id" thing. In fact, I'd agree with 
it so much that I think it should be part of this patch, just because that 
not only clarifies the code, but it also makes it more obvious what the 
real change of this one single _patch_ is.

No?

		Linus

Re: [PATCH] copy_process: fix CLONE_PARENT && parent_exec_id interaction

[Oleg Nesterov]

On 03/02, Linus Torvalds wrote:
> 
> 
> On Mon, 2 Mar 2009, Oleg Nesterov wrote:
> > 
> > I am re-sending this patch simplified to one-liner. If this patch is
> > accepted, I think it makes sense to move the first
> > "p->parent_exec_id = p->self_exec_id" in copy_process() down, under
> > the "else" branch. Just for readability.
> > 
> > Note! This patch doesn't even try to address the original CVE. Let me
> > repeat, I am not the security expert, please correct me. But, unless
> > parent or child change security context (via exec), it is OK to send
> > any ->exit_signal when the child exits.
> > 
> > Comments?
> 
> I think this looks correct and sane. And I agree with your "also move down 
> the "p->parent_exec_id = p->self_exec_id" thing. In fact, I'd agree with 
> it so much that I think it should be part of this patch, just because that 
> not only clarifies the code, but it also makes it more obvious what the 
> real change of this one single _patch_ is.

Agreed, please find v2 below. But I failed to make the comment...

Also, "p->parent_exec_id = current->parent_exec_id" is not really needed
in v2, it was already copied by dup_task_struct(). But I think it is better
to make the code a bit more explicit.

------------------
[PATCH v2] copy_process: fix CLONE_PARENT && parent_exec_id interaction

CLONE_PARENT can fool the ->self_exec_id/parent_exec_id logic. If we
re-use the old parent, we must also re-use ->parent_exec_id to make
sure exit_notify() sees the right ->xxx_exec_id's when the CLONE_PARENT'ed
task exits.

Also, move down the "p->parent_exec_id = p->self_exec_id" thing, to place
two different cases together.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/fork.c~3_CLONE_PARENT	2009-03-02 20:24:59.000000000 +0100
+++ 6.29-rc3/kernel/fork.c	2009-03-02 22:38:39.000000000 +0100
@@ -1177,10 +1177,6 @@ static struct task_struct *copy_process(
 #endif
 	clear_all_latency_tracing(p);
 
-	/* Our parent execution domain becomes current domain
-	   These must match for thread signalling to apply */
-	p->parent_exec_id = p->self_exec_id;
-
 	/* ok, now we should be set up.. */
 	p->exit_signal = (clone_flags & CLONE_THREAD) ? -1 : (clone_flags & CSIGNAL);
 	p->pdeath_signal = 0;
@@ -1218,10 +1214,13 @@ static struct task_struct *copy_process(
 		set_task_cpu(p, smp_processor_id());
 
 	/* CLONE_PARENT re-uses the old parent */
-	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
+	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
 		p->real_parent = current->real_parent;
-	else
+		p->parent_exec_id = current->parent_exec_id;
+	} else {
 		p->real_parent = current;
+		p->parent_exec_id = current->self_exec_id;
+	}
 
 	spin_lock(&current->sighand->siglock);
 

Re: [PATCH] copy_process: fix CLONE_PARENT && parent_exec_id interaction

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> -	/* Our parent execution domain becomes current domain
> -	   These must match for thread signalling to apply */

Should you retain this comment somewhere?

David

Re: [PATCH] copy_process: fix CLONE_PARENT && parent_exec_id interaction

[Oleg Nesterov]

On 03/09, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > -	/* Our parent execution domain becomes current domain
> > -	   These must match for thread signalling to apply */
>
> Should you retain this comment somewhere?

This comment should be changed, but I failed to make the new one.
Actually, this was one of the reasons why the first patch I sent
http://marc.info/?l=linux-kernel&m=123602926516698
didn't touch the first ""p->parent_exec_id = ...".

There is a fat comment in exit_notify(), perhaps we can just add
/* see the comment in exit_notify() */.

But the "good" commenent should also explain that, since we re-use
->real_parent, the child also "inherits" the rights _and_ restrictions
to send ->exit_signal != SIGCHLD.

Oleg.