linux-kernel - Re: [PATCH] Discard notification signals when a tracer exits

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: <20080325163316.GA176@tv-sign.ru>
Date:	Tue, 25 Mar 2008 19:33:16 +0300
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Petr Tesarik <ptesarik@...e.cz>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Roland McGrath <roland@...hat.com>
Subject: Re: [PATCH] Discard notification signals when a tracer exits

On 03/25, Oleg Nesterov wrote:
>
> This patch needs Roland's opinion. I can't really judge, but I
> have some (perhaps wrong) doubts.
> 
> On 03/25, Petr Tesarik wrote:
> >
> > When a tracer exits without detaching from the traced process, the
> > tracee may be at a tracer notification stop and will thus interpret
> > the value in task->exit_code (SIGTRAP | 0x80) as the signal to be
> > delivered.
> > 
> > This patch fixes the problem by clearing exit_code when detaching
> > the traced process from a dying tracer.
> > 
> > Signed-off-by: Petr Tesarik <ptesarik@...e.cz>
> > 
> > ---
> >  exit.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -642,8 +642,10 @@ reparent_thread(struct task_struct *p, s
> >  			/*
> >  			 * If it was at a trace stop, turn it into
> >  			 * a normal stop since it's no longer being
> > -			 * traced.
> > +			 * traced.  Cancel the notification signal,
> > +			 * or the tracee may get a SIGTRAP.
> >  			 */
> > +			p->exit_code = 0;
> >  			ptrace_untrace(p);
> >  		}
> >  	}
> > @@ -713,6 +715,10 @@ static void forget_original_parent(struc
> >  			p->real_parent = reaper;
> >  			reparent_thread(p, father, 0);
> >  		} else {
> > +			/* cancel the notification signal at a trace stop */
> > +			if (p->state == TASK_TRACED)
> > +				p->exit_code = 0;
> 
> This reduce the likelihood that the tracee will be SIGTRAP'ed, but doesn't
> solve the problem, no?
> 
> Suppose that the tracee does send_sigtrap(current) in do_syscall_trace()
> and then ptracer exits. Or ptracer wakes up the TASK_TRACED tracee without
> clearing its ->exit_code and then you kill(ptracer, SIGKILL).
> 
> If we really need this, _perhaps_ it is better to change do_syscall_trace(),
> so that the tracee checks ->ptrace before sending the signal to itself.
> 
> 
> But actually, I don't understand what is the problem. Ptracer has full control,
> you should not kill it with SIGKILL, this may leave the child in some bad/
> inconsistent change.

Additional note. Suppose that the tracee dequeues the "good" signal, notices
PT_PTRACED and calls ptrace_stop(). We set TASK_TRACED under ->siglock, without
holding tasklist_lock. At this moment you kill strace, it clears ->exit_code.
The tracee notices it is not traced any longer and returns to get_signal_to_deliver().
Since ->exit_code is cleared, the "right" signal is lost.

So I think this patch adds a race. In some sense (yes I am biased) this is
even worse than the problem this patch tries to solve, because this race
is unlikely and is hard to trigger/debug, and it could be easily unnoticed.

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/