From owner-freebsd-hackers@freebsd.org Tue Feb 2 18:16:44 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 437DBA994E4 for ; Tue, 2 Feb 2016 18:16:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) 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 DFD93800 for ; Tue, 2 Feb 2016 18:16:43 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u12IGZZN010619 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Tue, 2 Feb 2016 20:16:35 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u12IGZZN010619 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u12IGZt9010618; Tue, 2 Feb 2016 20:16:35 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 2 Feb 2016 20:16:35 +0200 From: Konstantin Belousov To: Mateusz Guzik Cc: freebsd-hackers@freebsd.org Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160202181635.GC91220@kib.kiev.ua> References: <20160201103632.GL91220@kib.kiev.ua> <1454386069-29657-1-git-send-email-mjguzik@gmail.com> <1454386069-29657-3-git-send-email-mjguzik@gmail.com> <20160202132322.GU91220@kib.kiev.ua> <20160202175652.GA9812@dft-labs.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160202175652.GA9812@dft-labs.eu> User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no 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: Tue, 02 Feb 2016 18:16:44 -0000 On Tue, Feb 02, 2016 at 06:56:52PM +0100, Mateusz Guzik wrote: > On Tue, Feb 02, 2016 at 03:23:22PM +0200, Konstantin Belousov wrote: > > On Tue, Feb 02, 2016 at 05:07:49AM +0100, Mateusz Guzik wrote: > > > + flags = fr->fr_flags; > > Why not use fr->fr_flags directly ? It is slightly more churn, but IMO > > it is worth it. > > > > I'm indiffernet on this one, can change it no problem. > > > > + /* > > > + * Hold the process so that it cannot exit after we make it runnable, > > > + * but before we wait for the debugger. > > Is this possible ? The forked child must execute through fork_return(), > > and there we do ptracestop() before the child has a chance to ever return > > to usermode. > > > > Do you mean a scenario where the debugger detaches before child executes > > fork_return() and TDP_STOPATFORK is cleared in advance ? > > > > The comment is somewhat misworded and I forgot to update it, how about > just stating we hold the process so that we can mark the thread runnable > and not have it disappear under we are done. This means that the reader has to guess too much, IMHO. At least, add a note that despite fork_return() stops when the child is traced, it is not enough because ... . > > While I have not tested this particular bug, prior to the patch the > following should possible: p2 is untraced and td2 is marked as runnable, > after which it exits and p2 is automatically reaped. If the code reaches > the TDB_STOPATFORK check after that, PROC_LOCK(p2) succeeds due to I.e. td2 is reused and the TDB_STOPATFORK is set by unrelated activity ? You reference the do_fork() code checking TDB_STOPATFORK, and not fork_return(), I guess. > processes being type stable. td2 dereference can cause no issues due to > threads being type stable as well. But the thread could have been resued > in a traced process, thus inducing cv_wait(&p2->p_dbgwait, ..) even > though td2 is not linked in p2 anymore and p2 is not even a valid > process, making curthread wait indefinitely since there is nobody to > wake it up. > Well, if TDP_STOPATFORK bit is set, it has the same meaning due to type stability, and eventually the wake up would be performed. It just the unintended sleep waiting for condvar which is problematic and which I agree with.