Date: Tue, 2 Feb 2016 20:16:35 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-hackers@freebsd.org Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160202181635.GC91220@kib.kiev.ua> In-Reply-To: <20160202175652.GA9812@dft-labs.eu> 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> <20160202175652.GA9812@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 02, 2016 at 06:56:52PM +0100, Mateusz Guzik wrote: > 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. This means that the reader has to guess too much, IMHO. At least, add a note that despite fork_return() stops when the child is traced, it is not enough because ... . > > 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 I.e. td2 is reused and the TDB_STOPATFORK is set by unrelated activity ? You reference the do_fork() code checking TDB_STOPATFORK, and not fork_return(), I guess. > 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. > Well, if TDP_STOPATFORK bit is set, it has the same meaning due to type stability, and eventually the wake up would be performed. It just the unintended sleep waiting for condvar which is problematic and which I agree with.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160202181635.GC91220>