From owner-freebsd-hackers@freebsd.org Tue Feb 2 17:56:58 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 66AAEA98BA2 for ; Tue, 2 Feb 2016 17:56:58 +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 C9A2217AB; Tue, 2 Feb 2016 17:56:57 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x244.google.com with SMTP id p63so3516731wmp.1; Tue, 02 Feb 2016 09:56:57 -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=vKglTS5hfhOeTYlUWPeMidOsEahTBAZReAAsYVh52p0=; b=Rsq0VF+xjE6K3qPI65U7zpUPrOvbdLRTWVZE7+5Yr0YjsDuNagPoqj50BDXTSK1uD0 O9T0nbmBsPOPb9PFeiyuX4tTEkhTY/X1WDT5cc+nsyZV4qe4/ZbpMrttnUIrpapcT8lu +jFcEUkWd57q9zo5ShDjMVzoEilNfJX8YmeDN5ta3K+3M0HhSeE1YnhxmTcOOWU7Mkbu yrkQ49zViWauZGBPRZ82U180gt2UZUKadaPKeBLfAGxGIQ4fpHov3wcWZNd8geo7Zcwf JM0peJ33XB7mwAM45jEK/GL+AS2Z189cKv7KbkiaZRTvUPCv4Hl78CJ8GY3Z/oiOi0oE iMag== 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=vKglTS5hfhOeTYlUWPeMidOsEahTBAZReAAsYVh52p0=; b=Re2rLFzyLwJjH/xO/iKySMYZIeioDydP8E4iYwWoZf7R5z7Vu+2xhBQ+jUbo6lfnRr GJgbj5Dj4DSzlLwBBnZvHxa2HcrAV2pIFWsiRrnXgdWNmXiGGMrnssTDHCCbUMlsSB4r jCKNDCBpoeUYubTnXn+d5O/Wqr7R9Gf0kZpUMWF4/x+oV+3iypOG8E+9IExFx1UuRqlh tYfiPhoUp++oyh2ep4ZVdIq2u8VYMenpPyQsAOeoQhYkyx1QgIvti6Wbi4Uu4OL2b2Go 2Ew1USMTe+d6DlFhlB0QcRDWU0ixoGx5SC+MA30MpZb05Ppf+7/1QU6w+UOj6p1/2YIL DejQ== X-Gm-Message-State: AG10YOSjkJzg00KuQ8C65PnKw6beGdjMRVgVVDePP6MBP3SsUGrpa/Fow940S4iPeRyLUg== X-Received: by 10.28.170.139 with SMTP id t133mr20388362wme.50.1454435815194; Tue, 02 Feb 2016 09:56:55 -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 yz5sm2570225wjc.36.2016.02.02.09.56.54 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Tue, 02 Feb 2016 09:56:54 -0800 (PST) Date: Tue, 2 Feb 2016 18:56:52 +0100 From: Mateusz Guzik To: Konstantin Belousov Cc: freebsd-hackers@freebsd.org Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160202175652.GA9812@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , freebsd-hackers@freebsd.org 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160202132322.GU91220@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: Tue, 02 Feb 2016 17:56:58 -0000 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. 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 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. -- Mateusz Guzik