Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752950AbbLKTtW (ORCPT ); Fri, 11 Dec 2015 14:49:22 -0500 Received: from out01.mta.xmission.com ([166.70.13.231]:50888 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751417AbbLKTtT (ORCPT ); Fri, 11 Dec 2015 14:49:19 -0500 From: ebiederm@xmission.com (Eric W. Biederman) To: Greg KH , Jiri Slaby Cc: "H. Peter Anvin" , Linus Torvalds , Aurelien Jarno , Andy Lutomirski , Florian Weimer , Al Viro , Serge Hallyn , Jann Horn , "security\@kernel.org" , "security\@ubuntu.com \>\> security" , security@debian.org, Willy Tarreau , References: <43AD2BA7-B594-4299-95F3-D86FD38AF21B@zytor.com> <87egexpf4o.fsf@x220.int.ebiederm.org> <1CB621EF-1647-463B-A144-D611DB150E15@zytor.com> <20151208223135.GA8352@kroah.com> <87oae0h2bo.fsf@x220.int.ebiederm.org> <56677DE3.5040705@zytor.com> <20151209012311.GA11794@kroah.com> <84B136DF-55E4-476A-9CB2-062B15677EE5@zytor.com> <20151209013859.GA12442@kroah.com> <20151209083225.GA30452@1wt.eu> Date: Fri, 11 Dec 2015 13:40:40 -0600 In-Reply-To: <20151209083225.GA30452@1wt.eu> (Willy Tarreau's message of "Wed, 9 Dec 2015 09:32:26 +0100") Message-ID: <87wpskyds7.fsf_-_@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX19K5d5IyVAHl5juK4jFWLRjelO6rYM3b2E= X-SA-Exim-Connect-IP: 70.59.167.217 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.3 TooManyTo_001 Multiple "To" Header Recipients 2x (uncommon) * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Greg KH , Jiri Slaby X-Spam-Relay-Country: X-Spam-Timing: total 758 ms - load_scoreonly_sql: 0.07 (0.0%), signal_user_changed: 4.9 (0.7%), b_tie_ro: 3.5 (0.5%), parse: 1.51 (0.2%), extract_message_metadata: 31 (4.1%), get_uri_detail_list: 10 (1.3%), tests_pri_-1000: 13 (1.7%), tests_pri_-950: 2.1 (0.3%), tests_pri_-900: 1.85 (0.2%), tests_pri_-400: 53 (7.0%), check_bayes: 52 (6.8%), b_tokenize: 26 (3.5%), b_tok_get_all: 13 (1.7%), b_comp_prob: 5 (0.7%), b_tok_touch_all: 4.2 (0.6%), b_finish: 0.75 (0.1%), tests_pri_0: 639 (84.3%), check_dkim_signature: 0.91 (0.1%), check_dkim_adsp: 4.2 (0.6%), tests_pri_500: 5 (0.7%), rewrite_mail: 0.00 (0.0%) Subject: [PATCH] devpts: Sensible /dev/ptmx & force newinstance X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -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: 8059 Lines: 249 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 --- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/