[PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[bsegall]

In PT_SEIZED + LISTEN mode SIGSTOP/SIGCONT signals cause a wakeup
against __TASK_TRACED. If this races with the ptrace_unfreeze_traced at
the end of a PTRACE_LISTEN, this can wake the task /after/ the check
against __TASK_TRACED, but before the reset of state to TASK_TRACED.
This causes it to instead clobber TASK_WAKING, allowing a subsequent
wakeup against TASK_TRACED while the task is still on the rq wake_list,
corrupting it.

Signed-off-by: Ben Segall <bsegall@google.com>
---
 kernel/ptrace.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0af928712174..852d71440ded 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -184,10 +184,14 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 
        WARN_ON(!task->ptrace || task->parent != current);
 
+       /*
+        * Double check __TASK_TRACED under the lock to prevent corrupting state
+        * in case of a ptrace_trap_notify wakeup
+        */
        spin_lock_irq(&task->sighand->siglock);
        if (__fatal_signal_pending(task))
                wake_up_state(task, __TASK_TRACED);
-       else
+       else if (task->state == __TASK_TRACED)
                task->state = TASK_TRACED;
        spin_unlock_irq(&task->sighand->siglock);
 }
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Oleg Nesterov]

On 02/21, bsegall@google.com wrote:
>
> In PT_SEIZED + LISTEN mode SIGSTOP/SIGCONT signals cause a wakeup
> against __TASK_TRACED. If this races with the ptrace_unfreeze_traced at
> the end of a PTRACE_LISTEN, this can wake the task /after/ the check
> against __TASK_TRACED, but before the reset of state to TASK_TRACED.

Oh, thanks...

note also that PTRACE_LISTEN itself can do ptrace_signal_wake_up(true),

> This causes it to instead clobber TASK_WAKING,

even if it is already TASK_RUNNING it is simply wrong to set TASK_TRACED
in both cases, right?

Thanks. The patch looks good at first glance, but let me think a bit...
perhaps we should change PTRACE_LISTEN instead, not sure.

Oleg.

Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Oleg Nesterov]

On 02/22, Oleg Nesterov wrote:
>
> note also that PTRACE_LISTEN itself can do ptrace_signal_wake_up(true),

please ignore, in this case the __TASK_TRACED at the start of _unfreeze()
saves us.

> 
> > This causes it to instead clobber TASK_WAKING,
> 
> even if it is already TASK_RUNNING it is simply wrong to set TASK_TRACED
> in both cases, right?
> 
> Thanks. The patch looks good at first glance, but let me think a bit...
> perhaps we should change PTRACE_LISTEN instead, not sure.
> 
> Oleg.

Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[bsegall]

Oleg Nesterov <oleg@redhat.com> writes:

> On 02/21, bsegall@google.com wrote:
>>
>> In PT_SEIZED + LISTEN mode SIGSTOP/SIGCONT signals cause a wakeup
>> against __TASK_TRACED. If this races with the ptrace_unfreeze_traced at
>> the end of a PTRACE_LISTEN, this can wake the task /after/ the check
>> against __TASK_TRACED, but before the reset of state to TASK_TRACED.
>
> Oh, thanks...
>
> note also that PTRACE_LISTEN itself can do ptrace_signal_wake_up(true),
>
>> This causes it to instead clobber TASK_WAKING,
>
> even if it is already TASK_RUNNING it is simply wrong to set TASK_TRACED
> in both cases, right?

Yeah, that's also wrong and could possibly lead to different errors, but
is likely to work out by accident when say ttwu checks on_rq and sees true.

>
> Thanks. The patch looks good at first glance, but let me think a bit...
> perhaps we should change PTRACE_LISTEN instead, not sure.
>
> Oleg.

Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Oleg Nesterov]

(add akpm, we usually route ptrace fixes via -mm tree)

