[PATCH 0/2] plug fork use-after-free

[Konstantin Belousov]

On Mon, Feb 01, 2016 at 06:13:02AM +0100, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg at freebsd.org>
> 
> Quit some time ago I reported a problem with fork and provided a half-assed
> patch, see:
> https://lists.freebsd.org/pipermail/freebsd-hackers/2014-October/046212.html
> 
> Now I got around to fixing the problem in a less hackish manner.
> 
> Note that despite the new process possibly immediatley exiting and being
> waited on, returning its (possibly now reused PID) is fine - that's the
> pid it possibly saw by other means and in worst case the process is racing
> with itself.
> 
> To reiterate, as it is, the code has use-after-free in procdesc and racct
> handling.
> 
> The first patch is a small cleanup to reduce the number of arguments to
> fork1, which was getting out of hand. I don't feel strongly about the
> name of the structure used in there.
> 
> Mateusz Guzik (2):
>   fork: move procdesc-related parameters into a dedicated struct
>   fork: plug a use after free of the returned process pointer
> 
>  sys/compat/cloudabi/cloudabi_proc.c |  11 ++--
>  sys/compat/linux/linux_fork.c       |   6 +-
>  sys/kern/init_main.c                |   2 +-
>  sys/kern/kern_fork.c                | 125 ++++++++++++++++++++----------------
>  sys/kern/kern_kthread.c             |   2 +-
>  sys/sys/proc.h                      |   5 +-
>  sys/sys/procdesc.h                  |   6 ++
>  7 files changed, 91 insertions(+), 66 deletions(-)

I agree with the fix, but I want the approach to be pushed further.

First, please pack all arguments to fork1() into the struct. I think
everything except the curthread pointer should be packed into the
argument structure. You have to touch all fork1() callers anyway, and
with the structure approach you could avoid doing the second pass over
the all callers (in the second patch), esp. if the structure is bzeroed
before being filled.

Second, it puzzles me that do_fork() takes both the p2 and
procp arguments. Wouldn't it be cleaner to assign to *procp (or
fork_req->procp) in fork1 ? I understand why this cannot be done with
*procpid.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20160201/28a7c4a6/attachment.sig>