Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585620 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state bsegall@google.com - 2017-02-21 19:50 +0100
    Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state Oleg Nesterov <oleg@redhat.com> - 2017-02-22 18:00 +0100
      Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state Oleg Nesterov <oleg@redhat.com> - 2017-02-22 18:20 +0100
      Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state bsegall@google.com - 2017-02-22 19:00 +0100
    Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state Oleg Nesterov <oleg@redhat.com> - 2017-02-24 17:40 +0100
      Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state bsegall@google.com - 2017-02-27 19:10 +0100

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

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

[toc] | [next] | [standalone]

#1586315

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.

[toc] | [prev] | [next] | [standalone]

#1586322

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.

[toc] | [prev] | [next] | [standalone]

#1586352

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.

[toc] | [prev] | [next] | [standalone]

#1587779

(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

[toc] | [prev] | [next] | [standalone]

#1588843

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web