Re: [PATCH 2.6.32 00/38] 2.6.32.69-longterm review

[Willy Tarreau <w@1wt.eu>]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Mon, Nov 30, 2015 at 07:51:48AM +0100, Willy Tarreau wrote:
> > Commit 397d425dc26d ("vfs: Test for and handle paths that are
> > unreachable from their mnt_root") is missing.
> 
> OK I'm seeing it in your 3.2 branch, I'll try to backport it.

The code in 2.6.32 looks like a plate of spaghetti, which was heavily
reworked in 2.6.38-rc1 by commit 31e6b01 ("fs: rcu-walk for path lookup").
It even does something suspiciously useless :

		if (this.name[0] == '.') switch (this.len) {
			default:
				break;
			case 2:	
				if (this.name[1] != '.')
					break;
				follow_dotdot(nd);
				inode = nd->path.dentry->d_inode;
				/* fallthrough */
			case 1:
				goto return_reval;
		}

Look how inode is assigned after follow_dotdot() and is never used
between the moment it's assigned and the moment it's re-assigned. I
think I got it right though. I checked that I don't need to pass
via path_put() on exit since follow_dotdot() does it on the error
path. I'd appreciate that you, Eric, or anyone else would review it
though.

Thanks,
Willy


[-- Attachment #2: 0001-vfs-Test-for-and-handle-paths-that-are-unreachable-f.patch --]
[-- Type: text/plain, Size: 3729 bytes --]

>From a91bc7f9c52b053e7c0a59eed4eaec533934a7f4 Mon Sep 17 00:00:00 2001
From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Sat, 15 Aug 2015 20:27:13 -0500
Subject: vfs: Test for and handle paths that are unreachable from their
 mnt_root

commit 397d425dc26da728396e66d392d5dcb8dac30c37 upstream.

In rare cases a directory can be renamed out from under a bind mount.
In those cases without special handling it becomes possible to walk up
the directory tree to the root dentry of the filesystem and down
from the root dentry to every other file or directory on the filesystem.

Like division by zero .. from an unconnected path can not be given
a useful semantic as there is no predicting at which path component
the code will realize it is unconnected.  We certainly can not match
the current behavior as the current behavior is a security hole.

Therefore when encounting .. when following an unconnected path
return -ENOENT.

- Add a function path_connected to verify path->dentry is reachable
  from path->mnt.mnt_root.  AKA to validate that rename did not do
  something nasty to the bind mount.

  To avoid races path_connected must be called after following a path
  component to it's next path component.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
(cherry picked from commit 6d84ade2c8242ab83fbc5bacb66eb81a8d1ca6db)
[wt: backported to 2.6.32 which significantly differs]
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 fs/namei.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 0d766d2..1554add 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -414,6 +414,24 @@ do_revalidate(struct dentry *dentry, struct nameidata *nd)
 	return dentry;
 }
 
+/**
+ * path_connected - Verify that a path->dentry is below path->mnt.mnt_root
+ * @path: nameidate to verify
+ *
+ * Rename can sometimes move a file or directory outside of a bind
+ * mount, path_connected allows those cases to be detected.
+ */
+static bool path_connected(const struct path *path)
+{
+	struct vfsmount *mnt = path->mnt;
+
+	/* Only bind mounts can have disconnected paths */
+	if (mnt->mnt_root == mnt->mnt_sb->s_root)
+		return true;
+
+	return is_subdir(path->dentry, mnt->mnt_root);
+}
+
 /*
  * Internal lookup() using the new generic dcache.
  * SMP-safe
@@ -754,7 +772,7 @@ int follow_down(struct path *path)
 	return 0;
 }
 
-static __always_inline void follow_dotdot(struct nameidata *nd)
+static __always_inline int follow_dotdot(struct nameidata *nd)
 {
 	set_root(nd);
 
@@ -771,6 +789,10 @@ static __always_inline void follow_dotdot(struct nameidata *nd)
 			nd->path.dentry = dget(nd->path.dentry->d_parent);
 			spin_unlock(&dcache_lock);
 			dput(old);
+			if (unlikely(!path_connected(&nd->path))) {
+				path_put(&nd->path);
+				return -ENOENT;
+			}
 			break;
 		}
 		spin_unlock(&dcache_lock);
@@ -788,6 +810,7 @@ static __always_inline void follow_dotdot(struct nameidata *nd)
 		nd->path.mnt = parent;
 	}
 	follow_mount(&nd->path);
+	return 0;
 }
 
 /*
@@ -905,7 +928,9 @@ static int __link_path_walk(const char *name, struct nameidata *nd)
 			case 2:	
 				if (this.name[1] != '.')
 					break;
-				follow_dotdot(nd);
+				err = follow_dotdot(nd);
+				if (err)
+					goto return_err;
 				inode = nd->path.dentry->d_inode;
 				/* fallthrough */
 			case 1:
@@ -960,7 +985,9 @@ last_component:
 			case 2:	
 				if (this.name[1] != '.')
 					break;
-				follow_dotdot(nd);
+				err = follow_dotdot(nd);
+				if (err)
+					goto return_err;
 				inode = nd->path.dentry->d_inode;
 				/* fallthrough */
 			case 1:
-- 
1.7.12.1