diff mbox

[review,5/6] vfs: Test for and handle paths that are unreachable from their mnt_root

Message ID 878u9s9i1d.fsf_-_@x220.int.ebiederm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Eric W. Biederman Aug. 3, 2015, 9:30 p.m. UTC
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 nd->path.dentry is reachable
  from nd->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>
---
 fs/namei.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

Comments

Al Viro Aug. 10, 2015, 4:38 a.m. UTC | #1
On Mon, Aug 03, 2015 at 04:30:22PM -0500, Eric W. Biederman wrote:

> +	if (!is_subdir(nd->path.dentry, mnt->mnt_root))
> +		return false;

Umm...  What's to protect us from racing with d_move() right here?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman Aug. 10, 2015, 7:34 p.m. UTC | #2
Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Mon, Aug 03, 2015 at 04:30:22PM -0500, Eric W. Biederman wrote:
>
>> +	if (!is_subdir(nd->path.dentry, mnt->mnt_root))
>> +		return false;
>
> Umm...  What's to protect us from racing with d_move() right here?

is_subdir does the read_seqretry on rename_lock.  Which is enough
to ensure connectivity exists at a single moment in time.

Beyond that the entire path lookup races with d_move, and the code
calls path_connected just after finding the parent directory, which
ensures that in the moment that follow_dotdot is setting nd->dentry
that the original nd->dentry is connected, and by extension the new
as the new one is an ancestor.

Or are you thinking of a different race?

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/namei.c b/fs/namei.c
index ae4e4c18b2ac..bccd3810ff60 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -560,6 +560,27 @@  static int __nd_alloc_stack(struct nameidata *nd)
 	return 0;
 }
 
+/**
+ * path_connected - Verify that a nd->path.dentry is below nd->path.mnt->mnt.mnt_root
+ * @nd: 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(struct nameidata *nd)
+{
+	struct vfsmount *mnt = nd->path.mnt;
+	unsigned escape_count = read_mnt_escape_count(mnt);
+
+	if (likely(escape_count == 0))
+		return true;
+
+	if (!is_subdir(nd->path.dentry, mnt->mnt_root))
+		return false;
+
+	return true;
+}
+
 static inline int nd_alloc_stack(struct nameidata *nd)
 {
 	if (likely(nd->depth != EMBEDDED_LEVELS))
@@ -1294,6 +1315,8 @@  static int follow_dotdot_rcu(struct nameidata *nd)
 			seq = read_seqcount_begin(&parent->d_seq);
 			if (unlikely(read_seqcount_retry(&old->d_seq, nd->seq)))
 				return -ECHILD;
+			if (unlikely(!path_connected(nd)))
+				return -ENOENT;
 			nd->path.dentry = parent;
 			nd->seq = seq;
 			break;
@@ -1396,7 +1419,7 @@  static void follow_mount(struct path *path)
 	}
 }
 
-static void follow_dotdot(struct nameidata *nd)
+static int follow_dotdot(struct nameidata *nd)
 {
 	if (!nd->root.mnt)
 		set_root(nd);
@@ -1410,7 +1433,12 @@  static void follow_dotdot(struct nameidata *nd)
 		}
 		if (nd->path.dentry != nd->path.mnt->mnt_root) {
 			/* rare case of legitimate dget_parent()... */
-			nd->path.dentry = dget_parent(nd->path.dentry);
+			struct dentry *parent = dget_parent(nd->path.dentry);
+			if (unlikely(!path_connected(nd))) {
+				dput(parent);
+				return -ENOENT;
+			}
+			nd->path.dentry = parent;
 			dput(old);
 			break;
 		}
@@ -1419,6 +1447,7 @@  static void follow_dotdot(struct nameidata *nd)
 	}
 	follow_mount(&nd->path);
 	nd->inode = nd->path.dentry->d_inode;
+	return 0;
 }
 
 /*
@@ -1634,7 +1663,7 @@  static inline int handle_dots(struct nameidata *nd, int type)
 		if (nd->flags & LOOKUP_RCU) {
 			return follow_dotdot_rcu(nd);
 		} else
-			follow_dotdot(nd);
+			return follow_dotdot(nd);
 	}
 	return 0;
 }