From owner-freebsd-hackers@freebsd.org Thu Feb 4 10:16:01 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 3A98DA9B895 for ; Thu, 4 Feb 2016 10:16:01 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x244.google.com (mail-wm0-x244.google.com [IPv6:2a00:1450:400c:c09::244]) (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 E34EEE60; Thu, 4 Feb 2016 10:16:00 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x244.google.com with SMTP id 128so11438639wmz.3; Thu, 04 Feb 2016 02:16:00 -0800 (PST) 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=sWqVrnC1WgXlqmNAAU7DZmqme94wzXnxFHdffBGXHxM=; b=JJ9WYuqBquP0tzzR/526N1eXgIvf0WR0OTEyDXMXI9X0akmMzLmaoO9qKpzwPp2rM7 JpXpZKYK0ILlTTM7j/j/8qpEanZ3lhq5cUz7HERqCDGc/rjDeAk6ZpjzN1oPwwpojxup q5BXGADCnRBTp/hAeKngDR/gZb/XpEhQ4zmVdChHWKOGtKr8MwCsLa6uXnA1WaBwSe1Z sPkPXsEU1cghqW0EAwbWwEVnWIhbn53ojfN2RSVf9rsF5Qv54HbOIWOfYBIVasCcgW+0 RZBhYZDn52okkgnBH3/XWRcqa5WnVL4Lr9qXx2GMWf2Fti7dzBZPi1qWDvGwd5uWjJxd DWYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent; bh=sWqVrnC1WgXlqmNAAU7DZmqme94wzXnxFHdffBGXHxM=; b=BiI9hNLMpTm+S5CDXqgBlOPXEH30xT8lVKp59a9G1ZczkvrS20BebG8zc6Bf6Y/Oce Lv8f7kFkFzbmk3rJp/3ClXDou2sOipqlTjmDhHv47t4dnj7QfCXAdoc7rVGLt6cl2jFp hUQGrUjPYDU2YZj+pdJdnZqSn8ytyTuWwXWzu0cw8Zfk5bgQIG91pu1WSTRxMdCZ9wF8 1HmlUmxRL7Gme9lXAkx0TAyc81sVRII1xOFneDZ6YMspqWPS8fpvFWg2w01xBpaFfTlB DZYxhXSpR9EcumSQBzxtOC/q5yMCPjdmbwyhmbj0cytYL1fAt5VpswRyGrqsb+1I590n Oobg== X-Gm-Message-State: AG10YOSOYG63ZRpJzZ3xSdnWHk3tVjJBQYH3g0lWH3SqDuGvNk2R9g1+e/9CfceLlNtx2Q== X-Received: by 10.28.4.134 with SMTP id 128mr9006178wme.96.1454580959047; Thu, 04 Feb 2016 02:15:59 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id p9sm10602046wjy.41.2016.02.04.02.15.58 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 04 Feb 2016 02:15:58 -0800 (PST) Date: Thu, 4 Feb 2016 11:15:56 +0100 From: Mateusz Guzik To: Konstantin Belousov Cc: freebsd-hackers@freebsd.org, jmg@freebsd.org Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160204101556.GB21877@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , freebsd-hackers@freebsd.org, jmg@freebsd.org References: <1454386069-29657-3-git-send-email-mjguzik@gmail.com> <20160202132322.GU91220@kib.kiev.ua> <20160202175652.GA9812@dft-labs.eu> <20160202181635.GC91220@kib.kiev.ua> <20160202214427.GB9812@dft-labs.eu> <20160203010412.GC9812@dft-labs.eu> <20160203080514.GA8753@dft-labs.eu> <20160203141329.GF91220@kib.kiev.ua> <20160204093515.GA21877@dft-labs.eu> <20160204095341.GO91220@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160204095341.GO91220@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) 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: Thu, 04 Feb 2016 10:16:01 -0000 On Thu, Feb 04, 2016 at 11:53:41AM +0200, Konstantin Belousov wrote: > On Thu, Feb 04, 2016 at 10:35:15AM +0100, Mateusz Guzik wrote: > > Stuff below is just speculation. > > > > So the remaining problem, after we know the process has to survive, is > > survival of the thread and its relationship with the process. > > > > The problem stems from not having the proc lock over the entire time > > from the moment the thread is marked as runnable to the moment where the > > code is done with it. > > > > Race 1: > > > > CPU0 CPU1 > > p1: p2 and td2 created > > td2: marked runnable > > td2: scheduled here > > td2: does not have TDB_STOPATFORK set > > td2: calls thr_new > > td2: calls thr_exit > > td2: reused and linked into p3 > > td2: gets TDB_STOPATFORK > > p1: PROC_LOCK(p2); > > p1: TDB_STOPATFORK test on td2 > > p1: cv_wait(&p2->p_dbgwait, ..); > > > > p2 is the process we want, but td2 now belongs to a different thread. > > > > Race 2: > > > > However, seems to be even more buggy. To quote: > > > > while ((td2->td_dbgflags & TDB_STOPATFORK) != 0) > > cv_wait(&p2->p_dbgwait, &p2->p_mtx); > > > > The check is done in a loop which drops the proc lock. This makes me > > wonder about the following additional race: > > > > p2 is traced, TDB_STOPATFORK is set on td2. > > > > CPU0 CPU1 > > p1: PROC_LOCK(p2); > > p1: TDB_STOPATFORK test on td2 > > p1: cv_wait(&p2->p_dbgwait, ..); > > td2: is scheduled here > > td2: clears TDB_STOPATFORK > > td2: cv_broadcast(&p2->p_dbgwait) > > p1: not scheduled yet > > td2: calls thr_new > > td2: calls thr_exit > > td2: is reused and linked into p3 > > td2: gets TDB_STOPATFORK > > p1: scheduled here > > p1: internal PROC_LOCK(p2); > > p1: TDB_STOPATFORK test on td2 > > > > But td2 now belongs to p3. > > > > I think the patch below deals with race 1 just fine. > > > > For race 2, it is unclear to me if the while loop is justified. If a > > single 'if' statement was sufficient, there would be no problem since > > unlock + lock would be avoided guaranteeting the consistency. > > > > I was pondering borrowing fork_return's logic to check if tracing is > > enabled before testing TDB_STOPATFORK. However, tracing state could have > > changed several times invalidating the result. Maybe refreshing the > > pointer to th first thread would do the trick, but imho the lock > > dropping business is extremely fishy and will have to be dealt with at > > some point. > > > So if the issue is only reassignment of td2 to p3, why not do the following ? > I think that possible ABA problem where td2 gets TDB_STOPATFORK set after > being reused for p2 (and not p3) after yet another fork, is actually fine. > > diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c > index baee954..5bb14e8 100644 > --- a/sys/kern/kern_fork.c > +++ b/sys/kern/kern_fork.c > @@ -777,7 +777,7 @@ do_fork(struct thread *td, struct fork_req *fr, struct proc *p2, struct thread * > /* > * Wait until debugger is attached to child. > */ > - while ((td2->td_dbgflags & TDB_STOPATFORK) != 0) > + while (td2->td_proc == p2 && (td2->td_dbgflags & TDB_STOPATFORK) != 0) > cv_wait(&p2->p_dbgwait, &p2->p_mtx); > _PRELE(p2); > racct_proc_fork_done(p2); This is definitely fine for the being, it's just that unlock+lock pair which seems extremely error prone and someone(tm) should investigate it further at some point (tm). -- Mateusz Guzik