linux-kernel - Re: [PATCH 02/11] ptrace: implement PTRACE

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]

Message-ID: <20110509161838.GA27473@redhat.com>
Date:	Mon, 9 May 2011 18:18:38 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	jan.kratochvil@...hat.com, vda.linux@...glemail.com,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org, indan@....nu
Subject: Re: [PATCH 02/11] ptrace: implement PTRACE_SEIZE

On 05/08, Tejun Heo wrote:
>
> After PTRACE_SEIZE, tracee will trap.  Which trap will happen isn't
> fixed.  If other trap conditions exist (signal delivery or group
> stop), they might be taken; otherwise, a trap with exit_code SIGTRAP |
> (PTRACE_EVENT_INTERRUPT << 8) is taken.
> guaranteed.

Personally, I think the new behaviour is fine. But, as usual, I'd like
to know what Jan/Denys think.

As for the implementation,

> -static int ptrace_attach(struct task_struct *task)
> +static int ptrace_attach(struct task_struct *task, long request,
> +			 unsigned long flags)
>  {
> +	bool seize = request == PTRACE_SEIZE;

Cough. I really hate the cosmetic nits but can't resist...

	bool seize = (request == PTRACE_SEIZE);

looks more parseable, but feel free to ignore.

> @@ -247,6 +272,14 @@ static int ptrace_attach(struct task_struct *task)
>  	if (task_is_stopped(task)) {
>  		task->jobctl |= JOBCTL_STOP_PENDING | JOBCTL_TRAPPING;
>  		signal_wake_up(task, 1);
> +	} else if (seize) {
> +		/*
> +		 * Otherwise, SEIZE uses jobctl trap to put tracee into
> +		 * TASK_TRACED, which doesn't have the nasty side effects
> +		 * of sending SIGSTOP.
> +		 */
> +		task->jobctl |= JOBCTL_TRAP_SEIZE;
> +		signal_wake_up(task, 0);

OK... I am a bit worried we can set JOBCTL_TRAP_SEIZE even if the tracee
was already killed, and if it is killed later JOBCTL_TRAP_SEIZE won't be
cleared. Probably this is fine, ptrace_stop()->schedule() won't sleep in
this case.

Hmm. but see below.

> @@ -1752,12 +1752,13 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
>  	set_current_state(TASK_TRACED);
>
>  	/*
> -	 * We're committing to trapping.  Clearing JOBCTL_TRAPPING and
> -	 * transition to TASK_TRACED should be atomic with respect to
> -	 * siglock.  This should be done after the arch hook as siglock is
> -	 * released and regrabbed across it.
> +	 * We're committing to trapping.  Adjust ->jobctl.  Updates to
> +	 * these flags and transition to TASK_TRACED should be atomic with
> +	 * respect to siglock.  This should be done after the arch hook as
> +	 * siglock may be released and regrabbed across it.
>  	 */
>  	task_clear_jobctl_trapping(current);
> +	current->jobctl &= ~JOBCTL_TRAP_SEIZE;

Yes. But, it seems, this is too late.

Suppose that the JOBCTL_TRAP_SEIZE tracee was SIGKILLED before it reports
PTRACE_EVENT_INTERRUPT. Now, if arch_ptrace_stop_needed() == T, ptrace_stop()
returns without clearing JOBCTL_TRAP_SEIZE/TIF_SIGPENDING. This means
get_signal_to_deliver() will loop forever.

I never understood why ptrace_stop()->sigkill_pending() logic, I think
we should check fatal_signal_pending() unconditionally. Oh, and we have
other subtle issues here.

>  	for (;;) {
>  		struct k_sigaction *ka;
> +
> +		/*
> +		 * Check for ptrace trap conditions.  Jobctl traps are used
> +		 * to trap ptracee while staying transparent regarding
> +		 * signal and job control.
> +		 */
> +		if (unlikely(current->jobctl & JOBCTL_TRAP_MASK)) {
> +			ptrace_notify_locked(SIGTRAP |
> +					     (PTRACE_EVENT_INTERRUPT << 8));
> +			continue;

Shouldn't we recheck SIGNAL_CLD_MASK after ptrace_notify_locked() returns?
Probably not, but I am not sure...

In any case. This doesn't really matter, but can't we check JOBCTL_TRAP_MASK
outside of the main loop? Unless we drop ->siglock this bit can't be changed,
and every time we drop ->siglock we go to "relock".

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/