Date: Tue, 2 Feb 2016 22:44:27 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@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: <20160202214427.GB9812@dft-labs.eu> In-Reply-To: <20160202181635.GC91220@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> <20160202175652.GA9812@dft-labs.eu> <20160202181635.GC91220@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 02, 2016 at 08:16:35PM +0200, Konstantin Belousov wrote:
> 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.
CPU0 is executing fork1. p2 is not traced.
CPU0 CPU1
p2 and td2 created
td2 is marked runnable
td2 is scheduled here
td2 does not have TDB_STOPATFORK set
td2 exits
p2 is autoreaped
td2's space is reused
td2 gets linked into p3
td2 gets TDB_STOPATFORK
PROC_LOCK(p2);
TDB_STOPATFORK test on td2
cv_wait(&p2->p_dbgwait, ..);
So at this point p2 has no linked threads and is free. td2 belongs to
p3 and p2 is waiting for a wakeup which can't happen.
Now that I look at it this may be broken in an additonal way, which is
not fixed by the patch: what if td2 spawns a new thread and thr_exits?
In this case testing td2 is still invalid. Maybe I'm just getting
paranoid here, I don't have time to properly test this right now. In
worst case should be fixable well enough with FIRST_THREAD_IN_PROC.
How about the following comment around _PHOLD:
We are going to make the main thread runnable. It can quickly exit,
causing the process to be reaped and possibly reused, thus invalidating
our p2 pointer. Protect against this by holding the process, which
postpones the exit.
And if the suspicion gets confimed the following would be added:
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index d0c3837..2a076ed 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -773,6 +773,12 @@ do_fork(struct thread *td, struct fork_req *fr, struct proc *p2, struct thread *
PROC_LOCK(p2);
/*
+ * By the time we got here the thread could have created a new thread
+ * and exited. Reload the main thread to ensure we got the right
+ * pointer.
+ */
+ td2 = FIRST_THREAD_IN_PROC(p2);
+ /*
* Wait until debugger is attached to child.
*/
while ((td2->td_dbgflags & TDB_STOPATFORK) != 0)
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160202214427.GB9812>