From owner-freebsd-hackers@freebsd.org Mon Feb 1 10:36:40 2016 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 090AFA9719B for ; Mon, 1 Feb 2016 10:36:40 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id A8EAE26C; Mon, 1 Feb 2016 10:36:39 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u11AaXoh078184 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Mon, 1 Feb 2016 12:36:33 +0200 (EET) (envelope-from kib@freebsd.org) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u11AaXoh078184 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u11AaXw4078183; Mon, 1 Feb 2016 12:36:33 +0200 (EET) (envelope-from kib@freebsd.org) X-Authentication-Warning: tom.home: kostik set sender to kib@freebsd.org using -f Date: Mon, 1 Feb 2016 12:36:32 +0200 From: Konstantin Belousov To: Mateusz Guzik Cc: freebsd-hackers@freebsd.org, kib@freebsd.org, Mateusz Guzik Subject: Re: [PATCH 0/2] plug fork use-after-free Message-ID: <20160201103632.GL91220@kib.kiev.ua> References: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BwCQnh7xodEAoBMC" Content-Disposition: inline In-Reply-To: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Feb 2016 10:36:40 -0000 --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 >=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--