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

[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 ad72342..9ca4973 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -560,11 +560,6 @@ SYSCALL_DEFINE3(timer_create, const clockid_t, which_clock,
 	new_timer->it_clock = which_clock;
 	new_timer->it_overrun = -1;
 
-	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;
@@ -590,6 +585,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;
+	}
+
 	error = CLOCK_DISPATCH(which_clock, timer_create, (new_timer));
 	if (error)
 		goto out;
-- 
1.6.6

Re: [PATCH 1/3] posix_timer: remove wrong comment

[Stanislaw Gruszka]

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

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

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

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 3/3] posix_timer: move copy_to_user(created_timer_id) down in timer_create

[Stanislaw Gruszka]

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

> 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).

I'm not sure what for Oleg want that change, I'm not seeing any value of
it. I think patch should be dropped.

Thanks
Stanislaw

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

[Oleg Nesterov]

On 05/18, Stanislaw Gruszka wrote:
>
> On Mon, 17 May 2010 21:18:08 +0400
> Andrey Vagin <avagin@openvz.org> wrote:
>
> > 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).
>
> I'm not sure what for Oleg want that change, I'm not seeing any value of
> it. I think patch should be dropped.

I didn't mean this change is really needed. I just wanted to clarify
that currently the comment is wrong.

But. Now that we move CLOCK_DISPATCH() down, it becomes correct again:
we report created_timer_id to user-space despite the fact timer_create()
can fail later. This _perhaps_ means it makes sense to preserve the
comment and move the copy_to_user() block down, before CLOCK_DISPATCH(),
just to make the code more readable/understandable.

But I agree with either way you and Andrey prefer. And I believe 2/3
should fix the problem correctly.

Oleg.

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