Date: Tue, 2 Feb 2016 05:07:47 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: kib@freebsd.org Cc: freebsd-hackers@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 0/2] plug fork use-after-free Message-ID: <1454386069-29657-1-git-send-email-mjguzik@gmail.com> In-Reply-To: <20160201103632.GL91220@kib.kiev.ua> References: <20160201103632.GL91220@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
From: Mateusz Guzik <mjg@freebsd.org> On Mon, Feb 01, 2016 at 12:36:32PM +0200, Konstantin Belousov wrote: > On Mon, Feb 01, 2016 at 06:13:02AM +0100, Mateusz Guzik wrote: > > From: Mateusz Guzik <mjg@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. > > > > 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. Done. There is a local 'int pid' var passed around, I can change that to pass use td_retval[0]. > > 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. I would say it's cleaner to keep both assignments close, but don't care that much. In general, as was disussed some time ago the code should be resturctured anyay to not put PRS_NEW processes on the list and once that happens both assignments will likely be handled prior to enterding do_fork. -- Mateusz Guzik <mjguzik gmail.com> Mateusz Guzik (2): fork: pass arguments to fork1 in a dedicated structure fork: plug a use after free of the returned process pointer sys/compat/cloudabi/cloudabi_proc.c | 7 +- sys/compat/linux/linux_fork.c | 17 +++-- sys/kern/init_main.c | 6 +- sys/kern/kern_fork.c | 134 +++++++++++++++++++++--------------- sys/kern/kern_kthread.c | 7 +- sys/kern/kern_racct.c | 3 +- sys/sys/proc.h | 13 +++- 7 files changed, 117 insertions(+), 70 deletions(-) -- 2.7.0
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1454386069-29657-1-git-send-email-mjguzik>