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

[bsegall@google.com]

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