Date:	Fri, 11 Dec 2015 13:40:40 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Greg KH <greg@...ah.com>, Jiri Slaby <jslaby@...e.com>
Cc:	"H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Aurelien Jarno <aurelien@...el32.net>,
	Andy Lutomirski <luto@...capital.net>,
	Florian Weimer <fw@...eb.enyo.de>,
	Al Viro <viro@...iv.linux.org.uk>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	Jann Horn <jann@...jh.net>,
	"security\@kernel.org" <security@...nel.org>,
	"security\@ubuntu.com \>\> security" <security@...ntu.com>,
	security@...ian.org, Willy Tarreau <w@....eu>,
	<linux-kernel@...r.kernel.org>
Subject: [PATCH] devpts: Sensible /dev/ptmx & force newinstance


The suid root helper of grantpt /usr/lib/pt_chown remains in use today
because of sloppy userspace code that mount devpts and does not realize
their change in mount options also applies to the primary system devpts
instance.  As the system devpts instance looses it gid=5 mount option
/usr/lib/pt_chown becomes required to set the gid properly on slave
pty instances.

That can be trivially fixed by making each and every mount of devpts
their own independent filesystems.  Which can be done by always
forcing the existing newinstance option on.

To make the fix work one more thing has to be accomplished.  The
device node /dev/ptmx needs to associate itself with the instance
of the devpts filesystem currently mounted at /dev/pts.

The bulk of this patch adds path walking code to the implementation
of /dev/ptmx so that it finds the devpts filesystem to create a
pty on by looking at the pts entry in the device nodes parent
directory.

The path walker is called kern_path_pts and is stored in fs/namei.c so
the are no weird vfs exports are necessary.  The path walker is also
special in that it performs no permission checks for the path walk.

This patch addresses one additional practical problem with the opening
of /dev/ptmx.  How to find which instance of the devpts filesystem
userspace is dealing with.  The opening of /dev/ptmx now ensures that
fstat on the file descriptor returns st_dev of devpts filesystem on
which the slave pty resides, and that readlink /proc/self/NNN returns
the path to ptmx on the devpts filesystem.

Forcing newinstance for every mount of the devpts filesystem actually
requires the association between /dev/ptmx and the currently mounted
instance of devpts at /dev/pts.  Simply remembering the first mount of
the devpts filesystem and associating that with /dev/ptmx is not
enough.  I am aware of at least one instance where an initramfs mounts
devpts before the main system instance of devpts is mounted.  In that
system ptys simply did not work after boot when I tested associating
/dev/ptmx with the first mount of the devpts filesystem.

Similary replacing the /dev/ptmx node in devtmpfs with a symlink to
/dev/pts/ptmx is not sufficient as there are some versions of udev
that look at sysfs see that a device node is supposed to be at
/dev/ptmx and replace the symlink with the device node.

Signed-off-by: Eric W. Biederman <ebiederm@...ssion.com>
---

Greg I assume this change should go through your tty tree?

 drivers/tty/pty.c         |  4 +++
 fs/devpts/inode.c         | 46 ++++++++++++++++++++++++++++++-
 fs/namei.c                | 69 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/devpts_fs.h |  1 +
 include/linux/namei.h     |  1 +
 5 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a45660f62db5..81ae0945cd53 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -738,6 +738,10 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	int retval;
 	int index;
 
