[PATCH 1/2] Move chdir/chroot-related fdp manipulation to kern_descrip.c
Konstantin Belousov
kostikbel at gmail.com
Sat Jul 11 12:43:48 UTC 2015
On Sat, Jul 11, 2015 at 01:08:03AM +0200, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg at freebsd.org>
>
> 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.
> ---
> sys/compat/svr4/svr4_misc.c | 2 +-
> sys/kern/kern_descrip.c | 88 +++++++++++++++++++++++++++++++++++++++++
> sys/kern/kern_jail.c | 2 +-
> sys/kern/vfs_syscalls.c | 96 ++-------------------------------------------
> sys/sys/filedesc.h | 4 ++
> sys/sys/vnode.h | 1 -
> sys/ufs/ffs/ffs_alloc.c | 10 +----
> 7 files changed, 100 insertions(+), 103 deletions(-)
>
> 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 0d5ce41..37381ee 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -2855,6 +2855,94 @@ 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;
> + 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.
> + }
> + 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.
> + 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:
> --
> 2.4.5
More information about the freebsd-fs
mailing list