From owner-freebsd-fs@freebsd.org Sat Jul 11 13:40:30 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 9F8E4998039 for ; Sat, 11 Jul 2015 13:40:30 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-x236.google.com (mail-wi0-x236.google.com [IPv6:2a00:1450:400c:c05::236]) (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 218FC1BC0 for ; Sat, 11 Jul 2015 13:40:30 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wifm2 with SMTP id m2so67440735wif.1 for ; Sat, 11 Jul 2015 06:40:26 -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=/AkbFUdwD1umZA2datA7Kpjawzi+yyXqbKaFf+0yXcQ=; b=V/Kn58YCxkah9vk28KN0G47F2Nc1EmgPDFghcwVMbDz5nbnGuKePMJzpsO5AIjT43y dSfLnW5RNysP6EAVo4bYrQ0LEuEJpGvCa+5WY/CrH+LgYnA/aZxQzMFv5ElnPV3uSOzS rSANuelgu0Iku02R6GBbs39cAHV1IUP48oNCd3GZATwKqu3S6S/Hmx30oim1MkAe6iiC 25uWQ5Wh11Q0C1ERM98fEi5J2DYeK6fq8G+HqxoLpyOlh5IRc3bpIu7lxO5mj2DVd59U BsltmDGJOX13mWUtsL+nGPQ7N/eBwpeJ5/4D92hm0slD52DB/o5gAV8xInneiM42otq7 4Q4A== X-Received: by 10.194.172.130 with SMTP id bc2mr54011086wjc.85.1436622026259; Sat, 11 Jul 2015 06:40:26 -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 d7sm3674535wij.0.2015.07.11.06.40.23 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 11 Jul 2015 06:40:24 -0700 (PDT) Date: Sat, 11 Jul 2015 15:40:21 +0200 From: Mateusz Guzik To: Konstantin Belousov Cc: freebsd-fs@freebsd.org Subject: Re: [PATCH 1/2] Move chdir/chroot-related fdp manipulation to kern_descrip.c Message-ID: <20150711134021.GA1433@dft-labs.eu> References: <1436569684-3939-1-git-send-email-mjguzik@gmail.com> <1436569684-3939-2-git-send-email-mjguzik@gmail.com> <20150711124335.GF2080@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150711124335.GF2080@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: Sat, 11 Jul 2015 13:40:30 -0000 On Sat, Jul 11, 2015 at 03:43:35PM +0300, Konstantin Belousov wrote: > On Sat, Jul 11, 2015 at 01:08:03AM +0200, Mateusz Guzik wrote: > > From: Mateusz Guzik > > > > Prefix exported functions with pwd_. > > > > This also adds a helper which sets fd_cdir which deduplicates some code. > Patch is fine, I like the cleanup. Please use the opportunity to fix > minor existing issues, mostly with style, see below. > > > + oldvp = fdp->fd_rdir; > VREF(vp); > > + fdp->fd_rdir = vp; > > + VREF(fdp->fd_rdir); > Remove this line. > > > + if (!fdp->fd_jdir) { > if (fdp->fd_jdir == NULL) { > > > + fdp->fd_jdir = vp; > > + VREF(fdp->fd_jdir); > Again, swap the order: do VREF(vp), then assign. > Done. > > + } > > + FILEDESC_XUNLOCK(fdp); > > + vrele(oldvp); > > + return (0); > > +} > > + > > +void > > +pwd_chdir(struct thread *td, struct vnode *vp) > > +{ > > + struct filedesc *fdp; > > + struct vnode *oldvp; > > + > > + fdp = td->td_proc->p_fd; > > + FILEDESC_XLOCK(fdp); > > + oldvp = fdp->fd_cdir; > > + fdp->fd_cdir = vp; > Assert that vp v_usecount > 0, as the inaccurate but still useful check. > We have no way to ensure that the reference is owned by the code, but > we do know that there must be such reference. > Done. diff --git a/sys/compat/svr4/svr4_misc.c b/sys/compat/svr4/svr4_misc.c index ec4504e..5e53874 100644 --- a/sys/compat/svr4/svr4_misc.c +++ b/sys/compat/svr4/svr4_misc.c @@ -643,7 +643,7 @@ svr4_sys_fchroot(td, uap) goto fail; #endif VOP_UNLOCK(vp, 0); - error = change_root(vp, td); + error = pwd_chroot(td, vp); vrele(vp); return (error); fail: diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index e8f0014..51e8b3c 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -2875,6 +2875,96 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode, } /* + * This sysctl determines if we will allow a process to chroot(2) if it + * has a directory open: + * 0: disallowed for all processes. + * 1: allowed for processes that were not already chroot(2)'ed. + * 2: allowed for all processes. + */ + +static int chroot_allow_open_directories = 1; + +SYSCTL_INT(_kern, OID_AUTO, chroot_allow_open_directories, CTLFLAG_RW, + &chroot_allow_open_directories, 0, + "Allow a process to chroot(2) if it has a directory open"); + +/* + * Helper function for raised chroot(2) security function: Refuse if + * any filedescriptors are open directories. + */ +static int +chroot_refuse_vdir_fds(struct filedesc *fdp) +{ + struct vnode *vp; + struct file *fp; + int fd; + + FILEDESC_LOCK_ASSERT(fdp); + + for (fd = 0; fd <= fdp->fd_lastfile; fd++) { + fp = fget_locked(fdp, fd); + if (fp == NULL) + continue; + if (fp->f_type == DTYPE_VNODE) { + vp = fp->f_vnode; + if (vp->v_type == VDIR) + return (EPERM); + } + } + return (0); +} + +/* + * Common routine for kern_chroot() and jail_attach(). The caller is + * responsible for invoking priv_check() and mac_vnode_check_chroot() to + * authorize this operation. + */ +int +pwd_chroot(struct thread *td, struct vnode *vp) +{ + struct filedesc *fdp; + struct vnode *oldvp; + int error; + + fdp = td->td_proc->p_fd; + FILEDESC_XLOCK(fdp); + if (chroot_allow_open_directories == 0 || + (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) { + error = chroot_refuse_vdir_fds(fdp); + if (error != 0) { + FILEDESC_XUNLOCK(fdp); + return (error); + } + } + oldvp = fdp->fd_rdir; + VREF(vp); + fdp->fd_rdir = vp; + if (fdp->fd_jdir == NULL) { + VREF(vp); + fdp->fd_jdir = vp; + } + FILEDESC_XUNLOCK(fdp); + vrele(oldvp); + return (0); +} + +void +pwd_chdir(struct thread *td, struct vnode *vp) +{ + struct filedesc *fdp; + struct vnode *oldvp; + + fdp = td->td_proc->p_fd; + FILEDESC_XLOCK(fdp); + VNASSERT(vp->v_usecount > 0, vp, + ("chdir to a vnode with zero usecount")); + oldvp = fdp->fd_cdir; + fdp->fd_cdir = vp; + FILEDESC_XUNLOCK(fdp); + vrele(oldvp); +} + +/* * Scan all active processes and prisons to see if any of them have a current * or root directory of `olddp'. If so, replace them with the new mount point. */ diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index c118d74..6bdddd7 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -2432,7 +2432,7 @@ do_jail_attach(struct thread *td, struct prison *pr) goto e_unlock; #endif VOP_UNLOCK(pr->pr_root, 0); - if ((error = change_root(pr->pr_root, td))) + if ((error = pwd_chroot(td, pr->pr_root))) goto e_revert_osd; newcred = crget(); diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c index 9088017..258a1ef 100644 --- a/sys/kern/vfs_syscalls.c +++ b/sys/kern/vfs_syscalls.c @@ -728,8 +728,7 @@ sys_fchdir(td, uap) int fd; } */ *uap; { - register struct filedesc *fdp = td->td_proc->p_fd; - struct vnode *vp, *tdp, *vpold; + struct vnode *vp, *tdp; struct mount *mp; struct file *fp; cap_rights_t rights; @@ -761,11 +760,7 @@ sys_fchdir(td, uap) return (error); } VOP_UNLOCK(vp, 0); - FILEDESC_XLOCK(fdp); - vpold = fdp->fd_cdir; - fdp->fd_cdir = vp; - FILEDESC_XUNLOCK(fdp); - vrele(vpold); + pwd_chdir(td, vp); return (0); } @@ -791,9 +786,7 @@ sys_chdir(td, uap) int kern_chdir(struct thread *td, char *path, enum uio_seg pathseg) { - register struct filedesc *fdp = td->td_proc->p_fd; struct nameidata nd; - struct vnode *vp; int error; NDINIT(&nd, LOOKUP, FOLLOW | LOCKSHARED | LOCKLEAF | AUDITVNODE1, @@ -807,56 +800,11 @@ kern_chdir(struct thread *td, char *path, enum uio_seg pathseg) } VOP_UNLOCK(nd.ni_vp, 0); NDFREE(&nd, NDF_ONLY_PNBUF); - FILEDESC_XLOCK(fdp); - vp = fdp->fd_cdir; - fdp->fd_cdir = nd.ni_vp; - FILEDESC_XUNLOCK(fdp); - vrele(vp); - return (0); -} - -/* - * Helper function for raised chroot(2) security function: Refuse if - * any filedescriptors are open directories. - */ -static int -chroot_refuse_vdir_fds(fdp) - struct filedesc *fdp; -{ - struct vnode *vp; - struct file *fp; - int fd; - - FILEDESC_LOCK_ASSERT(fdp); - - for (fd = 0; fd <= fdp->fd_lastfile; fd++) { - fp = fget_locked(fdp, fd); - if (fp == NULL) - continue; - if (fp->f_type == DTYPE_VNODE) { - vp = fp->f_vnode; - if (vp->v_type == VDIR) - return (EPERM); - } - } + pwd_chdir(td, nd.ni_vp); return (0); } /* - * This sysctl determines if we will allow a process to chroot(2) if it - * has a directory open: - * 0: disallowed for all processes. - * 1: allowed for processes that were not already chroot(2)'ed. - * 2: allowed for all processes. - */ - -static int chroot_allow_open_directories = 1; - -SYSCTL_INT(_kern, OID_AUTO, chroot_allow_open_directories, CTLFLAG_RW, - &chroot_allow_open_directories, 0, - "Allow a process to chroot(2) if it has a directory open"); - -/* * Change notion of root (``/'') directory. */ #ifndef _SYS_SYSPROTO_H_ @@ -891,7 +839,7 @@ sys_chroot(td, uap) goto e_vunlock; #endif VOP_UNLOCK(nd.ni_vp, 0); - error = change_root(nd.ni_vp, td); + error = pwd_chroot(td, nd.ni_vp); vrele(nd.ni_vp); NDFREE(&nd, NDF_ONLY_PNBUF); return (error); @@ -926,42 +874,6 @@ change_dir(vp, td) return (VOP_ACCESS(vp, VEXEC, td->td_ucred, td)); } -/* - * Common routine for kern_chroot() and jail_attach(). The caller is - * responsible for invoking priv_check() and mac_vnode_check_chroot() to - * authorize this operation. - */ -int -change_root(vp, td) - struct vnode *vp; - struct thread *td; -{ - struct filedesc *fdp; - struct vnode *oldvp; - int error; - - fdp = td->td_proc->p_fd; - FILEDESC_XLOCK(fdp); - if (chroot_allow_open_directories == 0 || - (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) { - error = chroot_refuse_vdir_fds(fdp); - if (error != 0) { - FILEDESC_XUNLOCK(fdp); - return (error); - } - } - oldvp = fdp->fd_rdir; - fdp->fd_rdir = vp; - VREF(fdp->fd_rdir); - if (!fdp->fd_jdir) { - fdp->fd_jdir = vp; - VREF(fdp->fd_jdir); - } - FILEDESC_XUNLOCK(fdp); - vrele(oldvp); - return (0); -} - static __inline void flags_to_rights(int flags, cap_rights_t *rightsp) { diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h index ab7ce9f..e569a3b 100644 --- a/sys/sys/filedesc.h +++ b/sys/sys/filedesc.h @@ -205,6 +205,10 @@ fd_modified(struct filedesc *fdp, int fd, seq_t seq) return (!seq_consistent(fd_seq(fdp->fd_files, fd), seq)); } +/* cdir/rdir/jdir manipulation functions. */ +void pwd_chdir(struct thread *td, struct vnode *vp); +int pwd_chroot(struct thread *td, struct vnode *vp); + #endif /* _KERNEL */ #endif /* !_SYS_FILEDESC_H_ */ diff --git a/sys/sys/vnode.h b/sys/sys/vnode.h index 36ef8af..6d5da32 100644 --- a/sys/sys/vnode.h +++ b/sys/sys/vnode.h @@ -616,7 +616,6 @@ void cache_purge(struct vnode *vp); void cache_purge_negative(struct vnode *vp); void cache_purgevfs(struct mount *mp); int change_dir(struct vnode *vp, struct thread *td); -int change_root(struct vnode *vp, struct thread *td); void cvtstat(struct stat *st, struct ostat *ost); void cvtnstat(struct stat *sb, struct nstat *nsb); int getnewvnode(const char *tag, struct mount *mp, struct vop_vector *vops, diff --git a/sys/ufs/ffs/ffs_alloc.c b/sys/ufs/ffs/ffs_alloc.c index 2b9c334..c587dfb 100644 --- a/sys/ufs/ffs/ffs_alloc.c +++ b/sys/ufs/ffs/ffs_alloc.c @@ -2748,13 +2748,12 @@ sysctl_ffs_fsck(SYSCTL_HANDLER_ARGS) struct thread *td = curthread; struct fsck_cmd cmd; struct ufsmount *ump; - struct vnode *vp, *vpold, *dvp, *fdvp; + struct vnode *vp, *dvp, *fdvp; struct inode *ip, *dp; struct mount *mp; struct fs *fs; ufs2_daddr_t blkno; long blkcnt, blksize; - struct filedesc *fdp; struct file *fp, *vfp; cap_rights_t rights; int filetype, error; @@ -2968,12 +2967,7 @@ sysctl_ffs_fsck(SYSCTL_HANDLER_ARGS) break; } VOP_UNLOCK(vp, 0); - fdp = td->td_proc->p_fd; - FILEDESC_XLOCK(fdp); - vpold = fdp->fd_cdir; - fdp->fd_cdir = vp; - FILEDESC_XUNLOCK(fdp); - vrele(vpold); + pwd_chdir(td, vp); break; case FFS_SET_DOTDOT: -- Mateusz Guzik