fork: fix use-after-free with vfork

[mjg]

Summary

The pointer to the child is stored without any reference held. Then it is blindly used to wait until P_PPWAIT is cleared. However, if the child is autoreaped it could have exited and get freed before the parent started waiting.

Use the existing hold mechanism to mitigate the problem. Most common case of doing exec remains unchanged. The corner case of doing exit performs the wake up before waiting for the holds to clear.

Side note is that the current code can be refactored, which I'll do later. There is no need to hold the process if vfork is not used. This lets us avoid proc lock/unlock cycle by the end of do_fork.

Note the probem is very old. It predates similar fixed for other problems in the area made few years ago.

Test Plan

buildkernel, buildworld and others

[kib]

sys/kern/kern_exit.c

290 ↗

(On Diff #50909)

Why this chunk is needed ? We clear P_PPWAIT earlier, and do cv_broadcast() unconditionally later.

[mjg]

sys/kern/kern_exit.c

290 ↗

(On Diff #50909)

In the worst case the ref kept by the parent with _PHOLD will make this thread msleep due to p_lock > 0. So the broadcast done after clearing the flag convinces the parent to release the hold. I found this to be the easiest way to prevent the proc from disappearing (exit + reap) before vforking parent is done with it.

[kib]

sys/kern/kern_exit.c

290 ↗

(On Diff #50909)

But then, you should just move the broadcast call from below to this position, and do it uncoditionally. I do not see a reason to do two wakeups.

[mjg]

• remove now spurious cv_broadcast(&p->p_pwait);

I was somehow convinced there is a ptrace-related user. The syscall path for vfork is the only user of this condvar.

[kib]

sys/kern/kern_exit.c

287 ↗

(On Diff #50931)

There should be a block comment right above this line, explaining that the wakeup must be done before draining the hold count.

Style requires ((p->p_flags & P_PPWAIT) != 0 notation.

343 ↗

(On Diff #50931)

Why do you keep P_PPWAIT cleaning there ? I do not think that the flag can be set at this point.