linux-kernel - Re: [PATCH v2 1/5] sched,signal,ptrace: Rework TASK_TRACED, TASK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Apr 2022 18:34:09 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     rjw@...ysocki.net, oleg@...hat.com, mingo@...nel.org,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, mgorman@...e.de, bigeasy@...utronix.de,
        Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org,
        tj@...nel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 1/5] sched,signal,ptrace: Rework TASK_TRACED,
 TASK_STOPPED state

Peter Zijlstra <peterz@...radead.org> writes:

> Currently ptrace_stop() / do_signal_stop() rely on the special states
> TASK_TRACED and TASK_STOPPED resp. to keep unique state. That is, this
> state exists only in task->__state and nowhere else.
>
> There's two spots of bother with this:
>
>  - PREEMPT_RT has task->saved_state which complicates matters,
>    meaning task_is_{traced,stopped}() needs to check an additional
>    variable.
>
>  - An alternative freezer implementation that itself relies on a
>    special TASK state would loose TASK_TRACED/TASK_STOPPED and will
>    result in misbehaviour.
>
> As such, add additional state to task->jobctl to track this state
> outside of task->__state.
>
> NOTE: this doesn't actually fix anything yet, just adds extra state.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -770,7 +773,9 @@ void signal_wake_up_state(struct task_st
>  	 * By using wake_up_state, we ensure the process will wake up and
>  	 * handle its death signal.
>  	 */
> -	if (!wake_up_state(t, state | TASK_INTERRUPTIBLE))
> +	if (wake_up_state(t, state | TASK_INTERRUPTIBLE))
> +		t->jobctl &= ~(JOBCTL_STOPPED | JOBCTL_TRACED);
> +	else
>  		kick_process(t);
>  }

This hunk is subtle and I don't think it is actually what we want if the
code is going to be robust against tsk->__state becoming TASK_FROZEN.

I think we want the clearing of JOBCTL_STOPPED and JOBCTL_TRACED
to be independent of what tsk->__state and tsk->saved_state are.

Something like:

static inline void signal_wake_up(struct task_struct *t, bool resume)
{
	unsigned int state = 0;
	if (resume && !(t->jobctl & JOBCTL_DELAY_WAKEKILL)) {
		t->jobctl &= ~(JOBCTL_STOPPED | JOBCTL_TRACED);
		state = TASK_WAKEKILL;
	}
	signal_wake_up_state(t, state);
}

static inline void ptrace_signal_wake_up(struct task_struct *t, bool resume)
{
	unsigned int state = 0;
	if (resume) {
		t->jobctl &= ~JOBCTL_TRACED;
		state = __TASK_TRACED;
	}
	signal_wake_up_state(t, state);
}

That would allow __set_task_special in the final patch to look like:

/*
 * The special task states (TASK_STOPPED, TASK_TRACED) keep their canonical
 * state in p->jobctl. If either of them got a wakeup that was missed because
 * TASK_FROZEN, then their canonical state reflects that and the below will
 * refuse to restore the special state and instead issue the wakeup.
 */
static int __set_task_special(struct task_struct *p, void *arg)
{
        unsigned int state = 0;

	if (p->jobctl & JOBCTL_TRACED)
        	state = TASK_TRACED;

	else if (p->jobctl & JOBCTL_STOPPED)
		state = TASK_STOPPED;

	if (state)
		WRITE_ONCE(p->__state, state);

	return state;
}


With no need to figure out if a wake_up was dropped and reverse engineer
what the wakeup was.

Eric