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: <20110510132001.GA21834@redhat.com>
Date:	Tue, 10 May 2011 15:20:01 +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/10, Tejun Heo wrote:
>
> On Mon, May 09, 2011 at 06:18:38PM +0200, Oleg Nesterov wrote:
>
> > > @@ -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.
>
> Hmmm... if killed, the tracee would go through __ptrace_unlink() which
> always clears JOBCTL_TRAP_MASK which includes JOBCTL_TRAP_SEIZE.  What
> am I missing?

No. If killed, the tracee becomes a zombie (but see below). __ptrace_unlink()
won't be called until the tracee does wait().

> > > @@ -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.
>
> Argh... right it has an early exit path there.

Yes, and thus the tracee will loop forever until the tracer detaches, so
it can't even become a zombie. And the tracer can't detach via ptrace().

> > I never understood why ptrace_stop()->sigkill_pending() logic, I think
> > we should check fatal_signal_pending() unconditionally.
>
> Probably the best way to do it is adding fatal_signal_pending() into
> may_ptrace_stop() so that the failure path shares most of the code
> path instead of doing quick dirty exit?  It's just nasty to have early
> exit above there and walking through the normal code path wouldn't
> hurt SIGKILL either.

Yes, probably. But, apart from the user-visible change which needs another
discussion, there are more problems. fatal_signal_pending() can "wrongly"
return false if the caller is tracehook_report_exit(). OTOH, it can correctly
return true (exec), but we probably want to stop. Let's discuss this later,
I think this has nothing to do with your patches. The main problem is not
fatal_signal_pending(), we need the better definition of explicit SIGKILL
behaviour. Not for ptrace only, there are other issues.

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/