+	inode = devpts_ptmx(inode, filp);
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
+
 	nonseekable_open(inode, filp);
 
 	/* We refuse fsnotify events on ptmx, since it's a shared resource */
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c35ffdc12bba..79e8d60ba0fe 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -136,6 +136,50 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
+{
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+	struct path path, old;
+	struct super_block *sb;
+	struct dentry *root;
+
+	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
+		return inode;
+
+	old = filp->f_path;
+	path = old;
+	path_get(&path);
+	if (kern_path_pts(&path)) {
+		path_put(&path);
+		return ERR_PTR(-EINVAL);
+	}
+
+	sb = path.mnt->mnt_sb;
+	if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
+		path_put(&path);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Advance path to the ptmx dentry */
+	root = path.dentry;
+	path.dentry = dget(DEVPTS_SB(sb)->ptmx_dentry);
+	dput(root);
+
+	/*
+	 * Update filp with the new path so that userspace can use
+	 * fstat to know which instance of devpts is open, and so
+	 * userspace can use readlink /proc/self/fd/NNN to find the
+	 * path to the devpts filesystem for reporting slave inodes.
+	 */
+	inode = path.dentry->d_inode;
+	filp->f_path = path;
+	filp->f_inode = inode;
+	filp->f_mapping = inode->i_mapping;
+	path_put(&old);
+#endif
+	return inode;
+}
+
 static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 {
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
@@ -175,7 +219,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 
 	/* newinstance makes sense only on initial mount */
 	if (op == PARSE_MOUNT)
-		opts->newinstance = 0;
+		opts->newinstance = 1;
 
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
diff --git a/fs/namei.c b/fs/namei.c
index d84d7c7515fc..bd19db26a898 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2515,6 +2515,75 @@ kern_path_mountpoint(int dfd, const char *name, struct path *path,
 }
 EXPORT_SYMBOL(kern_path_mountpoint);
 
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+int kern_path_pts(struct path *path)
+{
+	/* This is a path walk to ${path}/../pts without permission checks */
+	struct path root;
+	struct dentry *parent, *pts;
+	struct qstr this;
+	int err;
+
+	get_fs_root(current->fs, &root);
+
+	/* Find the parent of the /dev/ptmx device node.
+	 * This is a variation of follow_dotdot.
+	 */
+	err = -ENOENT;
+	while (1) {
+		struct dentry *old = path->dentry;
+
+		if ((old == root.dentry) &&
+		    (path->mnt == root.mnt))
+			goto fail;
+
+		if (old != path->mnt->mnt_root) {
+			path->dentry = dget_parent(old);
+			dput(old);
+			if (unlikely(!path_connected(path)))
+				goto fail;
+			break;
+		}
+		if (!follow_up(path))
+			goto fail;
+	}
+	follow_mount(path);
+
+	/* In the parent directory find the cached pts dentry.  The
+	 * dentry must be cached if it is a mountpoint for the devpts
+	 * filesystem.
+	 */
+	parent = path->dentry;
+	this.name = "pts";
+	this.len = 3;
+	this.hash = full_name_hash(this.name, this.len);
+	if (parent->d_flags & DCACHE_OP_HASH) {
+		err = parent->d_op->d_hash(parent, &this);
+		if (err < 0)
+			goto fail;
+	}
+
+	err = -ENOENT;
+	mutex_lock(&parent->d_inode->i_mutex);
+	pts = d_lookup(parent, &this);
+	mutex_unlock(&parent->d_inode->i_mutex);
+	if (!pts)
+		goto fail;
+
+	/* Find what is mounted on pts */
+	path->dentry = pts;
+	dput(parent);
+	follow_mount(path);
+
+	path_put(&root);
+	return 0;
+
+fail:
+	path_put(&root);
+	return err;
+}
+#endif
+
 int __check_sticky(struct inode *dir, struct inode *inode)
 {
 	kuid_t fsuid = current_fsuid();
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 251a2090a554..8834dba07ff9 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -17,6 +17,7 @@
 
 #ifdef CONFIG_UNIX98_PTYS
 
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp);
 int devpts_new_index(struct inode *ptmx_inode);
 void devpts_kill_index(struct inode *ptmx_inode, int idx);
 /* mknod in devpts */
diff --git a/include/linux/namei.h b/include/linux/namei.h
index d8c6334cd150..cfac431bcd31 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -75,6 +75,7 @@ extern struct dentry *user_path_create(int, const char __user *, struct path *,
 extern void done_path_create(struct path *, struct dentry *);
 extern struct dentry *kern_path_locked(const char *, struct path *);
 extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int);
+extern int kern_path_pts(struct path *path);
 
 extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
 
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists