Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 9 Jul 2015 12:19:38 +0200
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        rwatson@FreeBSD.org, freebsd-fs@freebsd.org, Mateusz Guzik <mjg@freebsd.org>
Subject:   Re: [PATCH 1/4] vfs: plug a use-after-free of fd_rdir in namei
Message-ID:  <20150709101937.GB1718@dft-labs.eu>
In-Reply-To: <20150709101604.GM2080@kib.kiev.ua>
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>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjg@freebsd.org>
> > 
> > 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 <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150709101937.GB1718>