[lkml]   [2018]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER
Christian Brauner <> writes:

> Hey,
> We discovered a potential bug in the devpts implementation via
> TIOCGPTPEER ioctl()s today. We've tackled a similar problem already in:
> commit 311fc65c9fb9c966bca8e6f3ff8132ce57344ab9
> Author: Eric W. Biederman <>
> Date: Thu Aug 24 15:13:29 2017 -0500
> pty: Repair TIOCGPTPEER
> Most libcs will *still* look at /dev/ptmx when opening the master fd of
> pty device. Usually, /dev/ptmx will nowadays be either a symlink to
> /dev/pts/ptmx or it will be a second device node with permissions 666
> whereas /dev/pts/ptmx will usually have permissions 000. Afaik, we've
> also always supported making /dev/ptmx a bind-mount to /dev/pts/ptmx or
> at least I haven't observed any issues with this so far and it's
> something fairly common in containers. So in short, it should be legal
> to do:
> mount --bind /dev/pts/ptmx /dev/ptmx
> chmod 666 /dev/ptmx
> However, for any libc implementation or program that uses TIOCGPTPEER
> the /proc/<pid>/fd/{0,1,2} symlinks are broken (currently affects at
> least glibc 2.27) with bind-mounts of /dev/pts/ptmx to /dev/ptmx. A
> quick reproducer is:
> unshare --mount
> mount --bind /dev/pts/ptmx /dev/ptmx
> chmod 666 /dev/ptmx
> script
> ls -al /proc/self/fd/0
> Let's assume the slave device index I received was 5 then I would expect to
> see:
> ls -al /proc/self/fd/0
> lrwx------ 1 chb chb 64 Mar 7 16:41 /proc/self/fd/0 -> /dev/pts/5
> But what I actually see is:
> ls -al /proc/self/fd/0
> lrwx------ 1 chb chb 64 Mar 7 16:41 /proc/self/fd/0 -> /
> I think the explanation for this is fairly straightforward. When
> userspace does:
> master = open("/dev/ptmx", O_RDWR | O_NOCTTY);
> slave = ioctl(master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> and /dev/ptmx is a bind-mount of /dev/pts/ptmx looking up the root mount
> of the dentry for the slave it appears to the kernel as if the dentry is
> escaping it's bind-mount:
> ├─/dev udev devtmpfs rw,nosuid,relatime,size=4001260k,nr_inodes=1000315,mode=755
> │ ├─/dev/pts devpts devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000
> │ └─/dev/ptmx devpts[/ptmx] devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000
> since the root mount of the dentry is /dev/pts but the root mount of
> /dev/ptmx is /dev if I'm correct so similar to what Linus pointed out in
> a previous discussion (see [1]) before. So we still record the "wrong"
> vfsmount when /dev/ptmx is a bind-mount and then hit the problem when we
> call devpts_mntget() in drivers/tty/pty.c.

I think your analysis of why we return / is correct. If the root of the
mount is a file (aka /dev/pts/ptmx). Then any other file will on that
mount will not be under the root of the mount, and will be displayed
as '/'. Because we have in fact escaped the root of the mount.

I think this is more of a quality of implementation issue more than a
bug per se.

> So I thought about this and - in case my analysis is correct - the
> solution didn't seem obvious to me as a bind-mount has no concept of
> what it's "parent" is (Which in this case should be the devpts mount at
> /dev/pts.).

We might be able to improve the quality of the implementation, by
noticing this case early (sb->s_root != mnt->mnt_root) and using the
same tricks on /dev/pts/ptmx as we do on /dev/ptmx. That is looking
in ../pts and see if the filesystem we want is there.

It would be a wee bit tricky but doable. The practical question becomes
what breaks and what makes it worth maintaining such a mechanism.

I don't remember how important it is to have a valid path in proc. So
I won't comment on how important it is to improve the quality of
the implementation.

The code can be improved by doing something like:

static int devpts_ptmx_pts_path(struct path *path)
struct super_block *sb;
int err;

/* Is a devpts filesystem at "pts" in the same directory? */
err = path_pts(path);
if (err)
return err;

/* Is the path the root of a devpts filesystem? */
sb = path->mnt->mnt_sb;
if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
(path->mnt->mnt_root != sb->s_root))
return -ENODEV;

return 0;

if ((DEVPTS_SB(path.mnt->mnt_sb) == fsi) &&
(path.mnt->mnt_root == fsi->ptmx_dentry)) {
/* While the start point is a bind mount of single file
* walk upwards.
while ((path.mnt->mnt_root == path.dentry) && follow_up(&path))
if (devpts_ptmx_pts_path(&path) == 0) {
return path.mnt;
/* No luck fall through to the old code */
path = filp->f_path;

The fall through vs fail would be a judgement on how important it is to
have a useable path in proc for TIOCPTPEER.


 \ /
  Last update: 2018-03-07 20:31    [W:0.059 / U:1.112 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site