devpts: handle bind-mounts
| Affects | Status | Importance | Assigned to | Milestone | |
|---|---|---|---|---|---|
| linux (Ubuntu) |
Fix Released
|
High
|
Unassigned | ||
| Bionic |
Fix Released
|
High
|
Unassigned | ||
Bug Description
Hey everyone,
I sent a few patches to fix a regression caused by wrong behavior in the devpts filesystem when used with the TIOCGPTPEER ioctl() we implemented a while back. The context and reproducer can be taken from my following commit message. The required patches are in Greg's tree and are:
1. https:/
2. https:/
3. https:/
4. https:/
devpts: resolve devpts bind-mounts
Most libcs will still look at /dev/ptmx when opening the master fd of a 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/
point to /. A very simply reproducer for this issue presupposing a libc
that uses TIOCGPTPEER in its openpty() implementation is:
unshare --mount
mount --bind /dev/pts/ptmx /dev/ptmx
chmod 666 /dev/ptmx
script
ls -al /proc/self/fd/0
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.
The reason for the current failure is that the kernel tries to verify the
useability of the devpts filesystem without resolving the /dev/ptmx
bind-mount first. This will lead it to detect that the dentry is escaping
its bind-mount. The reason is that while the devpts filesystem mounted at
/dev/pts has the devtmpfs mounted at /dev as its parent mount:
21 -- -- / /dev
-- 21 -- / /dev/pts
devtmpfs and devpts are on different devices
-- -- 0:6 / /dev
-- -- 0:20 / /dev/pts
This has the consequence that the pathname of the parent directory of the
devpts filesystem mount at /dev/pts is /. So if /dev/ptmx is a bind-mount
of /dev/pts/ptmx then the /dev/ptmx bind-mount and the devpts mount at
/dev/pts will end up being located on the same device which is recorded in
the superblock of their vfsmount. This means the parent directory of the
/dev/ptmx bind-mount will be /ptmx:
-- -- ---- /ptmx /dev/ptmx
Without the bind-mount resolution patch the kernel will now perform the
bind-mount escape check directly on /dev/ptmx. The function responsible for
this is devpts_ptmx_path() which calls pts_path() which in turn calls
path_parent_
path_parent_
/dev/ptmx bind-mount and not the expected /dev. Thus, the kernel detects
that /dev/ptmx is escaping its bind-mount and will set /proc/<pid>/fd/<nr>
to /.
This patch changes the logic to first resolve any bind-mounts. After the
bind-mounts have been resolved (i.e. we have traced it back to the
associated devpts mount) devpts_ptmx_path() can be called. In order to
guarantee correct path generation for the slave file descriptor the kernel
now requires that a pts directory is found in the parent directory of the
ptmx bind-mount. This implies that when doing bind-mounts the ptmx
bind-mount and the devpts mount should have a common parent directory. A
valid example is:
mount -t devpts devpts /dev/pts
mount --bind /dev/pts/ptmx /dev/ptmx
an invalid example is:
mount -t devpts devpts /dev/pts
mount --bind /dev/pts/ptmx /ptmx
This allows us to support:
- calling open on ptmx devices located inside non-standard devpts mounts:
mount -t devpts devpts /mnt
master = open("/mnt/ptmx", ...);
slave = ioctl(master, TIOCGPTPEER, ...);
- calling open on ptmx devices located outside the devpts mount with a
common ancestor directory:
mount -t devpts devpts /dev/pts
mount --bind /dev/pts/ptmx /dev/ptmx
master = open("/dev/ptmx", ...);
slave = ioctl(master, TIOCGPTPEER, ...);
while failing on ptmx devices located outside the devpts mount without a
common ancestor directory:
mount -t devpts devpts /dev/pts
mount --bind /dev/pts/ptmx /ptmx
master = open("/ptmx", ...);
slave = ioctl(master, TIOCGPTPEER, ...);
in which case save path generation cannot be guaranteed.
CVE References
| Christian Brauner (cbrauner) wrote | #1 |
| Ubuntu Kernel Bot (ubuntu-kernel-bot) wrote Missing required logs. | #2 |
| Changed in linux (Ubuntu): | |
| status: | New → Incomplete |
| Changed in linux (Ubuntu): | |
| status: | Incomplete → Confirmed |
| Joseph Salisbury (jsalisbury) wrote | #3 |
| Changed in linux (Ubuntu Bionic): | |
| importance: | Undecided → High |
| status: | Confirmed → Triaged |
| tags: | added: bionic kernel-da-key |
| Christian Brauner (cbrauner) wrote | #4 |
| Changed in linux (Ubuntu Bionic): | |
| status: | Triaged → Fix Committed |
| Launchpad Janitor (janitor) wrote | #5 |
| Changed in linux (Ubuntu Bionic): | |
| status: | Fix Committed → Fix Released |
| Brad Figg (brad-figg) wrote | #6 |
| tags: | added: verification-needed-bionic |
| tags: |
added: kernel-fixup-verification-needed-bionic removed: verification-needed-bionic |
| tags: | added: verification-needed-bionic |
| Andy Whitcroft (apw) wrote | #8 |
| tags: | removed: verification-needed-bionic |
| tags: | added: verification-done-bionic |
| tags: | added: cscc |
Oh sorry, it would be great if we could cherry-pick these as this currently breaks users. Sorry for all the pre-release extra work this causes. :)
Christian