Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753445AbdHXSlE (ORCPT ); Thu, 24 Aug 2017 14:41:04 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:48998 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752974AbdHXSlC (ORCPT ); Thu, 24 Aug 2017 14:41:02 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Linus Torvalds Cc: Stefan Lippers-Hollmann , Christian Brauner , Christian Brauner , Linux Kernel Mailing List , "Serge E. Hallyn" , Al Viro , Thorsten Leemhuis References: <20170816171211.4021-1-christian.brauner@ubuntu.com> <87pobvruzt.fsf@xmission.com> <87ziazqdfr.fsf@xmission.com> <20170824022436.44adb497@mir> <87378hhi3y.fsf@xmission.com> <87wp5tfynr.fsf@xmission.com> <20170824062432.1e05e6f8@mir> <874lsxezal.fsf@xmission.com> Date: Thu, 24 Aug 2017 13:40:39 -0500 In-Reply-To: (Linus Torvalds's message of "Thu, 24 Aug 2017 11:13:02 -0700") Message-ID: <87y3q8ermg.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dkx3l-0003Tu-CM;;;mid=<87y3q8ermg.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.200.44;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/fb/Q4ckhdrWqvEWOSexEPOLGQ+fX/FcQ= X-SA-Exim-Connect-IP: 67.3.200.44 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa05 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_01 4+ unique symbols in subject * 0.0 T_TooManySym_02 5+ unique symbols in subject * 0.0 T_TooManySym_03 6+ unique symbols in subject * 1.0 T_XMDrugObfuBody_12 obfuscated drug references X-Spam-DCC: XMission; sa05 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ***;Linus Torvalds X-Spam-Relay-Country: X-Spam-Timing: total 5715 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 3.6 (0.1%), b_tie_ro: 2.4 (0.0%), parse: 2.2 (0.0%), extract_message_metadata: 37 (0.7%), get_uri_detail_list: 10 (0.2%), tests_pri_-1000: 17 (0.3%), tests_pri_-950: 1.48 (0.0%), tests_pri_-900: 1.19 (0.0%), tests_pri_-400: 48 (0.8%), check_bayes: 47 (0.8%), b_tokenize: 23 (0.4%), b_tok_get_all: 13 (0.2%), b_comp_prob: 3.9 (0.1%), b_tok_touch_all: 4.4 (0.1%), b_finish: 0.70 (0.0%), tests_pri_0: 494 (8.7%), check_dkim_signature: 0.74 (0.0%), check_dkim_adsp: 2.8 (0.0%), tests_pri_500: 5104 (89.3%), poll_dns_idle: 5090 (89.1%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9783 Lines: 331 Linus Torvalds writes: > On Thu, Aug 24, 2017 at 11:06 AM, Linus Torvalds > 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. It still retains two of my bugs. I forgot to return from ioctl in tty_io.c without which the return code is lost. I failed to verify that the devpts filesystem we find via path lookup is the same one that was found when /dev/ptmx was opened. Here is my tested version of the patch. It survives light testing here. Eric From: "Eric W. Biederman" Date: Thu, 24 Aug 2017 10:55:19 -0500 Subject: [PATCH] pty: Repair TIOCGPTPEER The implementation of TIOCGPTPEER has two issues. When /dev/ptmx (as opposed to /dev/pts/ptmx) is opened the wrong vfsmount is passed to dentry_open. Which results in the kernel displaying the wrong pathname for the peer. The second is simply by caching the vfsmount and dentry of the peer it leaves them open, in a way they were not previously Which because of the inreased reference counts can cause unnecessary behaviour differences resulting in regressions. To fix these move the ioctl into tty_io.c at a generic level allowing the ioctl to have access to the struct file on which the ioctl is being called. This allows the path of the slave to be derived when opening the slave through TIOCGPTPEER instead of requiring the path to the slave be cached. Thus removing the need for caching the path. A new function devpts_ptmx_path is factored out of devpts_acquire and used to implement a function devpts_mntget. The new function devpts_mntget takes a filp to perform the lookup on and fsi so that it can confirm that the superblock that is found by devpts_ptmx_path is the proper superblock. Fixes: 54ebbfb16034 ("tty: add TIOCGPTPEER ioctl") Reported-by: Christian Brauner Reported-by: Stefan Lippers-Hollmann Signed-off-by: "Eric W. Biederman" --- drivers/tty/pty.c | 62 +++++++++++++++++++-------------------------- drivers/tty/tty_io.c | 3 +++ fs/devpts/inode.c | 64 ++++++++++++++++++++++++++++++++++++----------- include/linux/devpts_fs.h | 10 ++++++++ 4 files changed, 89 insertions(+), 50 deletions(-) diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 284749fb0f6b..3856bd228fa9 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; 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_mntget(master, tty->driver_data); + 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..10c4038c0e8d 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: + /* Special because the struct file is needed */ + return ptm_open_peer(file, tty, (int)arg); 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..809da242a452 100644 --- a/fs/devpts/inode.c +++ b/fs/devpts/inode.c @@ -133,37 +133,73 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb) return sb->s_fs_info; } -struct pts_fs_info *devpts_acquire(struct file *filp) +static int devpts_ptmx_path(struct path *path) { - struct pts_fs_info *result; - struct path path; struct super_block *sb; int err; - path = filp->f_path; - path_get(&path); - /* Has the devpts filesystem already been found? */ - sb = path.mnt->mnt_sb; + 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); + err = path_pts(path); + if (err) goto out; - } /* Is the path the root of a devpts filesystem? */ - result = ERR_PTR(-ENODEV); - sb = path.mnt->mnt_sb; + err = -ENODEV; + sb = path->mnt->mnt_sb; if ((sb->s_magic != DEVPTS_SUPER_MAGIC) || - (path.mnt->mnt_root != sb->s_root)) + (path->mnt->mnt_root != sb->s_root)) goto out; } + err = 0; +out: + return err; +} + +struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi) +{ + struct path path; + int err; + + path = filp->f_path; + path_get(&path); + + err = devpts_ptmx_path(&path); + dput(path.dentry); + if (err) { + mntput(path.mnt); + path.mnt = ERR_PTR(err); + } + if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) { + mntput(path.mnt); + path.mnt = ERR_PTR(-ENODEV); + } + return path.mnt; +} + +struct pts_fs_info *devpts_acquire(struct file *filp) +{ + struct pts_fs_info *result; + struct path path; + struct super_block *sb; + int err; + + path = filp->f_path; + path_get(&path); + + 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..100cb4343763 100644 --- a/include/linux/devpts_fs.h +++ b/include/linux/devpts_fs.h @@ -19,6 +19,7 @@ struct pts_fs_info; +struct vfsmount *devpts_mntget(struct file *, struct pts_fs_info *); 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 -- 2.14.1