linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] tty: add TIOCGPTPEER ioctl
@ 2017-06-03 14:15 Aleksa Sarai
  2017-06-03 14:15 ` [PATCH v4 1/2] tty: add compat_ioctl callbacks Aleksa Sarai
  2017-06-03 14:15 ` [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
  0 siblings, 2 replies; 8+ messages in thread
From: Aleksa Sarai @ 2017-06-03 14:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Arnd Bergmann
  Cc: linux-kernel, linux-alpha, linux-mips, linux-parisc,
	linuxppc-dev, linux-sh, sparclinux, linux-xtensa, linux-arch,
	Christian Brauner, Valentin Rothberg, Aleksa Sarai

Changes in v4:
  * Dropped an accidental change to Makefile.

Aleksa Sarai (2):
  tty: add compat_ioctl callbacks
  tty: add TIOCGPTPEER ioctl

 arch/alpha/include/uapi/asm/ioctls.h   |  1 +
 arch/mips/include/uapi/asm/ioctls.h    |  1 +
 arch/parisc/include/uapi/asm/ioctls.h  |  1 +
 arch/powerpc/include/uapi/asm/ioctls.h |  1 +
 arch/sh/include/uapi/asm/ioctls.h      |  1 +
 arch/sparc/include/uapi/asm/ioctls.h   |  3 +-
 arch/xtensa/include/uapi/asm/ioctls.h  |  1 +
 drivers/tty/pty.c                      | 93 ++++++++++++++++++++++++++++++++--
 fs/compat_ioctl.c                      |  6 ---
 include/uapi/asm-generic/ioctls.h      |  1 +
 10 files changed, 98 insertions(+), 11 deletions(-)

-- 
2.13.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 1/2] tty: add compat_ioctl callbacks
  2017-06-03 14:15 [PATCH v4 0/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
@ 2017-06-03 14:15 ` Aleksa Sarai
  2017-06-06 12:49   ` Arnd Bergmann
  2017-06-03 14:15 ` [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
  1 sibling, 1 reply; 8+ messages in thread
From: Aleksa Sarai @ 2017-06-03 14:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Arnd Bergmann
  Cc: linux-kernel, linux-alpha, linux-mips, linux-parisc,
	linuxppc-dev, linux-sh, sparclinux, linux-xtensa, linux-arch,
	Christian Brauner, Valentin Rothberg, Aleksa Sarai

In order to avoid future diversions between fs/compat_ioctl.c and
drivers/tty/pty.c, define .compat_ioctl callbacks for the relevant
tty_operations structs. Since both pty_unix98_ioctl() and
pty_bsd_ioctl() are compatible between 32-bit and 64-bit userspace no
special translation is required.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 drivers/tty/pty.c | 22 ++++++++++++++++++++++
 fs/compat_ioctl.c |  6 ------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 65799575c666..2a6bd9ae3f8b 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -481,6 +481,16 @@ static int pty_bsd_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static long pty_bsd_compat_ioctl(struct tty_struct *tty,
+				 unsigned int cmd, unsigned long arg)
+{
+	/*
+	 * PTY ioctls don't require any special translation between 32-bit and
+	 * 64-bit userspace, they are already compatible.
+	 */
+	return pty_bsd_ioctl(tty, cmd, arg);
+}
+
 static int legacy_count = CONFIG_LEGACY_PTY_COUNT;
 /*
  * not really modular, but the easiest way to keep compat with existing
@@ -502,6 +512,7 @@ static const struct tty_operations master_pty_ops_bsd = {
 	.chars_in_buffer = pty_chars_in_buffer,
 	.unthrottle = pty_unthrottle,
 	.ioctl = pty_bsd_ioctl,
+	.compat_ioctl = pty_bsd_compat_ioctl,
 	.cleanup = pty_cleanup,
 	.resize = pty_resize,
 	.remove = pty_remove
@@ -609,6 +620,16 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
 	return -ENOIOCTLCMD;
 }
 
+static long pty_unix98_compat_ioctl(struct tty_struct *tty,
+				 unsigned int cmd, unsigned long arg)
+{
+	/*
+	 * PTY ioctls don't require any special translation between 32-bit and
+	 * 64-bit userspace, they are already compatible.
+	 */
+	return pty_unix98_ioctl(tty, cmd, arg);
+}
+
 /**
  *	ptm_unix98_lookup	-	find a pty master
  *	@driver: ptm driver
@@ -681,6 +702,7 @@ static const struct tty_operations ptm_unix98_ops = {
 	.chars_in_buffer = pty_chars_in_buffer,
 	.unthrottle = pty_unthrottle,
 	.ioctl = pty_unix98_ioctl,
+	.compat_ioctl = pty_unix98_compat_ioctl,
 	.resize = pty_resize,
 	.cleanup = pty_cleanup
 };
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 6116d5275a3e..112b3e1e20e3 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -866,8 +866,6 @@ COMPATIBLE_IOCTL(TIOCGDEV)
 COMPATIBLE_IOCTL(TIOCCBRK)
 COMPATIBLE_IOCTL(TIOCGSID)
 COMPATIBLE_IOCTL(TIOCGICOUNT)
-COMPATIBLE_IOCTL(TIOCGPKT)
-COMPATIBLE_IOCTL(TIOCGPTLCK)
 COMPATIBLE_IOCTL(TIOCGEXCL)
 /* Little t */
 COMPATIBLE_IOCTL(TIOCGETD)
@@ -883,16 +881,12 @@ COMPATIBLE_IOCTL(TIOCMGET)
 COMPATIBLE_IOCTL(TIOCMBIC)
 COMPATIBLE_IOCTL(TIOCMBIS)
 COMPATIBLE_IOCTL(TIOCMSET)
-COMPATIBLE_IOCTL(TIOCPKT)
 COMPATIBLE_IOCTL(TIOCNOTTY)
 COMPATIBLE_IOCTL(TIOCSTI)
 COMPATIBLE_IOCTL(TIOCOUTQ)
 COMPATIBLE_IOCTL(TIOCSPGRP)
 COMPATIBLE_IOCTL(TIOCGPGRP)
-COMPATIBLE_IOCTL(TIOCGPTN)
-COMPATIBLE_IOCTL(TIOCSPTLCK)
 COMPATIBLE_IOCTL(TIOCSERGETLSR)
-COMPATIBLE_IOCTL(TIOCSIG)
 #ifdef TIOCSRS485
 COMPATIBLE_IOCTL(TIOCSRS485)
 #endif
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl
  2017-06-03 14:15 [PATCH v4 0/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
  2017-06-03 14:15 ` [PATCH v4 1/2] tty: add compat_ioctl callbacks Aleksa Sarai
@ 2017-06-03 14:15 ` Aleksa Sarai
  2017-06-09  9:26   ` Greg Kroah-Hartman
  1 sibling, 1 reply; 8+ messages in thread
From: Aleksa Sarai @ 2017-06-03 14:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Arnd Bergmann
  Cc: linux-kernel, linux-alpha, linux-mips, linux-parisc,
	linuxppc-dev, linux-sh, sparclinux, linux-xtensa, linux-arch,
	Christian Brauner, Valentin Rothberg, Aleksa Sarai

When opening the slave end of a PTY, it is not possible for userspace to
safely ensure that /dev/pts/$num is actually a slave (in cases where the
mount namespace in which devpts was mounted is controlled by an
untrusted process). In addition, there are several unresolvable
race conditions if userspace were to attempt to detect attacks through
stat(2) and other similar methods [in addition it is not clear how
userspace could detect attacks involving FUSE].

Resolve this by providing an interface for userpace to safely open the
"peer" end of a PTY file descriptor by using the dentry cached by
devpts. Since it is not possible to have an open master PTY without
having its slave exposed in /dev/pts this interface is safe. This
interface currently does not provide a way to get the master pty (since
it is not clear whether such an interface is safe or even useful).

Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Valentin Rothberg <vrothberg@suse.com>
Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 arch/alpha/include/uapi/asm/ioctls.h   |  1 +
 arch/mips/include/uapi/asm/ioctls.h    |  1 +
 arch/parisc/include/uapi/asm/ioctls.h  |  1 +
 arch/powerpc/include/uapi/asm/ioctls.h |  1 +
 arch/sh/include/uapi/asm/ioctls.h      |  1 +
 arch/sparc/include/uapi/asm/ioctls.h   |  3 +-
 arch/xtensa/include/uapi/asm/ioctls.h  |  1 +
 drivers/tty/pty.c                      | 71 ++++++++++++++++++++++++++++++++--
 include/uapi/asm-generic/ioctls.h      |  1 +
 9 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/arch/alpha/include/uapi/asm/ioctls.h b/arch/alpha/include/uapi/asm/ioctls.h
index f30c94ae1bdb..ff67b8373bf7 100644
--- a/arch/alpha/include/uapi/asm/ioctls.h
+++ b/arch/alpha/include/uapi/asm/ioctls.h
@@ -100,6 +100,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG	0x5453
 #define TIOCSERGWILD	0x5454
diff --git a/arch/mips/include/uapi/asm/ioctls.h b/arch/mips/include/uapi/asm/ioctls.h
index 740219c2c894..68e19b689a00 100644
--- a/arch/mips/include/uapi/asm/ioctls.h
+++ b/arch/mips/include/uapi/asm/ioctls.h
@@ -91,6 +91,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 /* I hope the range from 0x5480 on is free ... */
 #define TIOCSCTTY	0x5480		/* become controlling tty */
diff --git a/arch/parisc/include/uapi/asm/ioctls.h b/arch/parisc/include/uapi/asm/ioctls.h
index b6572f051b67..674c68a5bbd0 100644
--- a/arch/parisc/include/uapi/asm/ioctls.h
+++ b/arch/parisc/include/uapi/asm/ioctls.h
@@ -60,6 +60,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define FIONCLEX	0x5450  /* these numbers need to be adjusted. */
 #define FIOCLEX		0x5451
diff --git a/arch/powerpc/include/uapi/asm/ioctls.h b/arch/powerpc/include/uapi/asm/ioctls.h
index 49a25796a61a..bfd609a3e928 100644
--- a/arch/powerpc/include/uapi/asm/ioctls.h
+++ b/arch/powerpc/include/uapi/asm/ioctls.h
@@ -100,6 +100,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG	0x5453
 #define TIOCSERGWILD	0x5454
diff --git a/arch/sh/include/uapi/asm/ioctls.h b/arch/sh/include/uapi/asm/ioctls.h
index c9903e56ccf4..eec7901e9e65 100644
--- a/arch/sh/include/uapi/asm/ioctls.h
+++ b/arch/sh/include/uapi/asm/ioctls.h
@@ -93,6 +93,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG	_IO('T', 83) /* 0x5453 */
 #define TIOCSERGWILD	_IOR('T', 84,  int) /* 0x5454 */
diff --git a/arch/sparc/include/uapi/asm/ioctls.h b/arch/sparc/include/uapi/asm/ioctls.h
index 06b3f6c3bb9a..6d27398632ea 100644
--- a/arch/sparc/include/uapi/asm/ioctls.h
+++ b/arch/sparc/include/uapi/asm/ioctls.h
@@ -27,7 +27,7 @@
 #define TIOCGRS485	_IOR('T', 0x41, struct serial_rs485)
 #define TIOCSRS485	_IOWR('T', 0x42, struct serial_rs485)
 
-/* Note that all the ioctls that are not available in Linux have a 
+/* Note that all the ioctls that are not available in Linux have a
  * double underscore on the front to: a) avoid some programs to
  * think we support some ioctls under Linux (autoconfiguration stuff)
  */
@@ -88,6 +88,7 @@
 #define TIOCGPTN	_IOR('t', 134, unsigned int) /* Get Pty Number */
 #define TIOCSPTLCK	_IOW('t', 135, int) /* Lock/unlock PTY */
 #define TIOCSIG		_IOW('t', 136, int) /* Generate signal on Pty slave */
+#define TIOCGPTPEER	_IOR('t', 137, int) /* Safely open the slave */
 
 /* Little f */
 #define FIOCLEX		_IO('f', 1)
diff --git a/arch/xtensa/include/uapi/asm/ioctls.h b/arch/xtensa/include/uapi/asm/ioctls.h
index 518954e74e6d..98b004e24e85 100644
--- a/arch/xtensa/include/uapi/asm/ioctls.h
+++ b/arch/xtensa/include/uapi/asm/ioctls.h
@@ -105,6 +105,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define TIOCSERCONFIG	_IO('T', 83)
 #define TIOCSERGWILD	_IOR('T', 84,  int)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 2a6bd9ae3f8b..d1399aac05a1 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -24,6 +24,9 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/poll.h>
+#include <linux/mount.h>
+#include <linux/file.h>
+#include <linux/ioctl.h>
 
 #undef TTY_DEBUG_HANGUP
 #ifdef TTY_DEBUG_HANGUP
@@ -66,8 +69,13 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 #ifdef CONFIG_UNIX98_PTYS
 		if (tty->driver == ptm_driver) {
 			mutex_lock(&devpts_mutex);
-			if (tty->link->driver_data)
-				devpts_pty_kill(tty->link->driver_data);
+			if (tty->link->driver_data) {
+				struct path *path = tty->link->driver_data;
+
+				devpts_pty_kill(path->dentry);
+				path_put(path);
+				kfree(path);
+			}
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
@@ -440,6 +448,48 @@ static int pty_common_install(struct tty_driver *driver, struct tty_struct *tty,
 	return retval;
 }
 
+/**
+ *	pty_open_peer - open the peer of a pty
+ *	@tty: the peer of the pty being opened
+ *
+ *	Open the cached dentry in tty->link, providing a safe way for userspace
+ *	to get the slave end of a pty (where they have the master fd and cannot
+ *	access or trust the mount namespace /dev/pts was mounted inside).
+ */
+static struct file *pty_open_peer(struct tty_struct *tty, int flags)
+{
+	if (tty->driver->subtype != PTY_TYPE_MASTER)
+		return ERR_PTR(-EIO);
+	return dentry_open(tty->link->driver_data, flags, current_cred());
+}
+
+static int pty_get_peer(struct tty_struct *tty, int flags)
+{
+	int fd = -1;
+	struct file *filp = NULL;
+	int retval = -EINVAL;
+
+	fd = get_unused_fd_flags(0);
+	if (fd < 0) {
+		retval = fd;
+		goto err;
+	}
+
+	filp = pty_open_peer(tty, flags);
+	if (IS_ERR(filp)) {
+		retval = PTR_ERR(filp);
+		goto err_put;
+	}
+
+	fd_install(fd, filp);
+	return fd;
+
+err_put:
+	put_unused_fd(fd);
+err:
+	return retval;
+}
+
 static void pty_cleanup(struct tty_struct *tty)
 {
 	tty_port_put(tty->port);
@@ -613,6 +663,8 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
 		return pty_get_pktmode(tty, (int __user *)arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
+	case TIOCGPTPEER: /* Open the other end */
+		return pty_get_peer(tty, (int) arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
 	}
@@ -740,6 +792,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	struct pts_fs_info *fsi;
 	struct tty_struct *tty;
+	struct path *pts_path;
 	struct dentry *dentry;
 	int retval;
 	int index;
@@ -793,16 +846,26 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 		retval = PTR_ERR(dentry);
 		goto err_release;
 	}
-	tty->link->driver_data = dentry;
+	/* We need to cache a fake path for TIOCGPTPEER. */
+	pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
+	if (!pts_path)
+		goto err_release;
+	pts_path->mnt = filp->f_path.mnt;
+	pts_path->dentry = dentry;
+	path_get(pts_path);
+	tty->link->driver_data = pts_path;
 
 	retval = ptm_driver->ops->open(tty, filp);
 	if (retval)
-		goto err_release;
+		goto err_path_put;
 
 	tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
 
 	tty_unlock(tty);
 	return 0;
+err_path_put:
+	path_put(pts_path);
+	kfree(pts_path);
 err_release:
 	tty_unlock(tty);
 	// This will also put-ref the fsi
diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
index 143dacbb7d9a..06d5f7ddf84e 100644
--- a/include/uapi/asm-generic/ioctls.h
+++ b/include/uapi/asm-generic/ioctls.h
@@ -77,6 +77,7 @@
 #define TIOCGPKT	_IOR('T', 0x38, int) /* Get packet mode state */
 #define TIOCGPTLCK	_IOR('T', 0x39, int) /* Get Pty lock state */
 #define TIOCGEXCL	_IOR('T', 0x40, int) /* Get exclusive mode state */
+#define TIOCGPTPEER	_IOR('T', 0x41, int) /* Safely open the slave */
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 1/2] tty: add compat_ioctl callbacks
  2017-06-03 14:15 ` [PATCH v4 1/2] tty: add compat_ioctl callbacks Aleksa Sarai
@ 2017-06-06 12:49   ` Arnd Bergmann
  0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2017-06-06 12:49 UTC (permalink / raw)
  To: Aleksa Sarai
  Cc: Greg Kroah-Hartman, Jiri Slaby, Linux Kernel Mailing List,
	linux-alpha, open list:RALINK MIPS ARCHITECTURE, Parisc List,
	linuxppc-dev, linux-sh, sparclinux, linux-xtensa, linux-arch,
	Christian Brauner, Valentin Rothberg

On Sat, Jun 3, 2017 at 4:15 PM, Aleksa Sarai <asarai@suse.de> wrote:
> In order to avoid future diversions between fs/compat_ioctl.c and
> drivers/tty/pty.c, define .compat_ioctl callbacks for the relevant
> tty_operations structs. Since both pty_unix98_ioctl() and
> pty_bsd_ioctl() are compatible between 32-bit and 64-bit userspace no
> special translation is required.
>
> Signed-off-by: Aleksa Sarai <asarai@suse.de>

Looks good,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl
  2017-06-03 14:15 ` [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
@ 2017-06-09  9:26   ` Greg Kroah-Hartman
  2017-06-09  9:50     ` Aleksa Sarai
  0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2017-06-09  9:26 UTC (permalink / raw)
  To: Aleksa Sarai
  Cc: Jiri Slaby, Arnd Bergmann, linux-kernel, linux-alpha, linux-mips,
	linux-parisc, linuxppc-dev, linux-sh, sparclinux, linux-xtensa,
	linux-arch, Christian Brauner, Valentin Rothberg

On Sun, Jun 04, 2017 at 12:15:15AM +1000, Aleksa Sarai wrote:
> When opening the slave end of a PTY, it is not possible for userspace to
> safely ensure that /dev/pts/$num is actually a slave (in cases where the
> mount namespace in which devpts was mounted is controlled by an
> untrusted process). In addition, there are several unresolvable
> race conditions if userspace were to attempt to detect attacks through
> stat(2) and other similar methods [in addition it is not clear how
> userspace could detect attacks involving FUSE].
> 
> Resolve this by providing an interface for userpace to safely open the
> "peer" end of a PTY file descriptor by using the dentry cached by
> devpts. Since it is not possible to have an open master PTY without
> having its slave exposed in /dev/pts this interface is safe. This
> interface currently does not provide a way to get the master pty (since
> it is not clear whether such an interface is safe or even useful).
> 
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Valentin Rothberg <vrothberg@suse.com>
> Signed-off-by: Aleksa Sarai <asarai@suse.de>

Is this going to be documented anywhere?  Is there a man page update
that also goes along with this?  What userspace program wants to use
this?

I'm not objecting to this, I just want to know that people will use
this, and that they can find out information about it if they want to.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl
  2017-06-09  9:26   ` Greg Kroah-Hartman
@ 2017-06-09  9:50     ` Aleksa Sarai
  2017-06-09 10:04       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 8+ messages in thread
From: Aleksa Sarai @ 2017-06-09  9:50 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Arnd Bergmann, linux-kernel, linux-alpha, linux-mips,
	linux-parisc, linuxppc-dev, linux-sh, sparclinux, linux-xtensa,
	linux-arch, Christian Brauner, Valentin Rothberg

>> When opening the slave end of a PTY, it is not possible for userspace to
>> safely ensure that /dev/pts/$num is actually a slave (in cases where the
>> mount namespace in which devpts was mounted is controlled by an
>> untrusted process). In addition, there are several unresolvable
>> race conditions if userspace were to attempt to detect attacks through
>> stat(2) and other similar methods [in addition it is not clear how
>> userspace could detect attacks involving FUSE].
>>
>> Resolve this by providing an interface for userpace to safely open the
>> "peer" end of a PTY file descriptor by using the dentry cached by
>> devpts. Since it is not possible to have an open master PTY without
>> having its slave exposed in /dev/pts this interface is safe. This
>> interface currently does not provide a way to get the master pty (since
>> it is not clear whether such an interface is safe or even useful).
>>
>> Cc: Christian Brauner <christian.brauner@ubuntu.com>
>> Cc: Valentin Rothberg <vrothberg@suse.com>
>> Signed-off-by: Aleksa Sarai <asarai@suse.de>
> 
> Is this going to be documented anywhere?  Is there a man page update
> that also goes along with this?

I will add one, I didn't know where the man-pages project is hosted / 
where patches get pushed? What is the ML?

> What userspace program wants to use this?

LXC (Christian is on Cc) will use this, runC will most likely use it, 
pending on some design discussions (as well as some future container 
runtimes I'm planning on working on). Effectively any container runtime 
that wants to safely create terminals and spawn containers inside an 
existing container's namespaces will likely want to use this.

[ As an aside, I /would/ argue this is a security fix (it fixes an 
interface problem that made doing certain operations securely possible) 
but I didn't want to Cc stable@ because it's a feature and not a strict 
bugfix. ]

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl
  2017-06-09  9:50     ` Aleksa Sarai
@ 2017-06-09 10:04       ` Greg Kroah-Hartman
  2017-06-09 10:24         ` Aleksa Sarai
  0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2017-06-09 10:04 UTC (permalink / raw)
  To: Aleksa Sarai
  Cc: Jiri Slaby, Arnd Bergmann, linux-kernel, linux-alpha, linux-mips,
	linux-parisc, linuxppc-dev, linux-sh, sparclinux, linux-xtensa,
	linux-arch, Christian Brauner, Valentin Rothberg

On Fri, Jun 09, 2017 at 07:50:43PM +1000, Aleksa Sarai wrote:
> > > When opening the slave end of a PTY, it is not possible for userspace to
> > > safely ensure that /dev/pts/$num is actually a slave (in cases where the
> > > mount namespace in which devpts was mounted is controlled by an
> > > untrusted process). In addition, there are several unresolvable
> > > race conditions if userspace were to attempt to detect attacks through
> > > stat(2) and other similar methods [in addition it is not clear how
> > > userspace could detect attacks involving FUSE].
> > > 
> > > Resolve this by providing an interface for userpace to safely open the
> > > "peer" end of a PTY file descriptor by using the dentry cached by
> > > devpts. Since it is not possible to have an open master PTY without
> > > having its slave exposed in /dev/pts this interface is safe. This
> > > interface currently does not provide a way to get the master pty (since
> > > it is not clear whether such an interface is safe or even useful).
> > > 
> > > Cc: Christian Brauner <christian.brauner@ubuntu.com>
> > > Cc: Valentin Rothberg <vrothberg@suse.com>
> > > Signed-off-by: Aleksa Sarai <asarai@suse.de>
> > 
> > Is this going to be documented anywhere?  Is there a man page update
> > that also goes along with this?
> 
> I will add one, I didn't know where the man-pages project is hosted / where
> patches get pushed? What is the ML?

>From the MAINTAINERS file:
  MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
  M:      Michael Kerrisk <mtk.manpages@gmail.com>
  W:      http://www.kernel.org/doc/man-pages
  L:      linux-man@vger.kernel.org
  S:      Maintained

> > What userspace program wants to use this?
> 
> LXC (Christian is on Cc) will use this, runC will most likely use it,
> pending on some design discussions (as well as some future container
> runtimes I'm planning on working on). Effectively any container runtime that
> wants to safely create terminals and spawn containers inside an existing
> container's namespaces will likely want to use this.
> 
> [ As an aside, I /would/ argue this is a security fix (it fixes an interface
> problem that made doing certain operations securely possible) but I didn't
> want to Cc stable@ because it's a feature and not a strict bugfix. ]

Yeah, it's a new feature, so stable doesn't really fit here.  And as
people who use containers are all keeping up to date with their kernel
versions, this shouldn't be that big of a deal, not like the Android
kernel mess :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl
  2017-06-09 10:04       ` Greg Kroah-Hartman
@ 2017-06-09 10:24         ` Aleksa Sarai
  0 siblings, 0 replies; 8+ messages in thread
From: Aleksa Sarai @ 2017-06-09 10:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Arnd Bergmann, linux-kernel, linux-alpha, linux-mips,
	linux-parisc, linuxppc-dev, linux-sh, sparclinux, linux-xtensa,
	linux-arch, Christian Brauner, Valentin Rothberg

>>>> When opening the slave end of a PTY, it is not possible for userspace to
>>>> safely ensure that /dev/pts/$num is actually a slave (in cases where the
>>>> mount namespace in which devpts was mounted is controlled by an
>>>> untrusted process). In addition, there are several unresolvable
>>>> race conditions if userspace were to attempt to detect attacks through
>>>> stat(2) and other similar methods [in addition it is not clear how
>>>> userspace could detect attacks involving FUSE].
>>>>
>>>> Resolve this by providing an interface for userpace to safely open the
>>>> "peer" end of a PTY file descriptor by using the dentry cached by
>>>> devpts. Since it is not possible to have an open master PTY without
>>>> having its slave exposed in /dev/pts this interface is safe. This
>>>> interface currently does not provide a way to get the master pty (since
>>>> it is not clear whether such an interface is safe or even useful).
>>>>
>>>> Cc: Christian Brauner <christian.brauner@ubuntu.com>
>>>> Cc: Valentin Rothberg <vrothberg@suse.com>
>>>> Signed-off-by: Aleksa Sarai <asarai@suse.de>
>>>
>>> Is this going to be documented anywhere?  Is there a man page update
>>> that also goes along with this?
>>
>> I will add one, I didn't know where the man-pages project is hosted / where
>> patches get pushed? What is the ML?
> 
>  From the MAINTAINERS file:
>    MAN-PAGES: MANUAL PAGES FOR LINUX -- Sections 2, 3, 4, 5, and 7
>    M:      Michael Kerrisk <mtk.manpages@gmail.com>
>    W:      http://www.kernel.org/doc/man-pages
>    L:      linux-man@vger.kernel.org
>    S:      Maintained

Ah, should've looked there first!

Thanks Greg, I'll send it over the weekend.

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-06-09 10:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-03 14:15 [PATCH v4 0/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
2017-06-03 14:15 ` [PATCH v4 1/2] tty: add compat_ioctl callbacks Aleksa Sarai
2017-06-06 12:49   ` Arnd Bergmann
2017-06-03 14:15 ` [PATCH v4 2/2] tty: add TIOCGPTPEER ioctl Aleksa Sarai
2017-06-09  9:26   ` Greg Kroah-Hartman
2017-06-09  9:50     ` Aleksa Sarai
2017-06-09 10:04       ` Greg Kroah-Hartman
2017-06-09 10:24         ` Aleksa Sarai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).