fork: hold newly created processes

[Konstantin Belousov]

On Sun, Oct 05, 2014 at 08:46:21PM +0200, Mateusz Guzik wrote:
> On Sun, Oct 05, 2014 at 08:14:58PM +0300, Konstantin Belousov wrote:
> > On Sun, Oct 05, 2014 at 12:29:12PM +0200, Mateusz Guzik wrote:
> > > fork: hold newly created processes
> > > 
> > > Consumers of fork1 -> do_fork receive new proc pointer, but nothing
> > > guarnatees its stability at that time.
> > > 
> > > New process could already exit and be waited for, in which case we get a
> > > use after free.
> > Since the new process is the child of the current process, it can happen
> > only if the code is self-inflicting.  I can imagine that the only way
> > to achieve it, do wait() in other thread.
> > 
> 
> Yes, the patch in question is an anti local dos measure.
> 
> > That said, there is no harm for the kernel state, since struct proc is
> > type-stable, so we do not dereference a random memory, do you agree ?
> > We could return non-existing or reused pid, but this can occur with
> > your patch as well, since the same exit/wait could be done while forking
> > thread executes syscall return code.
> 
> Pinning the process with PHOLD means *fork will always return the right
> pid.
No, because, as you noted, the process may be already consumed by wait.
And pid theoretically can be reused.

> 
> Of course the child could be gone by the time fork returns, but this is
> not a problem.
> 
> In fork1 you can find:
> 	do_fork(td, flags, newproc, td2, vm2, pdflags);
> 
> 	/*
> 	 * Return child proc pointer to parent.
> 	 */
> 	*procp = newproc;
> 	if (flags & RFPROCDESC) {
> 		procdesc_finit(newproc->p_procdesc, fp_procdesc);
> 		fdrop(fp_procdesc, td);
> 	}
> 	racct_proc_fork_done(newproc);
> 	return (0);
> 
> Here nothing guarantees newproc is stable and I managed to provoke a crash
> with null pointer dereference in procdesc_finit since it got a now
> cleared up process.
Why not just move the two statements above into some place in the
do_fork() where the p2 is not yet put on the runqueue ?  This would
avoid the need of all callers of fork* to care about hold.

The racct_proc_fork_done() could be significantly improved by
requiring the locked child, instead of locking it itself.

If the 'correct' (for very vague definition of correct) pid is considered
critical, td_retval[0] should be set also there.

> 
> I think it is possible it will get a different process, provided someone
> managed to fork it in the meantime.
> 
> Also, although I didn't try to provoke anything, linux emulation layer
> does a lot of work with newly returned proc pointer.

Linux emulation does not use FreeBSD threads, so the issue probably does
not m