[PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Eric W. Biederman]

The weird things we do when we exec from a thread group are just ugly.
Those ugly things do not handle the case of init and I suspect
extending that code to properly support a threaded init would be just
hideous, and impossible to maintain. 

So just in case someone ever threads init return an error for the
unimplemented case.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/exec.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

408dad0f2b7067b23929866150e73b2b2f12d662
diff --git a/fs/exec.c b/fs/exec.c
index 055378d..c9d8e31 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -600,6 +600,12 @@ static int de_thread(struct task_struct 
 	if (thread_group_empty(current))
 		goto no_thread_group;
 
+	/* A threaded init must exec from it's primary thread.
+	 * As the init task (i.e. child_reaper) may not exit.
+	 */
+	if (!thread_group_leader(current) && (current->tgid == 1))
+		return -EINVAL;
+	
 	/*
 	 * Kill all other threads in the thread group.
 	 * We must hold tasklist_lock to call zap_other_threads.
-- 
1.1.5.g3480

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
> The weird things we do when we exec from a thread group are just ugly.
> Those ugly things do not handle the case of init and I suspect
> extending that code to properly support a threaded init would be just
> hideous, and impossible to maintain. 
> 
> So just in case someone ever threads init return an error for the
> unimplemented case.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> 
> 
> ---
> 
>  fs/exec.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> 408dad0f2b7067b23929866150e73b2b2f12d662
> diff --git a/fs/exec.c b/fs/exec.c
> index 055378d..c9d8e31 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -600,6 +600,12 @@ static int de_thread(struct task_struct 
>  	if (thread_group_empty(current))
>  		goto no_thread_group;
>  
> +	/* A threaded init must exec from it's primary thread.
> +	 * As the init task (i.e. child_reaper) may not exit.
> +	 */
> +	if (!thread_group_leader(current) && (current->tgid == 1))
> +		return -EINVAL;
> +	
>  	/*
>  	 * Kill all other threads in the thread group.
>  	 * We must hold tasklist_lock to call zap_other_threads.

hmm, this just looks like overhead.  If sometime someone _does_ try to
thread init, what will happen to them?  If it's something nice and nasty,
they'll just whine at us and stop doing that.  Same net effect, no runtime
cost.

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

>> 408dad0f2b7067b23929866150e73b2b2f12d662
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 055378d..c9d8e31 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -600,6 +600,12 @@ static int de_thread(struct task_struct 
>>  	if (thread_group_empty(current))
>>  		goto no_thread_group;
>>  
>> +	/* A threaded init must exec from it's primary thread.
>> +	 * As the init task (i.e. child_reaper) may not exit.
>> +	 */
>> +	if (!thread_group_leader(current) && (current->tgid == 1))
>> +		return -EINVAL;
>> +	
>>  	/*
>>  	 * Kill all other threads in the thread group.
>>  	 * We must hold tasklist_lock to call zap_other_threads.
>
> hmm, this just looks like overhead.  If sometime someone _does_ try to
> thread init, what will happen to them?  If it's something nice and nasty,
> they'll just whine at us and stop doing that.  Same net effect, no runtime
> cost.

So threading init will work just fine.  The only case that will blow up
is calling exec from something that is not the thread group leader.
i.e.  If tgid == 1 but pid != 1 the kernel will cause pid == 1 to exit.

I think that will trigger a kernel panic.  It might just ensure that
no more processes are re-parented to init.  And we dereference a
bad pointer we look at child_reaper.  I haven't been brave enough
to try it.

The cost is only paid if you are a threaded task and you call exec.
Normal process never take that path, so we are already off of the
fast path.

The test when all expanded out is only:

if ((current->tgid == current->pid) && (current->tgid == 1))
	return -EINVAL

So it should be relatively cheap.


If process id namespaces become a reality init stops being
terribly special, and becomes something you may have several
of running at any one time.  If one of those inits is compromised
by a hostile user I having the whole system go down so we can
avoid executing a cheap test sounds terribly wrong.  That is
why I really care.

Eric

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>  If process id namespaces become a reality init stops being
>  terribly special, and becomes something you may have several
>  of running at any one time.  If one of those inits is compromised
>  by a hostile user I having the whole system go down so we can
>  avoid executing a cheap test sounds terribly wrong.  That is
>  why I really care.

Wouldn't it be better to do nothing until/unless there's some code in the
kernel or init which actually needs the change?

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Pavel Machek]

On Ne 29-01-06 02:48:31, Andrew Morton wrote:
> ebiederm@xmission.com (Eric W. Biederman) wrote:
> >
> >  If process id namespaces become a reality init stops being
> >  terribly special, and becomes something you may have several
> >  of running at any one time.  If one of those inits is compromised
> >  by a hostile user I having the whole system go down so we can
> >  avoid executing a cheap test sounds terribly wrong.  That is
> >  why I really care.
> 
> Wouldn't it be better to do nothing until/unless there's some code in the
> kernel or init which actually needs the change?

It is common to do init=/bin/bash, and I guess people are doing it
with all kinds of wonderful apps....
								Pavel
-- 
Thanks, Sharp!

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> On Ne 29-01-06 02:48:31, Andrew Morton wrote:
> > ebiederm@xmission.com (Eric W. Biederman) wrote:
> > >
> > >  If process id namespaces become a reality init stops being
> > >  terribly special, and becomes something you may have several
> > >  of running at any one time.  If one of those inits is compromised
> > >  by a hostile user I having the whole system go down so we can
> > >  avoid executing a cheap test sounds terribly wrong.  That is
> > >  why I really care.
> > 
> > Wouldn't it be better to do nothing until/unless there's some code in the
> > kernel or init which actually needs the change?
> 
> It is common to do init=/bin/bash, and I guess people are doing it
> with all kinds of wonderful apps....

err, good point.  And no reports of peculiar things happening with
threading.  Eric's check has the (slight) potential to cause some things to
stop working though.

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Eric W. Biederman]

Pavel Machek <pavel@ucw.cz> writes:

> On Ne 29-01-06 02:48:31, Andrew Morton wrote:
>> ebiederm@xmission.com (Eric W. Biederman) wrote:
>> >
>> >  If process id namespaces become a reality init stops being
>> >  terribly special, and becomes something you may have several
>> >  of running at any one time.  If one of those inits is compromised
>> >  by a hostile user I having the whole system go down so we can
>> >  avoid executing a cheap test sounds terribly wrong.  That is
>> >  why I really care.
>> 
>> Wouldn't it be better to do nothing until/unless there's some code in the
>> kernel or init which actually needs the change?

Well I think deliberate kernel bugs should at least be accompanied by
a big fat warning in the code, and an explanation of what is happening
and why.

> It is common to do init=/bin/bash, and I guess people are doing it
> with all kinds of wonderful apps....

So it is just a matter of time before someone actually hits this.

I went back through and tested and the actual symptoms are weird.
The thread calling exec just hangs forever, waiting for pid == 1 to
exit.  Anyway I found a simple fix so it is probably better to fix
it than to paper over the problem anyway.

Patch follows in a minute.

Eric

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Jan Engelhardt]

>
>So threading init will work just fine.  The only case that will blow up
>is calling exec from something that is not the thread group leader.
>i.e.  If tgid == 1 but pid != 1 the kernel will cause pid == 1 to exit.

Should not it, at its best, replace the whole thread group by the new 
program and have things carry on?


Jan Engelhardt
-- 

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Eric W. Biederman]

Jan Engelhardt <jengelh@linux01.gwdg.de> writes:

>>
>>So threading init will work just fine.  The only case that will blow up
>>is calling exec from something that is not the thread group leader.
>>i.e.  If tgid == 1 but pid != 1 the kernel will cause pid == 1 to exit.
>
> Should not it, at its best, replace the whole thread group by the new 
> program and have things carry on?

You see the bug.

Eric

Re: [PATCH] exec: Only allow a threaded init to exec from the thread_group_leader

[Bodo Eggert]

Eric W. Biederman <ebiederm@xmission.com> wrote:

> If process id namespaces become a reality init stops being
> terribly special, and becomes something you may have several
> of running at any one time.  If one of those inits is compromised
> by a hostile user I having the whole system go down so we can
> avoid executing a cheap test sounds terribly wrong.  That is
> why I really care.

There are virtual environments like linux-vserver(.org), where init is
running several times on one system, each with their local/virtual
pid being 1. Killing them does no harm unless it's the real init.
I asume in your system,  the real init will exist under the control
of the administrator, too, so there should be no danger.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.