- Change chroot_refuse_vdir_fds() to require that the passed in struct

  filedesc is already locked rather than having chroot() unlock the
  filedesc so chroot_refuse_vdir_fds() can immediately relock it.
- Reorder chroot() a bitso that we do the namei lookup before checking
  the process's struct filedesc.  This closes at least one potential race
  and allows us to only acquire the filedsec lock once in chroot().
- Push down Giant slightly into chroot().

2 files changed, 36 insertions, 32 deletions

diff --git a/sys/kern/vfs_extattr.c b/sys/kern/vfs_extattr.c
index 17da270..69159e7 100644
--- a/sys/kern/vfs_extattr.c
+++ b/sys/kern/vfs_extattr.c
@@ -450,20 +450,17 @@ chroot_refuse_vdir_fds(fdp)
 	struct file *fp;
 	int fd;
 
-	FILEDESC_LOCK(fdp);
+	FILEDESC_LOCK_ASSERT(fdp, MA_OWNED);
 	for (fd = 0; fd < fdp->fd_nfiles ; fd++) {
 		fp = fget_locked(fdp, fd);
 		if (fp == NULL)
 			continue;
 		if (fp->f_type == DTYPE_VNODE) {
 			vp = (struct vnode *)fp->f_data;
-			if (vp->v_type == VDIR) {
-				FILEDESC_UNLOCK(fdp);
+			if (vp->v_type == VDIR)
 				return (EPERM);
-			}
 		}
 	}
-	FILEDESC_UNLOCK(fdp);
 	return (0);
 }
 
@@ -504,21 +501,18 @@ chroot(td, uap)
 	error = suser_cred(td->td_ucred, PRISON_ROOT);
 	if (error)
 		return (error);
-	FILEDESC_LOCK(fdp);
-	if (chroot_allow_open_directories == 0 ||
-	    (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) {
-		FILEDESC_UNLOCK(fdp);
-		error = chroot_refuse_vdir_fds(fdp);
-	} else
-		FILEDESC_UNLOCK(fdp);
-	if (error)
-		return (error);
 	NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, UIO_USERSPACE,
 	    SCARG(uap, path), td);
+	mtx_lock(&Giant);
 	if ((error = change_dir(&nd, td)) != 0)
-		return (error);
-	NDFREE(&nd, NDF_ONLY_PNBUF);
+		goto error;
 	FILEDESC_LOCK(fdp);
+	if (chroot_allow_open_directories == 0 ||
+	    (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) {
+		error = chroot_refuse_vdir_fds(fdp);
+		if (error)
+			goto error_unlock;
+	}
 	vp = fdp->fd_rdir;
 	fdp->fd_rdir = nd.ni_vp;
 	if (!fdp->fd_jdir) {
@@ -526,8 +520,16 @@ chroot(td, uap)
                 VREF(fdp->fd_jdir);
 	}
 	FILEDESC_UNLOCK(fdp);
+	NDFREE(&nd, NDF_ONLY_PNBUF);
 	vrele(vp);
+	mtx_unlock(&Giant);
 	return (0);
+error_unlock:
+	FILEDESC_UNLOCK(fdp);
+error:
+	mtx_unlock(&Giant);
+	NDFREE(&nd, 0);
+	return (error);
 }
 
 /*
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 17da270..69159e7 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -450,20 +450,17 @@ chroot_refuse_vdir_fds(fdp)
 	struct file *fp;
 	int fd;
 
-	FILEDESC_LOCK(fdp);
+	FILEDESC_LOCK_ASSERT(fdp, MA_OWNED);
 	for (fd = 0; fd < fdp->fd_nfiles ; fd++) {
 		fp = fget_locked(fdp, fd);
 		if (fp == NULL)
 			continue;
 		if (fp->f_type == DTYPE_VNODE) {
 			vp = (struct vnode *)fp->f_data;
-			if (vp->v_type == VDIR) {
-				FILEDESC_UNLOCK(fdp);
+			if (vp->v_type == VDIR)
 				return (EPERM);
-			}
 		}
 	}
-	FILEDESC_UNLOCK(fdp);
 	return (0);
 }
 
@@ -504,21 +501,18 @@ chroot(td, uap)
 	error = suser_cred(td->td_ucred, PRISON_ROOT);
 	if (error)
 		return (error);
-	FILEDESC_LOCK(fdp);
-	if (chroot_allow_open_directories == 0 ||
-	    (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) {
-		FILEDESC_UNLOCK(fdp);
-		error = chroot_refuse_vdir_fds(fdp);
-	} else
-		FILEDESC_UNLOCK(fdp);
-	if (error)
-		return (error);
 	NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, UIO_USERSPACE,
 	    SCARG(uap, path), td);
+	mtx_lock(&Giant);
 	if ((error = change_dir(&nd, td)) != 0)
-		return (error);
-	NDFREE(&nd, NDF_ONLY_PNBUF);
+		goto error;
 	FILEDESC_LOCK(fdp);
+	if (chroot_allow_open_directories == 0 ||
+	    (chroot_allow_open_directories == 1 && fdp->fd_rdir != rootvnode)) {
+		error = chroot_refuse_vdir_fds(fdp);
+		if (error)
+			goto error_unlock;
+	}
 	vp = fdp->fd_rdir;
 	fdp->fd_rdir = nd.ni_vp;
 	if (!fdp->fd_jdir) {
@@ -526,8 +520,16 @@ chroot(td, uap)
                 VREF(fdp->fd_jdir);
 	}
 	FILEDESC_UNLOCK(fdp);
+	NDFREE(&nd, NDF_ONLY_PNBUF);
 	vrele(vp);
+	mtx_unlock(&Giant);
 	return (0);
+error_unlock:
+	FILEDESC_UNLOCK(fdp);
+error:
+	mtx_unlock(&Giant);
+	NDFREE(&nd, 0);
+	return (error);
 }
 
 /*