[PATCH 1/3] posix_timer: remove wrong comment

[PATCH 1/3] posix_timer: remove wrong comment

[Andrey Vagin]

currently "The next step is hard to back out if there is an error." comment is not right,
release_posix_timer() does put_pid().

Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 kernel/posix-timers.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 00d1fda..5555e7c 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -563,10 +563,6 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	if (error)
 		goto out;
 
-	/*
-	 * return the timer_id now.  The next step is hard to
-	 * back out if there is an error.
-	 */
 	if (copy_to_user(created_timer_id,
 			 &new_timer_id, sizeof (new_timer_id))) {
 		error = -EFAULT;
-- 
1.6.6

[PATCH 2/3] posix_timer: fix error path in timer_create

[Andrey Vagin]

move CLOCK_DISPATCH(which_clock, timer_create, (new_timer)) after all
possible EFAULT errors.

*_timer_create may allocate/get resources.
(for example posix_cpu_timer_create does get_task_struct)

Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 kernel/posix-timers.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 5555e7c..8393624 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -555,14 +555,6 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 		goto out;
 	}
 
-	it_id_set = IT_ID_SET;
-	new_timer->it_id = (timer_t) new_timer_id;
-	new_timer->it_clock = which_clock;
-	new_timer->it_overrun = -1;
-	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
-	if (error)
-		goto out;
-
 	if (copy_to_user(created_timer_id,
 			 &new_timer_id, sizeof (new_timer_id))) {
 		error = -EFAULT;
@@ -593,6 +585,14 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	new_timer->sigq->info.si_tid   = new_timer->it_id;
 	new_timer->sigq->info.si_code  = SI_TIMER;
 
+	it_id_set = IT_ID_SET;
+	new_timer->it_id = (timer_t) new_timer_id;
+	new_timer->it_clock = which_clock;
+	new_timer->it_overrun = -1;
+	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
+	if (error)
+		goto out;
+
 	spin_lock_irq(&current->sighand->siglock);
 	new_timer->it_signal = current->signal;
 	list_add(&new_timer->list, &current->signal->posix_timers);
-- 
1.6.6

[PATCH 3/3] posix_timer: move copy_to_user(created_timer_id) down in timer_create

[Andrey Vagin]

According to Oleg Nesterov:
"We can move copy_to_user(created_timer_id) down after "if (timer_event_spec)"
block too. (but before CLOCK_DISPATCH(), of course)."

Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 kernel/posix-timers.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 8393624..6da1752 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -555,11 +555,6 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 		goto out;
 	}
 
