Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

[Christian Brauner]

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 <ebiederm@xmission.com>
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.

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.).

Christian

[1]: https://lkml.org/lkml/2017/8/23/826

Re: Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

[Eric W. Biederman]

Christian Brauner <christian.brauner@canonical.com> 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 <ebiederm@xmission.com>
> 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) {
                	dput(path.dentry);
                   	return path.mnt;     
                }
                /* No luck fall through to the old code */
                path_put(path);
                path = filp->f_path;
                path_get(&path);
        }

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

Eric

Re: Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

[Linus Torvalds]

On Wed, Mar 7, 2018 at 8:17 AM, Christian Brauner
<christian.brauner@canonical.com> wrote:
>
> unshare --mount
> mount --bind /dev/pts/ptmx /dev/ptmx
> chmod 666 /dev/ptmx

Oh. Why are you using a bind mount in the first place?

Anyway, I guess we just have to add another special case for this.

Which doesn't look horrible. Right now path_pts() just does

        ret = path_parent_directory(path);

and that simply doesn't work for a bind mount file.

I think we could just change path_parent_directory() to go through
file bind mounts. The other user is follow_dotdot(), but that always
takes a directory, so it wouldn't be affected.

But it's probably safer to just teach path_pts to just walk up the
bind mount first, and then do the existing path_parent_directory.

Anybody want to just try that thing?

            Linus

Re: Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

[Christian Brauner]

On Wed, Mar 07, 2018 at 11:44:35AM -0800, Linus Torvalds wrote:
> On Wed, Mar 7, 2018 at 8:17 AM, Christian Brauner
> <christian.brauner@canonical.com> wrote:
> >
> > unshare --mount
> > mount --bind /dev/pts/ptmx /dev/ptmx
> > chmod 666 /dev/ptmx
> 
> Oh. Why are you using a bind mount in the first place?

Containers employing user namespaces can't mknod() and because of the
way some LSMs check access permissions (path-based AppArmor being one
example) a symlink to /dev/pts/ptmx won't work either so a bind-mount
seems like the most reliable solution.

> 
> Anyway, I guess we just have to add another special case for this.
> 
> Which doesn't look horrible. Right now path_pts() just does
> 
>         ret = path_parent_directory(path);
> 
> and that simply doesn't work for a bind mount file.
> 
> I think we could just change path_parent_directory() to go through
> file bind mounts. The other user is follow_dotdot(), but that always
> takes a directory, so it wouldn't be affected.
> 
> But it's probably safer to just teach path_pts to just walk up the
> bind mount first, and then do the existing path_parent_directory.
> 
> Anybody want to just try that thing?

Sure. I can try and take a look.

Christian

Re: Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

[Christian Brauner]

On Wed, Mar 07, 2018 at 01:30:52PM -0600, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@canonical.com> 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 <ebiederm@xmission.com>
> > 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.

It's at least a regression since this used to work before. :)

> 
> > 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.

It's quite important for containers. The problem is that we can't (yet)
mknod() in a user namespace and making /dev/ptmx a symlink to
/dev/pts/ptmx will cause issues when used together with path-based LSMs
like AppArmor so a bind-mount is the only reliable option.

> 
> The code can be improved by doing something like:

Right, what do you think about Linus suggestion? I'm happy to look into
it.

Christian

> 
> 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) {
>                 	dput(path.dentry);
>                    	return path.mnt;     
>                 }
>                 /* No luck fall through to the old code */
>                 path_put(path);
>                 path = filp->f_path;
>                 path_get(&path);
>         }
> 
> The fall through vs fail would be a judgement on how important it is to
> have a useable path in proc for TIOCPTPEER.
> 
> Eric

Re: Invalid /proc/<pid>/fd/{0,1,2} symlinks with TIOCGPTPEER

[Christian Brauner]

On Thu, Mar 08, 2018 at 09:22:29AM +0100, Christian Brauner wrote:
> On Wed, Mar 07, 2018 at 01:30:52PM -0600, Eric W. Biederman wrote:
> > Christian Brauner <christian.brauner@canonical.com> 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 <ebiederm@xmission.com>
> > > 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.
> 
> It's at least a regression since this used to work before. :)
> 
> > 
> > > 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.
> 
> It's quite important for containers. The problem is that we can't (yet)
> mknod() in a user namespace and making /dev/ptmx a symlink to
> /dev/pts/ptmx will cause issues when used together with path-based LSMs
> like AppArmor so a bind-mount is the only reliable option.
> 
> > 
> > The code can be improved by doing something like:
> 
> Right, what do you think about Linus suggestion? I'm happy to look into
> it.

Ah you're pointing at a similar solution. Sorry, I missed it on the
first pass. I'll send a patch soon.

Christian

> 
> Christian
> 
> > 
> > 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) {
> >                 	dput(path.dentry);
> >                    	return path.mnt;     
> >                 }
> >                 /* No luck fall through to the old code */
> >                 path_put(path);
> >                 path = filp->f_path;
> >                 path_get(&path);
> >         }
> > 
> > The fall through vs fail would be a judgement on how important it is to
> > have a useable path in proc for TIOCPTPEER.
> > 
> > Eric