From owner-freebsd-hackers@freebsd.org Tue Feb 2 04:07:54 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 AAC34A978D8 for ; Tue, 2 Feb 2016 04:07:54 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 44FB71C49; Tue, 2 Feb 2016 04:07:54 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x241.google.com with SMTP id 128so483375wmz.3; Mon, 01 Feb 2016 20:07:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=QoBxuQvVEuBRl1hO/OLw7OZjvaPhcWnoiTIkgDJsPuI=; b=jo0+QZ7BV4psQvZhTDK0q84ebRrtIqd6dnfADKmaEebIKZ3J0ukv5KUSHl5JwhykUS qpTgWPLC4FFTWH+M460pcwcoz0Wq3NBek1Itf5uoEpkFH3ez5m/+Ztt2sR9qs6+P93nF VMvJcf4zKdAVvHEOI2wAas0qWoUeOQ1N0+Qf9udSNFHh1QiZrNuTlG2xMi2JzwuBuelC bShrTd8NHHbUrihM5bcmkckovRB0TlaeNgAUIcgyIuSKhOrNi/JHADsvY074jmsbgZfC HRYcPKcupa2ZTrKkgLOPNZ1T+0rMt5OHL8v3b6qL26ZdpzF40KsD+MefptpCU2dVXWP7 BvAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=QoBxuQvVEuBRl1hO/OLw7OZjvaPhcWnoiTIkgDJsPuI=; b=LLzQWXP9mmlV6j/pSledFojTLr1pbv6u/EX1p5SHw9JoJdEcOzTszHdswDU8zjdE2J 1B+vqIdu85Q9cGgezpwihZ8Gyxdfb3b1OBR9CL40wIa5t5O0OGKGoCZOL5wzj0sxHTWA HD/iq5AhxWXkmLlenz27WrMfnrMoPhg/kdWobqP6rv9LIL4XY25ZIvAg7o9Rru1s/3KQ J7UqP3Tz0XeQkSfMmogfy0UX50OL1DTAhCjSyGvbXF7tbEwMnpTcfVtR/LaeRKlBeSL/ 4UZ++fO0oEUKpLsWnMBBWB2QyYDzN9jRIt8/UZJguzBVJJpPeqfQWzsqgh3L7vPWjW5F i/bg== X-Gm-Message-State: AG10YOTOkx5jxMS3DueOB29ec9+cuZYK0Em/px3SOhzervbP1q5wzxlzg/MrnpsZJyO9Og== X-Received: by 10.194.20.5 with SMTP id j5mr25863534wje.71.1454386071816; Mon, 01 Feb 2016 20:07:51 -0800 (PST) Received: from mguzik.localdomain (ip-62-245-66-110.net.upcbroadband.cz. [62.245.66.110]) by smtp.gmail.com with ESMTPSA id n131sm550333wmf.9.2016.02.01.20.07.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Feb 2016 20:07:51 -0800 (PST) From: Mateusz Guzik To: kib@freebsd.org Cc: freebsd-hackers@freebsd.org, Mateusz Guzik Subject: Re: [PATCH 0/2] plug fork use-after-free Date: Tue, 2 Feb 2016 05:07:47 +0100 Message-Id: <1454386069-29657-1-git-send-email-mjguzik@gmail.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <20160201103632.GL91220@kib.kiev.ua> References: <20160201103632.GL91220@kib.kiev.ua> 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: Tue, 02 Feb 2016 04:07:54 -0000 From: Mateusz Guzik 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 > > > > 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 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