Date: Thu, 9 Jul 2015 13:16:04 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@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: <20150709101604.GM2080@kib.kiev.ua> In-Reply-To: <1436393231-5831-2-git-send-email-mjguzik@gmail.com> References: <20150707085857.GZ2080@kib.kiev.ua> <1436393231-5831-1-git-send-email-mjguzik@gmail.com> <1436393231-5831-2-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 ?
>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150709101604.GM2080>