On 02/21, bsegall@google.com wrote:
>
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -184,10 +184,14 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
>  
>         WARN_ON(!task->ptrace || task->parent != current);
>  
> +       /*
> +        * Double check __TASK_TRACED under the lock to prevent corrupting state
> +        * in case of a ptrace_trap_notify wakeup
> +        */
>         spin_lock_irq(&task->sighand->siglock);
>         if (__fatal_signal_pending(task))
>                 wake_up_state(task, __TASK_TRACED);
> -       else
> +       else if (task->state == __TASK_TRACED)
>                 task->state = TASK_TRACED;
>         spin_unlock_irq(&task->sighand->siglock);

So yes, I think your patch is fine except the comment should explain that
we need this because PTRACE_LISTEN makes ptrace_trap_notify() possible. And
perhaps it would be better to do the 2nd check before fatal_signal_pending:

	if (task->state == __TASK_TRACED) {
		if (__fatal_signal_pending(task))
			wake_up_state(task, __TASK_TRACED);
		else
			task->state = TASK_TRACED;
	}

just to make the logic more clear. wake_up_state(__TASK_TRACED) can
never hurt if the task is killed, just it doesn't look strictly correct
if the tracee was already woken. But this is minor.



You know, I'd prefer another fix, see below.

Why. ptrace_unfreeze_traced() assumes that - since ptrace_freeze_traced()
checks PTRACE_LISTEN - nobody but us can wake the tracee up. So the
__TASK_TRACED check at the start of ptrace_unfreeze_traced() means that
the tracee is still freezed, it was not woken up by (say) PTRACE_CONT.

IOW, currently we assume that only the caller of ptrace_freeze_traced()
can do the __TASK_TRACED -> WHATEVER transition.

However, as you pointed out, I forgot that JOBCTL_LISTENING set by LISTEN
breaks this assumption, and imo it would be nice to fix this.

What do you think? I won't insist too much if you prefer your simple change.

Oleg.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -174,6 +174,18 @@
 	return ret;
 }
 
+static bool __ptrace_unfreeze_traced(struct task_struct *task)
+{
+	bool killed = __fatal_signal_pending(task);
+
+	if (killed)
+		wake_up_state(task, __TASK_TRACED);
+	else
+		task->state = TASK_TRACED;
+
+	return !killed'
+}
+
 static void ptrace_unfreeze_traced(struct task_struct *task)
 {
 	if (task->state != __TASK_TRACED)
@@ -182,10 +194,7 @@
 	WARN_ON(!task->ptrace || task->parent != current);
 
 	spin_lock_irq(&task->sighand->siglock);
-	if (__fatal_signal_pending(task))
-		wake_up_state(task, __TASK_TRACED);
-	else
-		task->state = TASK_TRACED;
+	__ptrace_unfreeze_traced(task);
 	spin_unlock_irq(&task->sighand->siglock);
 }
 
@@ -993,7 +1002,12 @@
 			break;
 
 		si = child->last_siginfo;
