From owner-freebsd-hackers@FreeBSD.ORG Sun Oct 5 18:46:26 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DA546FC4 for ; Sun, 5 Oct 2014 18:46:26 +0000 (UTC) Received: from mail-wi0-x230.google.com (mail-wi0-x230.google.com [IPv6:2a00:1450:400c:c05::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6F115B0E for ; Sun, 5 Oct 2014 18:46:26 +0000 (UTC) Received: by mail-wi0-f176.google.com with SMTP id hi2so2733554wib.15 for ; Sun, 05 Oct 2014 11:46:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=Mxg0+r09NCWD/QiGv7cIUthtQR+4rVCeg9yuS5u9lqw=; b=ezr/HSfHqY0+mupAMR1Z13t1f9qsttHIaXjDOAguEf9jU6iA2xkpChwXkerdUrBT3s jtgWgxxTyw2uplCxW6SEfdnIcM3PiRbrvgHSoBbyGravB0v+38IlIHwBB7mo0PcnxFmw 9L6sDLNPZiFaH6T5AdKwZyl7K1pd5oQqMXhIqFS1TA7oCRvf8nGvg8z8l3rmvQAqpXiS X7vUgKOxto2ETqL64AneiVtWBNjHA/MCsrckMYnA8QzxmgL/xqwNT9jfO1HuV7IgDXtZ 0L0MGGCflu+8K87i7T/EQAVMYiv+e1WxOGI3RFx7VxVwmRfriHQ2yXWb1m6/Kgo+b9v2 vf3A== X-Received: by 10.194.3.42 with SMTP id 10mr24043860wjz.98.1412534784722; Sun, 05 Oct 2014 11:46:24 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id i1sm14733608wjx.32.2014.10.05.11.46.23 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 05 Oct 2014 11:46:23 -0700 (PDT) Date: Sun, 5 Oct 2014 20:46:21 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: fork: hold newly created processes Message-ID: <20141005184620.GC9262@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , freebsd-hackers@freebsd.org References: <20141005102912.GB9262@dft-labs.eu> <20141005171457.GA26076@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20141005171457.GA26076@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 05 Oct 2014 18:46:27 -0000 On Sun, Oct 05, 2014 at 08:14:58PM +0300, Konstantin Belousov wrote: > On Sun, Oct 05, 2014 at 12:29:12PM +0200, Mateusz Guzik wrote: > > fork: hold newly created processes > > > > Consumers of fork1 -> do_fork receive new proc pointer, but nothing > > guarnatees its stability at that time. > > > > New process could already exit and be waited for, in which case we get a > > use after free. > Since the new process is the child of the current process, it can happen > only if the code is self-inflicting. I can imagine that the only way > to achieve it, do wait() in other thread. > Yes, the patch in question is an anti local dos measure. > That said, there is no harm for the kernel state, since struct proc is > type-stable, so we do not dereference a random memory, do you agree ? > We could return non-existing or reused pid, but this can occur with > your patch as well, since the same exit/wait could be done while forking > thread executes syscall return code. Pinning the process with PHOLD means *fork will always return the right pid. Of course the child could be gone by the time fork returns, but this is not a problem. In fork1 you can find: do_fork(td, flags, newproc, td2, vm2, pdflags); /* * Return child proc pointer to parent. */ *procp = newproc; if (flags & RFPROCDESC) { procdesc_finit(newproc->p_procdesc, fp_procdesc); fdrop(fp_procdesc, td); } racct_proc_fork_done(newproc); return (0); Here nothing guarantees newproc is stable and I managed to provoke a crash with null pointer dereference in procdesc_finit since it got a now cleared up process. I think it is possible it will get a different process, provided someone managed to fork it in the meantime. Also, although I didn't try to provoke anything, linux emulation layer does a lot of work with newly returned proc pointer. -- Mateusz Guzik