Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 2 Feb 2016 18:56:52 +0100
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Konstantin Belousov <kib@freebsd.org>
Cc:        freebsd-hackers@freebsd.org
Subject:   Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer
Message-ID:  <20160202175652.GA9812@dft-labs.eu>
In-Reply-To: <20160202132322.GU91220@kib.kiev.ua>
References:  <20160201103632.GL91220@kib.kiev.ua> <1454386069-29657-1-git-send-email-mjguzik@gmail.com> <1454386069-29657-3-git-send-email-mjguzik@gmail.com> <20160202132322.GU91220@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 02, 2016 at 03:23:22PM +0200, Konstantin Belousov wrote:
> On Tue, Feb 02, 2016 at 05:07:49AM +0100, Mateusz Guzik wrote:
> > +	flags = fr->fr_flags;
> Why not use fr->fr_flags directly ?  It is slightly more churn, but IMO
> it is worth it.
> 

I'm indiffernet on this one, can change it no problem.

> > +	/*
> > +	 * Hold the process so that it cannot exit after we make it runnable,
> > +	 * but before we wait for the debugger.
> Is this possible ?  The forked child must execute through fork_return(),
> and there we do ptracestop() before the child has a chance to ever return
> to usermode.
> 
> Do you mean a scenario where the debugger detaches before child executes
> fork_return() and TDP_STOPATFORK is cleared in advance ?
> 

The comment is somewhat misworded and I forgot to update it, how about
just stating we hold the process so that we can mark the thread runnable
and not have it disappear under we are done.

While I have not tested this particular bug, prior to the patch the
following should possible: p2 is untraced and td2 is marked as runnable,
after which it exits and p2 is automatically reaped. If the code reaches
the TDB_STOPATFORK check after that, PROC_LOCK(p2) succeeds due to
processes being type stable. td2 dereference can cause no issues due to
threads being type stable as well. But the thread could have been resued
in a traced process, thus inducing cv_wait(&p2->p_dbgwait, ..) even
though td2 is not linked in p2 anymore and p2 is not even a valid
process, making curthread wait indefinitely since there is nobody to
wake it up.

-- 
Mateusz Guzik <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160202175652.GA9812>