linux-kernel - Re: [git pull] vfs fixes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 3 Apr 2017 03:21:00 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Eric Biederman <ebiederm@...ssion.com>
Subject: Re: [git pull] vfs fixes

On Sun, Apr 02, 2017 at 05:58:41PM -0700, Linus Torvalds wrote:

> I had to go and double-check that "DCACHE_DIRECTORY_TYPE" is what
> d_can_lookup() actually checks, so _that_ part is perhaps a bit
> subtle, and might be worth noting in that comment that you edited.
> 
> So the real "rule" ends up being that we only ever look up things from
> dentries of type DCACHE_DIRECTORY_TYPE set, and those had better have
> DCACHE_RCUACCESS bit set.
> 
> And the only reason path_init() only checks it for that case is that
> nd->root and nd->pwd both have to be of type d_can_lookup().
> 
> Do we check that when we set it? I hope/assume we do.

For chdir()/chroot()/pivot_root() it's done by LOOKUP_DIRECTORY in lookup
flags; fchdir() is slightly different - there we check S_ISDIR of inode
of opened file.  Which is almost the same thing, except for
kinda-sorta directories that have no ->lookup() - we have them for
NFS referral points.  It should be impossible to end up with
one of those opened - not even with O_PATH; follow_managed() will be called
and we'll either fail or cross into whatever ends up overmounting them.
Still, it might be cleaner to turn that check into
	d_can_lookup(f.file->f_path.dentry)
simply for consistency sake.

The thing I really don't like is mntns_install().  With sufficiently
nasty nfsroot setup it might be possible to end up with referral point
as one's root/pwd; getting out of such state would be interesting...
Smells like that place should be a solitary follow_down(), not a loop
of follow_down_one(), but I want Eric's opinion on that one; userns stuff
is weird.

diff --git a/fs/dcache.c b/fs/dcache.c
index 95d71eda8142..05550139a8a6 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1757,7 +1757,13 @@ static unsigned d_flags_for_inode(struct inode *inode)
 		return DCACHE_MISS_TYPE;
 
 	if (S_ISDIR(inode->i_mode)) {
-		add_flags = DCACHE_DIRECTORY_TYPE;
+		/*
+		 * Any potential starting point of lookup should have
+		 * DCACHE_RCUACCESS; currently directory dentries
+		 * come from d_alloc() anyway, but it costs us nothing
+		 * to enforce it here.
+		 */
+		add_flags = DCACHE_DIRECTORY_TYPE | DCACHE_RCUACCESS;
 		if (unlikely(!(inode->i_opflags & IOP_LOOKUP))) {
 			if (unlikely(!inode->i_op->lookup))
 				add_flags = DCACHE_AUTODIR_TYPE;
diff --git a/fs/namei.c b/fs/namei.c
index d41fab78798b..19dcf62133cc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2145,6 +2145,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 	int retval = 0;
 	const char *s = nd->name->name;
 
+	if (!*s)
+		flags &= ~LOOKUP_RCU;
+
 	nd->last_type = LAST_ROOT; /* if there are only slashes... */
 	nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
 	nd->depth = 0;
diff --git a/fs/namespace.c b/fs/namespace.c
index cc1375eff88c..31ec9a79d2d4 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3467,6 +3467,7 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns)
 	struct fs_struct *fs = current->fs;
 	struct mnt_namespace *mnt_ns = to_mnt_ns(ns);
 	struct path root;
+	int err;
 
 	if (!ns_capable(mnt_ns->user_ns, CAP_SYS_ADMIN) ||
 	    !ns_capable(current_user_ns(), CAP_SYS_CHROOT) ||
@@ -3484,15 +3485,14 @@ static int mntns_install(struct nsproxy *nsproxy, struct ns_common *ns)
 	root.mnt    = &mnt_ns->root->mnt;
 	root.dentry = mnt_ns->root->mnt.mnt_root;
 	path_get(&root);
-	while(d_mountpoint(root.dentry) && follow_down_one(&root))
-		;
-
-	/* Update the pwd and root */
-	set_fs_pwd(fs, &root);
-	set_fs_root(fs, &root);
-
+	err = follow_down(&root);
+	if (likely(!err)) {
+		/* Update the pwd and root */
+		set_fs_pwd(fs, &root);
+		set_fs_root(fs, &root);
+	}
 	path_put(&root);
-	return 0;
+	return err;
 }
 
 static struct user_namespace *mntns_owner(struct ns_common *ns)
diff --git a/fs/open.c b/fs/open.c
index 949cef29c3bb..217b5db588c8 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -459,20 +459,17 @@ SYSCALL_DEFINE1(chdir, const char __user *, filename)
 SYSCALL_DEFINE1(fchdir, unsigned int, fd)
 {
 	struct fd f = fdget_raw(fd);
-	struct inode *inode;
 	int error = -EBADF;
 
 	error = -EBADF;
 	if (!f.file)
 		goto out;
 
-	inode = file_inode(f.file);
-
 	error = -ENOTDIR;
-	if (!S_ISDIR(inode->i_mode))
+	if (!d_can_lookup(f.file->f_path.dentry))
 		goto out_putf;
 
-	error = inode_permission(inode, MAY_EXEC | MAY_CHDIR);
+	error = inode_permission(file_inode(f.file), MAY_EXEC | MAY_CHDIR);
 	if (!error)
 		set_fs_pwd(current->fs, &f.file->f_path);
 out_putf: