Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name

Linus Torvalds
2017-08-24T18:13:05Z

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

On Thu, Aug 24, 2017 at 11:06 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But that still fails the TIOCGPTPEER ioctl with an NULL pointer
> dereference in devpts_mnt, I probably messed up when I fixed the
> dentry refcount leak.

No, that was another bug in the original patch.

ptm_open_peer() passed in 'filp' to devpts_mnt(), but it should
obviously pass in the 'master' one.

filp is NULL at that point.

The attached patch should work. It's Eric's original patch with
various cleanups and fixes.

Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 7312 bytes --]

drivers/tty/pty.c | 64 ++++++++++++++++++++---------------------------
drivers/tty/tty_io.c | 3 +++
fs/devpts/inode.c | 63 ++++++++++++++++++++++++++++++++++------------
include/linux/devpts_fs.h | 10 ++++++++
4 files changed, 87 insertions(+), 53 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 284749fb0f6b..bfd66fcff7a3 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -69,13 +69,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
#ifdef CONFIG_UNIX98_PTYS
if (tty->driver == ptm_driver) {
mutex_lock(&devpts_mutex);
- if (tty->link->driver_data) {
- struct path *path = tty->link->driver_data;
-
- devpts_pty_kill(path->dentry);
- path_put(path);
- kfree(path);
- }
+ if (tty->link->driver_data)
+ devpts_pty_kill(tty->link->driver_data);
mutex_unlock(&devpts_mutex);
}
#endif
@@ -607,25 +602,24 @@ static inline void legacy_pty_init(void) { }
static struct cdev ptmx_cdev;

/**
- * pty_open_peer - open the peer of a pty
- * @tty: the peer of the pty being opened
+ * ptm_open_peer - open the peer of a pty
+ * @master: the open struct file of the ptmx device node
+ * @tty: the master of the pty being opened
+ * @flags: the flags for open
*
- * Open the cached dentry in tty->link, providing a safe way for userspace
- * to get the slave end of a pty (where they have the master fd and cannot
- * access or trust the mount namespace /dev/pts was mounted inside).
+ * Provide a race free way for userspace to open the slave end of a pty
+ * (where they have the master fd and cannot access or trust the mount
+ * namespace /dev/pts was mounted inside).
*/
-static struct file *pty_open_peer(struct tty_struct *tty, int flags)
-{
- if (tty->driver->subtype != PTY_TYPE_MASTER)
- return ERR_PTR(-EIO);
- return dentry_open(tty->link->driver_data, flags, current_cred());
-}
-
-static int pty_get_peer(struct tty_struct *tty, int flags)
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
{
int fd = -1;
- struct file *filp = NULL;
+ struct file *filp;
int retval = -EINVAL;
+ struct path path;
+
+ if (tty->driver != ptm_driver)
+ return -EIO;

fd = get_unused_fd_flags(0);
if (fd < 0) {
@@ -633,7 +627,16 @@ static int pty_get_peer(struct tty_struct *tty, int flags)
goto err;
}

- filp = pty_open_peer(tty, flags);
+ /* Compute the slave's path */
+ path.mnt = devpts_mnt(master);
+ if (IS_ERR(path.mnt)) {
+ retval = PTR_ERR(path.mnt);
+ goto err_put;
+ }
+ path.dentry = tty->link->driver_data;
+
+ filp = dentry_open(&path, flags, current_cred());
+ mntput(path.mnt);
if (IS_ERR(filp)) {
retval = PTR_ERR(filp);
goto err_put;
@@ -662,8 +665,6 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
return pty_get_pktmode(tty, (int __user *)arg);
case TIOCGPTN: /* Get PT Number */
return put_user(tty->index, (unsigned int __user *)arg);
- case TIOCGPTPEER: /* Open the other end */
- return pty_get_peer(tty, (int) arg);
case TIOCSIG: /* Send signal to other side of pty */
return pty_signal(tty, (int) arg);
}
@@ -791,7 +792,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
{
struct pts_fs_info *fsi;
struct tty_struct *tty;
- struct path *pts_path;
struct dentry *dentry;
int retval;
int index;
@@ -845,26 +845,16 @@ static int ptmx_open(struct inode *inode, struct file *filp)
retval = PTR_ERR(dentry);
goto err_release;
}
- /* We need to cache a fake path for TIOCGPTPEER. */
- pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
- if (!pts_path)
- goto err_release;
- pts_path->mnt = filp->f_path.mnt;
- pts_path->dentry = dentry;
- path_get(pts_path);
- tty->link->driver_data = pts_path;
+ tty->link->driver_data = dentry;

retval = ptm_driver->ops->open(tty, filp);
if (retval)
- goto err_path_put;
+ goto err_release;

tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);

tty_unlock(tty);
return 0;
-err_path_put:
- path_put(pts_path);
- kfree(pts_path);
err_release:
tty_unlock(tty);
// This will also put-ref the fsi
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 974b13d24401..ba3371449a5c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2518,6 +2518,9 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
case TIOCSSERIAL:
tty_warn_deprecated_flags(p);
break;
+ case TIOCGPTPEER:
+ retval = ptm_open_peer(file, tty, (int)arg);
+ break;
default:
retval = tty_jobctrl_ioctl(tty, real_tty, file, cmd, arg);
if (retval != -ENOIOCTLCMD)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 108df2e3602c..8ed105a22de3 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -133,6 +133,48 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
return sb->s_fs_info;
}

+static int devpts_ptmx_path(struct path *path)
+{
+ int err;
+ struct super_block *sb;
+
+ /* Has the devpts filesystem already been found? */
+ sb = path->mnt->mnt_sb;
+ if (sb->s_magic == DEVPTS_SUPER_MAGIC)
+ return 0;
+
+ /* Is a devpts filesystem at "pts" in the same directory? */
+ err = path_pts(path);
+ if (err)
+ return err;
+
+ sb = path->mnt->mnt_sb;
+
+ /* Is the path the root of a devpts filesystem? */
+ if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
+ (path->mnt->mnt_root != sb->s_root))
+ return -ENODEV;
+
+ return 0;
+}
+
+struct vfsmount *devpts_mnt(struct file *filp)
+{
+ struct path path;
+ int err;
+
+ path = filp->f_path;
+ path_get(&path);
+
+ err = devpts_ptmx_path(&path);
+ if (err) {
+ mntput(path.mnt);
+ path.mnt = ERR_PTR(err);
+ }
+ dput(path.dentry);
+ return path.mnt;
+}
+
struct pts_fs_info *devpts_acquire(struct file *filp)
{
struct pts_fs_info *result;
@@ -143,27 +185,16 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
path = filp->f_path;
path_get(&path);

- /* Has the devpts filesystem already been found? */
- sb = path.mnt->mnt_sb;
- if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
- /* Is a devpts filesystem at "pts" in the same directory? */
- err = path_pts(&path);
- if (err) {
- result = ERR_PTR(err);
- goto out;
- }
-
- /* Is the path the root of a devpts filesystem? */
- result = ERR_PTR(-ENODEV);
- sb = path.mnt->mnt_sb;
- if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
- (path.mnt->mnt_root != sb->s_root))
- goto out;
+ err = devpts_ptmx_path(&path);
+ if (err) {
+ result = ERR_PTR(err);
+ goto out;
}

/*
* pty code needs to hold extra references in case of last /dev/tty close
*/
+ sb = path.mnt->mnt_sb;
atomic_inc(&sb->s_active);
result = DEVPTS_SB(sb);

diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 277ab9af9ac2..e27c548acfb0 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -19,6 +19,7 @@

struct pts_fs_info;

+struct vfsmount *devpts_mnt(struct file *);
struct pts_fs_info *devpts_acquire(struct file *);
void devpts_release(struct pts_fs_info *);

@@ -32,6 +33,15 @@ void *devpts_get_priv(struct dentry *);
/* unlink */
void devpts_pty_kill(struct dentry *);

+/* in pty.c */
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags);
+
+#else
+static inline int
+ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
+{
+ return -EIO;
+}
#endif