-	if (copy_to_user(created_timer_id,
-			 &new_timer_id, sizeof (new_timer_id))) {
-		error = -EFAULT;
-		goto out;
-	}
 	if (timer_event_spec) {
 		if (copy_from_user(&event, timer_event_spec, sizeof (event))) {
 			error = -EFAULT;
@@ -585,6 +580,12 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	new_timer->sigq->info.si_tid   = new_timer->it_id;
 	new_timer->sigq->info.si_code  = SI_TIMER;
 
+	if (copy_to_user(created_timer_id,
+			 &new_timer_id, sizeof (new_timer_id))) {
+		error = -EFAULT;
+		goto out;
+	}
+
 	it_id_set = IT_ID_SET;
 	new_timer->it_id = (timer_t) new_timer_id;
 	new_timer->it_clock = which_clock;
-- 
1.6.6

Re: [PATCH 2/3] posix_timer: fix error path in timer_create

[Stanislaw Gruszka]

On Mon, 17 May 2010 17:41:43 +0400
Andrey Vagin <avagin@openvz.org> wrote:

> move CLOCK_DISPATCH(which_clock, timer_create, (new_timer)) after all
> possible EFAULT errors.
> 
> *_timer_create may allocate/get resources.
> (for example posix_cpu_timer_create does get_task_struct)
> 
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
>  kernel/posix-timers.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> index 5555e7c..8393624 100644
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -555,14 +555,6 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>  		goto out;
>  	}
>  
> -	it_id_set = IT_ID_SET;
> -	new_timer->it_id = (timer_t) new_timer_id;
> -	new_timer->it_clock = which_clock;
> -	new_timer->it_overrun = -1;
> -	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
> -	if (error)
> -		goto out;
> -
>  	if (copy_to_user(created_timer_id,
>  			 &new_timer_id, sizeof (new_timer_id))) {
>  		error = -EFAULT;
> @@ -593,6 +585,14 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>  	new_timer->sigq->info.si_tid   = new_timer->it_id;
>  	new_timer->sigq->info.si_code  = SI_TIMER;
>  
> +	it_id_set = IT_ID_SET;
> +	new_timer->it_id = (timer_t) new_timer_id;

This part should not be moved, this make possible leak of idr entry .

> +	new_timer->it_clock = which_clock;
> +	new_timer->it_overrun = -1;

I'm not so convenient of moving this as well.

> +	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
> +	if (error)
> +		goto out;
> +
>  	spin_lock_irq(&current->sighand->siglock);
>  	new_timer->it_signal = current->signal;
>  	list_add(&new_timer->list, &current->signal->posix_timers);

Thanks
Stanislaw

Re: [PATCH 2/3] posix_timer: fix error path in timer_create

[Andrew Vagin]

On 05/17/2010 06:31 PM, Stanislaw Gruszka wrote:
> On Mon, 17 May 2010 17:41:43 +0400
> Andrey Vagin<avagin@openvz.org>  wrote:
>
>    
>> @@ -593,6 +585,14 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
>>   	new_timer->sigq->info.si_tid   = new_timer->it_id;
>>   	new_timer->sigq->info.si_code  = SI_TIMER;
>>
>> +	it_id_set = IT_ID_SET;
>> +	new_timer->it_id = (timer_t) new_timer_id;
>>      
> This part should not be moved, this make possible leak of idr entry .
>    
You are right. Pls, skip this patches too.
>    
>> +	new_timer->it_clock = which_clock;
>> +	new_timer->it_overrun = -1;
>>      
> I'm not so convenient of moving this as well.
>
>    
>> +	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
>> +	if (error)
>> +		goto out;
>> +
>>   	spin_lock_irq(&current->sighand->siglock);
>>   	new_timer->it_signal = current->signal;
>>   	list_add(&new_timer->list,&current->signal->posix_timers);
>>      
> Thanks
> Stanislaw
>    

[PATCH 2/3] posix_timer: fix error path in timer_create

[Andrey Vagin]

move CLOCK_DISPATCH(which_clock, timer_create, (new_timer)) after all
posible EFAULT erros.

*_timer_create may allocate/get resources.
(for example posix_cpu_timer_create does get_task_struct)

Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 kernel/posix-timers.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 5555e7c..ad72342 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -559,9 +559,6 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	new_timer->it_id = (timer_t) new_timer_id;
 	new_timer->it_clock = which_clock;
 	new_timer->it_overrun = -1;
-	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
-	if (error)
-		goto out;
 
 	if (copy_to_user(created_timer_id,
 			 &new_timer_id, sizeof (new_timer_id))) {
@@ -593,6 +590,10 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	new_timer->sigq->info.si_tid   = new_timer->it_id;
 	new_timer->sigq->info.si_code  = SI_TIMER;
 
+	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
+	if (error)
+		goto out;
+
 	spin_lock_irq(&current->sighand->siglock);
 	new_timer->it_signal = current->signal;
 	list_add(&new_timer->list, &current->signal->posix_timers);
-- 
1.6.6

Re: [PATCH 2/3] posix_timer: fix error path in timer_create

[Stanislaw Gruszka]

On Mon, 17 May 2010 21:18:07 +0400
Andrey Vagin <avagin@openvz.org> wrote:

> move CLOCK_DISPATCH(which_clock, timer_create, (new_timer)) after all
> posible EFAULT erros.
> 
> *_timer_create may allocate/get resources.
> (for example posix_cpu_timer_create does get_task_struct)
> 
> Signed-off-by: Andrey Vagin <avagin@openvz.org>

Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>

Re: [PATCH 2/3] posix_timer: fix error path in timer_create

[Andrew Morton]

On Mon, 17 May 2010 21:18:07 +0400
Andrey Vagin <avagin@openvz.org> wrote:

> move CLOCK_DISPATCH(which_clock, timer_create, (new_timer)) after all
> posible EFAULT erros.
> 
> *_timer_create may allocate/get resources.
> (for example posix_cpu_timer_create does get_task_struct)
> 

You've added a Cc:stable@kernel.org to the mail headers, but there is
no "Cc: <stable@kernel.org>" in the changelog.  Please, if you think a
patch should go into -stable then add the tag to the changelog also -
this is more reliable and explicit than spraying things at a mailing
list.

You cc'ed stable@kernel.org on all three patches but IMO only [2/3]
(this patch) is needed in -stable.

And afaict, the bug which this patch fixes will allow a suitably-nasty
unprivileged application to leak an unbounded number of task-structs,
which is a box-killing local DoS.  So yes, -stable wants this.

Re: [PATCH 2/3] posix_timer: fix error path in timer_create

[Thomas Gleixner]

On Thu, 20 May 2010, Andrew Morton wrote:

> On Mon, 17 May 2010 21:18:07 +0400
> Andrey Vagin <avagin@openvz.org> wrote:
> 
> > move CLOCK_DISPATCH(which_clock, timer_create, (new_timer)) after all
> > posible EFAULT erros.
> > 
> > *_timer_create may allocate/get resources.
> > (for example posix_cpu_timer_create does get_task_struct)
> > 
> 
> You've added a Cc:stable@kernel.org to the mail headers, but there is
> no "Cc: <stable@kernel.org>" in the changelog.  Please, if you think a
> patch should go into -stable then add the tag to the changelog also -
> this is more reliable and explicit than spraying things at a mailing
> list.
> 
> You cc'ed stable@kernel.org on all three patches but IMO only [2/3]
> (this patch) is needed in -stable.
> 
> And afaict, the bug which this patch fixes will allow a suitably-nasty
> unprivileged application to leak an unbounded number of task-structs,
> which is a box-killing local DoS.  So yes, -stable wants this.

I'm adding it to 2/3 only. Have that lot queued up already.

Thanks,

	tglx