Date: Mon, 1 Feb 2016 12:36:32 +0200 From: Konstantin Belousov <kib@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-hackers@freebsd.org, kib@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 0/2] plug fork use-after-free Message-ID: <20160201103632.GL91220@kib.kiev.ua> In-Reply-To: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> References: <1454303584-20941-1-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--BwCQnh7xodEAoBMC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 01, 2016 at 06:13:02AM +0100, Mateusz Guzik wrote: > From: Mateusz Guzik <mjg@freebsd.org> >=20 > Quit some time ago I reported a problem with fork and provided a half-ass= ed > patch, see: > https://lists.freebsd.org/pipermail/freebsd-hackers/2014-October/046212.h= tml >=20 > Now I got around to fixing the problem in a less hackish manner. >=20 > 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. >=20 > To reiterate, as it is, the code has use-after-free in procdesc and racct > handling. >=20 > 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. >=20 > Mateusz Guzik (2): > fork: move procdesc-related parameters into a dedicated struct > fork: plug a use after free of the returned process pointer >=20 > 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. --BwCQnh7xodEAoBMC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWrzUwAAoJEJDCuSvBvK1BSYgQAIra0v0O2Bn2lfFg1dtFB4zH d3SrF9tuhQY2F/Cg/imPFC5Fhgce7eum0UfLznjiYSxks2tC3hdhEKe4T3mGTIT8 3r19zuwv6bGAprLHK7uzBJ0VU+Vy03FFWVCgeIm4XUtECTbOGqLho6iFklh4pA3W uKJ8YhTa6hoVLx90/8V0Gjp7Nmo5THZsQpLAD3NfLs3SKAeW4hy6Imiue804T3r/ b/u0dDBH1b/rW+v8VggvmVL8TGcyqAU8+11C41Lbrdy0pEeiA3DechBa5+2KClrB PKbgt0jqLXr8MwtdwjbqYdtFv66HCnzYWnt5IBEvkpodFyWVYH0CtAGO87s7rJ+2 6uS22OqgPhebd4Fwq2kyP0W6y3l1yt3nE6GJNnR/Srgx4JzXIkn8is1t7keqVhuH 1PeWlFC3UYet5xuKHNU2ejJOnKtSKk4xMHbDQYa7u7IEAWD5NHDW3eSQ3kPIwjNe Q1wq09FkqBnji4GYw3nWiEOeZL83PuwThC9pttbvkHe6LXbrBnPm4e4Ap7HTtY+A +8+sDThZ56nCeL88jbZ9cO5Y3Cdj/tXPuvmRHQzWGl3BDNNjZgnUHgDLcGRerlgw f/+p0uQ6F7amd0541PfgddVctAEAcXvldzXfUveAOfhnAKXT8OtDv43Qe90Pr1E2 7FZNDq1TXI6YtxCr1AHO =sKWa -----END PGP SIGNATURE----- --BwCQnh7xodEAoBMC--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160201103632.GL91220>