Re: [PATCH] devpts: resolve devpts bind-mounts

[Eric W. Biederman]

Christian Brauner <christian.brauner@xxxxxxxxxx> writes:

> Most libcs will still look at /dev/ptmx when opening the master fd of pty
> device. When /dev/ptmx is a bind-mount of /dev/pts/ptmx and the TIOCGPTPEER
> ioctl() is used to safely retrieve a file descriptor for the slave side of the
> pty based on the master fd the /proc/self/fd/{0,1,2} symlinks will point to "/".
> When the kernel tries to look up the root mount of the dentry for the slave
> file descriptor it will detect that the dentry is escaping it's bind-mount
> since the root mount of the dentry is /dev/pts where the devpts is mounted but
> the root mount of /dev/ptmx is /dev.
> Having bind-mounts of /dev/pts/ptmx to /dev/ptmx not working correctly is a
> regression. In addition, it is also a fairly common scenario in containers
> employing user namespaces.
> To handle this situation correctly we walk up the bind-mounts for the /dev/ptmx
> file.
>
> Here's a little reproducer that presupposes a libc that uses TIOCGPTPEER in its
> openpty() implementation:
>
> unshare --mount
> mount --bind /dev/pts/ptmx /dev/ptmx
> chmod 666 /dev/ptmx
> script
> ls -al /proc/self/fd/0
>
> with output:
>
> lrwx------ 1 chb chb 64 Mar  7 16:41 /proc/self/fd/0 -> /

I have two concerns.
1) By leaving the test:

	/* Has the devpts filesystem already been found? */
	if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
		return 0;

In the version of devpts_ptmx_path that you call in the new code
there is a chance it will trip up on that test and return early.
Not that we are likely to hit that in practice, but let's be clear
about what we are doing.

2) After the call of devpts_ptmx_path in the new code there is not
   a check that the returned mount is the filesystem we are looking
   for.  AKA it is missing a "DEVPTS_SB(path.mnt->mnt_sb) == fsi" test.

Eric


> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> Suggested-by: Eric Biederman <ebiederm@xxxxxxxxxxxx>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> ---
>  fs/devpts/inode.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index e31d6ed3ec32..4059e3e69d57 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -163,6 +163,26 @@ struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
>  
>  	path = filp->f_path;
>  	path_get(&path);
> +	if ((DEVPTS_SB(path.mnt->mnt_sb) == fsi) &&
> +	    (path.mnt->mnt_root == fsi->ptmx_dentry)) {
> +		/* Walk upward while the start point is a bind mount of a single
> +		 * file.
> +		 */
> +		while (path.mnt->mnt_root == path.dentry)
> +			if (follow_up(&path) == 0)
> +				break;
> +
> +		/* Is this path a valid devpts filesystem? */
> +		err = devpts_ptmx_path(&path);
> +		if (err == 0) {
> +			dput(path.dentry);
> +			return path.mnt;
> +		}
> +
> +		path_put(&path);
> +		path = filp->f_path;
> +		path_get(&path);
> +	}
>  
>  	err = devpts_ptmx_path(&path);
>  	dput(path.dentry);