From owner-freebsd-fs@freebsd.org Thu Jul 9 10:19:44 2015 Return-Path: Delivered-To: freebsd-fs@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 0E0C2996F97 for ; Thu, 9 Jul 2015 10:19:44 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wg0-x234.google.com (mail-wg0-x234.google.com [IPv6:2a00:1450:400c:c00::234]) (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 9976D10BE; Thu, 9 Jul 2015 10:19:43 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wgxm20 with SMTP id m20so35928510wgx.3; Thu, 09 Jul 2015 03:19:42 -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:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=QT94yjnO6DKnTitoibu6VQxJINAI5BNmY0FM03HcQK8=; b=WCejUtSsExkfmisD+zS2QOm7ZxhCSg1481kZDofVxU/cZ9jZw6QxlYfV2K7WKt5gwB WgbNayQP4CLPh/L1Q+kAMuDAO1/PJm1uAoe2ao/nCPsfHCtaunGCWguFnmvck4SUEujk 2MjXkvGHuUNbPKdIDAka1zRcZYX0yNs+IkCFobinnpXOM+LXvg3yvtA4elK+RYkemGnl /1DvrshyLbUULvTBA2/F2I7kWz1CNGFJw8BmDPe3lm4HAwNICCvxGKS1kkZ8MuK1VqTQ DB0MEpvOwmomBQeqM5lHun7qchHVsliE/qAkXYqTQfitNbonL5mdICdMu+DYqme5dGV9 s7zw== X-Received: by 10.194.58.69 with SMTP id o5mr29414898wjq.22.1436437182041; Thu, 09 Jul 2015 03:19:42 -0700 (PDT) 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 ee1sm7489358wic.8.2015.07.09.03.19.40 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 09 Jul 2015 03:19:40 -0700 (PDT) Date: Thu, 9 Jul 2015 12:19:38 +0200 From: Mateusz Guzik To: Konstantin Belousov Cc: rwatson@FreeBSD.org, freebsd-fs@freebsd.org, Mateusz Guzik Subject: Re: [PATCH 1/4] vfs: plug a use-after-free of fd_rdir in namei Message-ID: <20150709101937.GB1718@dft-labs.eu> References: <20150707085857.GZ2080@kib.kiev.ua> <1436393231-5831-1-git-send-email-mjguzik@gmail.com> <1436393231-5831-2-git-send-email-mjguzik@gmail.com> <20150709101604.GM2080@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150709101604.GM2080@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Jul 2015 10:19:44 -0000 On Thu, Jul 09, 2015 at 01:16:04PM +0300, Konstantin Belousov wrote: > On Thu, Jul 09, 2015 at 12:07:08AM +0200, Mateusz Guzik wrote: > > From: Mateusz Guzik > > > > fd_rdir vnode was stored in ni_rootdir without refing it in any way, > > after which the filedsc lock was being dropped. > > > > The vnode could have been freed by mountcheckdirs or another thread doing > > chroot. > > > > VREF the vnode while the lock is held. > Patch looks fine. > > Would it make sense to extend namei_cleanup to also handle deref ? > namei_cleanup_cnp is possibly called once prior to obtaining the reference, also within the lookup loop there is one call which may or may not call it prior to exiting. > > > > MFC after: 1 week > > --- > > sys/kern/vfs_lookup.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c > > index 5dc07dc..20f8e96 100644 > > --- a/sys/kern/vfs_lookup.c > > +++ b/sys/kern/vfs_lookup.c > > @@ -210,6 +210,7 @@ namei(struct nameidata *ndp) > > */ > > FILEDESC_SLOCK(fdp); > > ndp->ni_rootdir = fdp->fd_rdir; > > + VREF(ndp->ni_rootdir); > > ndp->ni_topdir = fdp->fd_jdir; > > > > /* > > @@ -260,6 +261,7 @@ namei(struct nameidata *ndp) > > } > > } > > if (error) { > > + vrele(ndp->ni_rootdir); > > namei_cleanup_cnp(cnp); > > return (error); > > } > > @@ -286,6 +288,7 @@ namei(struct nameidata *ndp) > > if (KTRPOINT(curthread, KTR_CAPFAIL)) > > ktrcapfail(CAPFAIL_LOOKUP, NULL, NULL); > > #endif > > + vrele(ndp->ni_rootdir); > > namei_cleanup_cnp(cnp); > > return (ENOTCAPABLE); > > } > > @@ -299,6 +302,7 @@ namei(struct nameidata *ndp) > > ndp->ni_startdir = dp; > > error = lookup(ndp); > > if (error) { > > + vrele(ndp->ni_rootdir); > > namei_cleanup_cnp(cnp); > > SDT_PROBE(vfs, namei, lookup, return, error, NULL, 0, > > 0, 0); > > @@ -308,6 +312,7 @@ namei(struct nameidata *ndp) > > * If not a symbolic link, we're done. > > */ > > if ((cnp->cn_flags & ISSYMLINK) == 0) { > > + vrele(ndp->ni_rootdir); > > if ((cnp->cn_flags & (SAVENAME | SAVESTART)) == 0) { > > namei_cleanup_cnp(cnp); > > } else > > @@ -371,6 +376,7 @@ namei(struct nameidata *ndp) > > vput(ndp->ni_vp); > > dp = ndp->ni_dvp; > > } > > + vrele(ndp->ni_rootdir); > > namei_cleanup_cnp(cnp); > > vput(ndp->ni_vp); > > ndp->ni_vp = NULL; > > -- > > 2.4.5 -- Mateusz Guzik