| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160405025425.GK17997@ZenIV.linux.org.uk>
Date: Tue, 5 Apr 2016 03:54:25 +0100
From: Al Viro <viro@...IV.linux.org.uk>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
"H. Peter Anvin" <hpa@...or.com>,
Peter Hurley <peter@...leysoftware.com>,
Greg KH <greg@...ah.com>, Jiri Slaby <jslaby@...e.com>,
Aurelien Jarno <aurelien@...el32.net>,
Andy Lutomirski <luto@...capital.net>,
Florian Weimer <fw@...eb.enyo.de>,
Serge Hallyn <serge.hallyn@...ntu.com>,
Jann Horn <jann@...jh.net>,
"security@...nel.org" <security@...nel.org>, security@...ntu.com,
security@...ian.org, Willy Tarreau <w@....eu>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated
devpts via path lookup
On Mon, Apr 04, 2016 at 08:29:17PM -0500, Eric W. Biederman wrote:
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +static int devpts_path_ptmx(struct file *filp)
> +{
> + struct pts_fs_info *fsi;
> + struct path root, path;
> + struct dentry *old;
> + int err = -ENOENT;
> + int ret;
> +
> + /* Can the pts filesystem be found with a path walk? */
> + path = filp->f_path;
> + path_get(&path);
> + get_fs_root(current->fs, &root);
> + ret = path_parent(&root, &path);
> + path_put(&root);
> + if (ret != 1)
> + goto fail;
That, I take it, is a lookup for .. and buggering off if it fails *or* if
we had been in caller's root or something that overmount it? Not that the
latter had been possible - root is a directory and can be overmounted only
by another such, and we are called from ->open() of a device node.
> + /* Remember the result of this permission check for later */
> + ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
> + if (path_pts(&path))
> + goto fail;
Egads, man - you've just introduced a special function for looking up
something named "pts" in a given directory!
The reason not to use kern_path() would be what, the fact that it doesn't
allow starting at given location? So let's make a variant that would - and
rather than bothering with RCU, just go for something like (completely
untested)
/* on success overwrite *path with the result of walk; do _not_ drop the
reference to old contents - let the caller arrange that */
int kern_path_relative(struct path *path, const char *s, int flags)
{
int err;
struct nameidata nd = {.path = *path};
struct filename *name;
if (!*s || *s == '/' || flags & (LOOKUP_ROOT | LOOKUP_RCU))
return -EINVAL;
name = getname_kernel(s);
if (IS_ERR(name))
return PTR_ERR(name);
set_nameidata(&nd, AT_FDCWD, name);
nd.last_type = LAST_ROOT;
nd.flags = flags | LOOKUP_REVAL | LOOKUP_JUMPED | LOOKUP_PARENT;
nd.m_seq = read_seqbegin(&mount_lock);
path_get(&nd.path);
nd.inode = nd.path.dentry->d_inode;
while (!(err = link_path_walk(s, &nd))
&& ((err = lookup_last(&nd)) > 0)) {
s = trailing_symlink(&nd);
if (IS_ERR(s)) {
err = PTR_ERR(s);
break;
}
}
if (!err)
err = complete_walk(&nd);
if (!err && flags & LOOKUP_DIRECTORY)
if (!d_can_lookup(nd.path.dentry))
err = -ENOTDIR;
if (!err) {
*path = nd.path;
nd.path.mnt = NULL;
nd.path.dentry = NULL;
}
terminate_walk(&nd);
restore_nameidata();
putname(name);
return err;
}
and use it as
path = filp->f_path;
err = kern_path_relative(&path, "../pts", LOOKUP_DIRECTORY);
if (err)
return err;
/* from here on we need to path_put() it */
if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
goto fail;
/* must be its root; no other directories on that puppy */
> + fsi = DEVPTS_SB(path.mnt->mnt_sb);
> +
> + /* Get out if the path walk resulted in the default devpts instance */
> + if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
> + goto fail;
> +
> + /* Don't allow bypassing the existing /dev/pts/ptmx permission check */
err = inode_permission(path.dentry->d_inode, MAY_EXEC);
if (err)
goto fail;
err = inode_permission(fsi->ptmx_dentry->d_inode,
ACC_MODE(filp->f_flags));
if (err)
goto fail;
> + /* Advance path to the ptmx dentry */
> + old = path.dentry;
> + path.dentry = dget(fsi->ptmx_dentry);
> + dput(old);
> +
> + /* Make it look like /dev/pts/ptmx was opened */
> + err = update_file_path(filp, &path);
> + if (err)
> + goto fail;
> +
> + return 0;
> +fail:
> + path_put(&path);
> + return err;
> +}
> +#else
> +static inline int devpts_path_ptmx(struct file *filp)
> +{
> + return -ENOENT;
> +}
> +#endif
> +
> +struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
> +{
> + int err;
> + if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> + return inode;
> +
> + err = devpts_path_ptmx(filp);
> + if (err == 0)
> + return filp->f_inode;
> + if (err != -ENOENT)
> + return ERR_PTR(err);
> +
> + return inode;
> +}
Umm... I'm not sure it makes for good calling conventions - the caller can
do inode = file_inode(filp) just as well, so why not simply return 0 or -E...?
"return inode;" cases become simply return 0...
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1415,29 +1415,41 @@ static void follow_mount(struct path *path)
> }
> }
>
> -static int follow_dotdot(struct nameidata *nd)
> +int path_parent(struct path *root, struct path *path)
Please, don't.
> +#ifdef CONFIG_UNIX98_PTYS
> +int path_pts(struct path *path)
Fuck, no.
> index 17cb6b1dab75..e1ed78fa474b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -679,6 +679,24 @@ int open_check_o_direct(struct file *f)
> return 0;
> }
>
> +int update_file_path(struct file *filp, struct path *path)
> +{
> + /* Only valid during f_op->open, and even in open use very carefully */
> + struct path old;
> + struct inode *inode;
> +
> + if (filp->f_mode & FMODE_WRITER)
> + return -EINVAL;
That really needs to be commented.
> + old = filp->f_path;
> + inode = path->dentry->d_inode;
> + filp->f_path = *path;
> + filp->f_inode = inode;
> + filp->f_mapping = inode->i_mapping;
> + path_put(&old);
> + return 0;
> +}
> +
> static int do_dentry_open(struct file *f,
> struct inode *inode,
> int (*open)(struct inode *, struct file *),
> @@ -736,6 +754,7 @@ static int do_dentry_open(struct file *f,
> error = open(inode, f);
> if (error)
> goto cleanup_all;
> + inode = f->f_inode;
> }
> if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
> i_readcount_inc(inode);
BTW, have you looked through the callers of dentry_open()? It can hit that
case as well...
Powered by blists - more mailing lists