Patchwork [Lucid,1/1] Fix ptrace when task is in task_is_stopped(), state

login
register
mail settings
Submitter John Johansen
Date March 21, 2013, 8:57 p.m.
Message ID <514B742E.3010304@canonical.com>
Download mbox | patch
Permalink /patch/229837/
State New
Headers show

Comments

John Johansen - March 21, 2013, 8:57 p.m.
From d6a1da349c76ac2ebe4774d1da9fb7e660df01d3 Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Thu, 21 Mar 2013 05:04:13 -0700
Subject: [PATCH] UBUNTU: SAUCE: Fix ptrace when task is in task_is_stopped()
 state

This patch fixes a regression in ptrace, introduced by commit 9e74eb39
(backport of 9899d11f) which makes assumptions about ptrace behavior
which are not true in the 2.6.32 kernel.

BugLink: http://bugs.launchpad.net/bugs/1145234

9899d11f makes the assumption that task_is_stopped() is not a valid state
in ptrace because it is built on top of a series of patches which change
how the TASK_STOPPED state is tracked (321fb561 which requires d79fdd6d
and several other patches).

Because Lucid does not have the set of patches that make task_is_stopped()
an invalid state in ptrace_check_attach, partially revert 9e74eb39 so
that ptrace_check_attach() correctly handles task_is_stopped(). However
we must replace the assignment of TASK_TRACED with __TASK_TRACED to
ensure TASK_WAKEKILL is cleared.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 kernel/ptrace.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
Colin King - March 21, 2013, 9:18 p.m.
On 21/03/13 20:57, John Johansen wrote:
>  From d6a1da349c76ac2ebe4774d1da9fb7e660df01d3 Mon Sep 17 00:00:00 2001
> From: John Johansen <john.johansen@canonical.com>
> Date: Thu, 21 Mar 2013 05:04:13 -0700
> Subject: [PATCH] UBUNTU: SAUCE: Fix ptrace when task is in task_is_stopped()
>   state
>
> This patch fixes a regression in ptrace, introduced by commit 9e74eb39
> (backport of 9899d11f) which makes assumptions about ptrace behavior
> which are not true in the 2.6.32 kernel.
>
> BugLink: http://bugs.launchpad.net/bugs/1145234
>
> 9899d11f makes the assumption that task_is_stopped() is not a valid state
> in ptrace because it is built on top of a series of patches which change
> how the TASK_STOPPED state is tracked (321fb561 which requires d79fdd6d
> and several other patches).
>
> Because Lucid does not have the set of patches that make task_is_stopped()
> an invalid state in ptrace_check_attach, partially revert 9e74eb39 so
> that ptrace_check_attach() correctly handles task_is_stopped(). However
> we must replace the assignment of TASK_TRACED with __TASK_TRACED to
> ensure TASK_WAKEKILL is cleared.
>
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>   kernel/ptrace.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index d0036f0..d9c8c47 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -81,14 +81,18 @@ void __ptrace_unlink(struct task_struct *child)
>   }
>
>   /* Ensure that nothing can wake it up, even SIGKILL */
> -static bool ptrace_freeze_traced(struct task_struct *task)
> +static bool ptrace_freeze_traced(struct task_struct *task, int kill)
>   {
> -	bool ret = false;
> +	bool ret = true;
>
>   	spin_lock_irq(&task->sighand->siglock);
> -	if (task_is_traced(task) && !__fatal_signal_pending(task)) {
> +	if (task_is_stopped(task) && !__fatal_signal_pending(task))
>   		task->state = __TASK_TRACED;
> -		ret = true;
> +	else if (!kill) {
> +		if (task_is_traced(task) && !__fatal_signal_pending(task))
> +			task->state = __TASK_TRACED;
> +		else
> +			ret = false;
>   	}
>   	spin_unlock_irq(&task->sighand->siglock);
>
> @@ -131,7 +135,7 @@ int ptrace_check_attach(struct task_struct *child, int kill)
>   		 * child->sighand can't be NULL, release_task()
>   		 * does ptrace_unlink() before __exit_signal().
>   		 */
> -		if (kill || ptrace_freeze_traced(child))
> +		if (ptrace_freeze_traced(child, kill))
>   			ret = 0;
>   	}
>   	read_unlock(&tasklist_lock);
>
I believe this fixes the original semantics as intended.  I had to study 
this a bit harder now that kill is passed into ptrace_freeze_traced() 
but from what I can see this does the trick.

Acked-by: Colin Ian King <colin.king@canonical.com>
Luis Henriques - March 22, 2013, 10:11 a.m.
On Thu, Mar 21, 2013 at 01:57:18PM -0700, John Johansen wrote:
> From d6a1da349c76ac2ebe4774d1da9fb7e660df01d3 Mon Sep 17 00:00:00 2001
> From: John Johansen <john.johansen@canonical.com>
> Date: Thu, 21 Mar 2013 05:04:13 -0700
> Subject: [PATCH] UBUNTU: SAUCE: Fix ptrace when task is in task_is_stopped()
>  state
> 
> This patch fixes a regression in ptrace, introduced by commit 9e74eb39
> (backport of 9899d11f) which makes assumptions about ptrace behavior
> which are not true in the 2.6.32 kernel.

Seems to be correct.  It fixes a problem introduced by a backport to fix
CVE-2013-0871.  Nice catch!

Cheers,
--
Luis

> 
> BugLink: http://bugs.launchpad.net/bugs/1145234
> 
> 9899d11f makes the assumption that task_is_stopped() is not a valid state
> in ptrace because it is built on top of a series of patches which change
> how the TASK_STOPPED state is tracked (321fb561 which requires d79fdd6d
> and several other patches).
> 
> Because Lucid does not have the set of patches that make task_is_stopped()
> an invalid state in ptrace_check_attach, partially revert 9e74eb39 so
> that ptrace_check_attach() correctly handles task_is_stopped(). However
> we must replace the assignment of TASK_TRACED with __TASK_TRACED to
> ensure TASK_WAKEKILL is cleared.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  kernel/ptrace.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index d0036f0..d9c8c47 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -81,14 +81,18 @@ void __ptrace_unlink(struct task_struct *child)
>  }
>  
>  /* Ensure that nothing can wake it up, even SIGKILL */
> -static bool ptrace_freeze_traced(struct task_struct *task)
> +static bool ptrace_freeze_traced(struct task_struct *task, int kill)
>  {
> -	bool ret = false;
> +	bool ret = true;
>  
>  	spin_lock_irq(&task->sighand->siglock);
> -	if (task_is_traced(task) && !__fatal_signal_pending(task)) {
> +	if (task_is_stopped(task) && !__fatal_signal_pending(task))
>  		task->state = __TASK_TRACED;
> -		ret = true;
> +	else if (!kill) {
> +		if (task_is_traced(task) && !__fatal_signal_pending(task))
> +			task->state = __TASK_TRACED;
> +		else
> +			ret = false;
>  	}
>  	spin_unlock_irq(&task->sighand->siglock);
>  
> @@ -131,7 +135,7 @@ int ptrace_check_attach(struct task_struct *child, int kill)
>  		 * child->sighand can't be NULL, release_task()
>  		 * does ptrace_unlink() before __exit_signal().
>  		 */
> -		if (kill || ptrace_freeze_traced(child))
> +		if (ptrace_freeze_traced(child, kill))
>  			ret = 0;
>  	}
>  	read_unlock(&tasklist_lock);
> -- 
> 1.8.1.2
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Stefan Bader - March 22, 2013, 10:31 a.m.
On 21.03.2013 21:57, John Johansen wrote:
> From d6a1da349c76ac2ebe4774d1da9fb7e660df01d3 Mon Sep 17 00:00:00 2001
> From: John Johansen <john.johansen@canonical.com>
> Date: Thu, 21 Mar 2013 05:04:13 -0700
> Subject: [PATCH] UBUNTU: SAUCE: Fix ptrace when task is in task_is_stopped()
>  state
> 
> This patch fixes a regression in ptrace, introduced by commit 9e74eb39
> (backport of 9899d11f) which makes assumptions about ptrace behavior
> which are not true in the 2.6.32 kernel.
> 
> BugLink: http://bugs.launchpad.net/bugs/1145234
> 
> 9899d11f makes the assumption that task_is_stopped() is not a valid state
> in ptrace because it is built on top of a series of patches which change
> how the TASK_STOPPED state is tracked (321fb561 which requires d79fdd6d
> and several other patches).
> 
> Because Lucid does not have the set of patches that make task_is_stopped()
> an invalid state in ptrace_check_attach, partially revert 9e74eb39 so
> that ptrace_check_attach() correctly handles task_is_stopped(). However
> we must replace the assignment of TASK_TRACED with __TASK_TRACED to
> ensure TASK_WAKEKILL is cleared.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>


Ok, it seems the logic reverts back to before task_is_stopped was invalid and
since before it waited for TASK_TRACED later on it looks to be sensible to
switch that to __TASK_TRACED which is used now.
Tim Gardner - March 22, 2013, 1:57 p.m.

Patch

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index d0036f0..d9c8c47 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -81,14 +81,18 @@  void __ptrace_unlink(struct task_struct *child)
 }
 
 /* Ensure that nothing can wake it up, even SIGKILL */
-static bool ptrace_freeze_traced(struct task_struct *task)
+static bool ptrace_freeze_traced(struct task_struct *task, int kill)
 {
-	bool ret = false;
+	bool ret = true;
 
 	spin_lock_irq(&task->sighand->siglock);
-	if (task_is_traced(task) && !__fatal_signal_pending(task)) {
+	if (task_is_stopped(task) && !__fatal_signal_pending(task))
 		task->state = __TASK_TRACED;
-		ret = true;
+	else if (!kill) {
+		if (task_is_traced(task) && !__fatal_signal_pending(task))
+			task->state = __TASK_TRACED;
+		else
+			ret = false;
 	}
 	spin_unlock_irq(&task->sighand->siglock);
 
@@ -131,7 +135,7 @@  int ptrace_check_attach(struct task_struct *child, int kill)
 		 * child->sighand can't be NULL, release_task()
 		 * does ptrace_unlink() before __exit_signal().
 		 */
-		if (kill || ptrace_freeze_traced(child))
+		if (ptrace_freeze_traced(child, kill))
 			ret = 0;
 	}
 	read_unlock(&tasklist_lock);