-		if (likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
+		/*
+		 * Once we set JOBCTL_LISTENING we do not own child->state,
+		 * need to unfreeze first.
+		 */
+		if (__ptrace_unfreeze_traced(child) &&
+		    likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
 			child->jobctl |= JOBCTL_LISTENING;
 			/*
 			 * If NOTIFY is set, it means event happened between

Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[bsegall]

Oleg Nesterov <oleg@redhat.com> writes:

> (add akpm, we usually route ptrace fixes via -mm tree)
>
> On 02/21, bsegall@google.com wrote:
>>
>> --- a/kernel/ptrace.c
>> +++ b/kernel/ptrace.c
>> @@ -184,10 +184,14 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
>>  
>>         WARN_ON(!task->ptrace || task->parent != current);
>>  
>> +       /*
>> +        * Double check __TASK_TRACED under the lock to prevent corrupting state
>> +        * in case of a ptrace_trap_notify wakeup
>> +        */
>>         spin_lock_irq(&task->sighand->siglock);
>>         if (__fatal_signal_pending(task))
>>                 wake_up_state(task, __TASK_TRACED);
>> -       else
>> +       else if (task->state == __TASK_TRACED)
>>                 task->state = TASK_TRACED;
>>         spin_unlock_irq(&task->sighand->siglock);
>
> So yes, I think your patch is fine except the comment should explain that
> we need this because PTRACE_LISTEN makes ptrace_trap_notify() possible. And
> perhaps it would be better to do the 2nd check before fatal_signal_pending:
>
> 	if (task->state == __TASK_TRACED) {
> 		if (__fatal_signal_pending(task))
> 			wake_up_state(task, __TASK_TRACED);
> 		else
> 			task->state = TASK_TRACED;
> 	}
>
> just to make the logic more clear. wake_up_state(__TASK_TRACED) can
> never hurt if the task is killed, just it doesn't look strictly correct
> if the tracee was already woken. But this is minor.
>
>
>
> You know, I'd prefer another fix, see below.
>
> Why. ptrace_unfreeze_traced() assumes that - since ptrace_freeze_traced()
> checks PTRACE_LISTEN - nobody but us can wake the tracee up. So the
> __TASK_TRACED check at the start of ptrace_unfreeze_traced() means that
> the tracee is still freezed, it was not woken up by (say) PTRACE_CONT.
>
> IOW, currently we assume that only the caller of ptrace_freeze_traced()
> can do the __TASK_TRACED -> WHATEVER transition.
>
> However, as you pointed out, I forgot that JOBCTL_LISTENING set by LISTEN
> breaks this assumption, and imo it would be nice to fix this.
>
> What do you think? I won't insist too much if you prefer your simple
> change.

My knowledge of the ptrace state machine isn't the best, but this looks
valid to me and doesn't crash

>
> Oleg.
>
> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -174,6 +174,18 @@
>  	return ret;
>  }
>  
> +static bool __ptrace_unfreeze_traced(struct task_struct *task)
> +{
> +	bool killed = __fatal_signal_pending(task);
> +
> +	if (killed)
> +		wake_up_state(task, __TASK_TRACED);
> +	else
> +		task->state = TASK_TRACED;
> +
> +	return !killed'
> +}
> +
>  static void ptrace_unfreeze_traced(struct task_struct *task)
>  {
>  	if (task->state != __TASK_TRACED)
> @@ -182,10 +194,7 @@
>  	WARN_ON(!task->ptrace || task->parent != current);
>  
>  	spin_lock_irq(&task->sighand->siglock);
> -	if (__fatal_signal_pending(task))
> -		wake_up_state(task, __TASK_TRACED);
> -	else
> -		task->state = TASK_TRACED;
> +	__ptrace_unfreeze_traced(task);
>  	spin_unlock_irq(&task->sighand->siglock);
>  }
>  
> @@ -993,7 +1002,12 @@
>  			break;
>  
>  		si = child->last_siginfo;
> -		if (likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
> +		/*
> +		 * Once we set JOBCTL_LISTENING we do not own child->state,
> +		 * need to unfreeze first.
> +		 */
> +		if (__ptrace_unfreeze_traced(child) &&
> +		    likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
>  			child->jobctl |= JOBCTL_LISTENING;
>  			/*
>  			 * If NOTIFY is set, it means event happened between

[PATCHv2] ptrace: fix PTRACE_LISTEN race corrupting task->state

[bsegall]

In PT_SEIZED + LISTEN mode STOP/CONT signals cause a wakeup against
__TASK_TRACED. If this races with the ptrace_unfreeze_traced at the end
of a PTRACE_LISTEN, this can wake the task /after/ the check against
__TASK_TRACED, but before the reset of state to TASK_TRACED. This causes
it to instead clobber TASK_WAKING, allowing a subsequent wakeup against
TRACED while the task is still on the rq wake_list, corrupting it.

Signed-off-by: Ben Segall <bsegall@google.com>
---

v2: slight clarification in comments, put the conditional around the
whole wakeup area

Oleg mentioned a preference for making LISTEN unfreeze instead; I have
no preference there, just wanted to make sure that this doesn't get
forgotten entirely.

 kernel/ptrace.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 0af928712174..7cc49c3e73af 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -184,11 +184,17 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 
        WARN_ON(!task->ptrace || task->parent != current);
 
+       /*
+        * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up
+        * remotely. Recheck state under the lock to close this race.
+        */
        spin_lock_irq(&task->sighand->siglock);
-       if (__fatal_signal_pending(task))
-               wake_up_state(task, __TASK_TRACED);
-       else
-               task->state = TASK_TRACED;
+       if (task->state == __TASK_TRACED) {
+               if (__fatal_signal_pending(task))
+                       wake_up_state(task, __TASK_TRACED);
+               else
+                       task->state = TASK_TRACED;
+       }
        spin_unlock_irq(&task->sighand->siglock);
 }
 
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCHv2] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Andrew Morton]

On Tue, 04 Apr 2017 14:47:34 -0700 bsegall@google.com wrote:

> In PT_SEIZED + LISTEN mode STOP/CONT signals cause a wakeup against
> __TASK_TRACED. If this races with the ptrace_unfreeze_traced at the end
> of a PTRACE_LISTEN, this can wake the task /after/ the check against
> __TASK_TRACED, but before the reset of state to TASK_TRACED. This causes
> it to instead clobber TASK_WAKING, allowing a subsequent wakeup against
> TRACED while the task is still on the rq wake_list, corrupting it.

The changelog doesn't convey the urgency of the fix.  To understand
this we'll need to know the user-visible impact of the bug and the
likelihood of someone hitting it.

Also your suggestion regarding which kernel version(s) should be fixed
(and the reasoning) is always valuable.

Re: [PATCHv2] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Oleg Nesterov]

On 04/04, Andrew Morton wrote:
>
> On Tue, 04 Apr 2017 14:47:34 -0700 bsegall@google.com wrote:
>
> > In PT_SEIZED + LISTEN mode STOP/CONT signals cause a wakeup against
> > __TASK_TRACED. If this races with the ptrace_unfreeze_traced at the end
> > of a PTRACE_LISTEN, this can wake the task /after/ the check against
> > __TASK_TRACED, but before the reset of state to TASK_TRACED. This causes
> > it to instead clobber TASK_WAKING, allowing a subsequent wakeup against
> > TRACED while the task is still on the rq wake_list, corrupting it.
>
> The changelog doesn't convey the urgency of the fix.  To understand
> this we'll need to know the user-visible impact of the bug and the
> likelihood of someone hitting it.

The kernel can crash or this can lead to other hard-to-debug problems.
In short, "task->state = TASK_TRACED" in ptrace_unfreeze_traced() assumes
that nobody else can wake it up, but PTRACE_LISTEN breaks the contract.
Obviusly it is veru wrong to manipulate task->state if this task is already
running, or WAKING, or it sleeps again.

> Also your suggestion regarding which kernel version(s) should be fixed
> (and the reasoning) is always valuable.

This fixes 9899d11f "ptrace: ensure arch_ptrace/ptrace_request can never
race with SIGKILL"

Oleg.

Re: [PATCHv2] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Oleg Nesterov]

On 04/04, bsegall@google.com wrote:
>
> v2: slight clarification in comments, put the conditional around the
> whole wakeup area

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

and I think this should go to -stable.

> Oleg mentioned a preference for making LISTEN unfreeze instead; I have
> no preference there,

Yes, but I won't insist if you prefer this more simple fix,

> just wanted to make sure that this doesn't get
> forgotten entirely.

And you are right, I forgot about this bug. Thanks!

Oleg.

>  kernel/ptrace.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 0af928712174..7cc49c3e73af 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -184,11 +184,17 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
>
>         WARN_ON(!task->ptrace || task->parent != current);
>
> +       /*
> +        * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up
> +        * remotely. Recheck state under the lock to close this race.
> +        */
>         spin_lock_irq(&task->sighand->siglock);
> -       if (__fatal_signal_pending(task))
> -               wake_up_state(task, __TASK_TRACED);
> -       else
> -               task->state = TASK_TRACED;
> +       if (task->state == __TASK_TRACED) {
> +               if (__fatal_signal_pending(task))
> +                       wake_up_state(task, __TASK_TRACED);
> +               else
> +                       task->state = TASK_TRACED;
> +       }
>         spin_unlock_irq(&task->sighand->siglock);
>  }
>
> --
> 2.12.2.715.g7642488e1d-goog
>