Re: [PATCH] devpts_get_tty() should validate inode

Re: [PATCH] devpts_get_tty() should validate inode

[Greg KH]

On Tue, Nov 17, 2009 at 06:35:43PM -0800, Sukadev Bhattiprolu wrote:
> 
> devpts_get_tty() assumes that the inode passed in is associated with a valid
> pty.  But if the only reference to the pty is via a bind-mount, the inode
> passed to devpts_get_tty() while valid, would refer to a pty that no longer
> exists.
> 
> With a lot of debug effort, Grzegorz Nosek developed a small program (see
> below) to reproduce a crash on recent kernels. This crash is a regression
> introduced by the commit:
> 
> 	commit 527b3e4773628b30d03323a2cb5fb0d84441990f
> 	Author: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 	Date:   Mon Oct 13 10:43:08 2008 +0100
> 
> To fix, ensure that the dentry associated with the inode has not yet been
> deleted/unhashed by devpts_pty_kill().
> 
> See also:
> https://lists.linux-foundation.org/pipermail/containers/2009-July/019273.html 

Good debugging, I'll queue this up.

greg k-h

Re: [PATCH] devpts_get_tty() should validate inode

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> 
> devpts_get_tty() assumes that the inode passed in is associated with a valid
> pty.  But if the only reference to the pty is via a bind-mount, the inode
> passed to devpts_get_tty() while valid, would refer to a pty that no longer
> exists.
> 
> With a lot of debug effort, Grzegorz Nosek developed a small program (see
> below) to reproduce a crash on recent kernels. This crash is a regression
> introduced by the commit:
> 
> 	commit 527b3e4773628b30d03323a2cb5fb0d84441990f
> 	Author: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 	Date:   Mon Oct 13 10:43:08 2008 +0100
> 
> To fix, ensure that the dentry associated with the inode has not yet been
> deleted/unhashed by devpts_pty_kill().
> 
> See also:
> https://lists.linux-foundation.org/pipermail/containers/2009-July/019273.html 
> 
> tty-bug.c:
> 
> #define _GNU_SOURCE
> #include <fcntl.h>
> #include <sched.h>
> #include <stdlib.h>
> #include <sys/mount.h>
> #include <sys/signal.h>
> #include <unistd.h>
> #include <stdio.h>
> 
> #include <linux/fs.h>
> 
> void dummy(int sig)
> {
> }
> 
> static int child(void *unused)
> {
> 	int fd;
> 
> 	signal(SIGINT, dummy); signal(SIGHUP, dummy);
> 	pause(); /* cheesy synchronisation to wait for /dev/pts/0 to appear */
> 
> 	mount("/dev/pts/0", "/dev/console", NULL, MS_BIND, NULL);
> 	sleep(2);
> 
> 	fd = open("/dev/console", O_RDWR);
> 	dup(0); dup(0);
> 	write(1, "Hello world!\n", sizeof("Hello world!\n")-1);
> 	return 0;
> }
> 
> int main(void)
> {
> 	pid_t pid;
> 	char *stack;
> 
> 	stack = malloc(16384);
> 	pid = clone(child, stack+16384, CLONE_NEWNS|SIGCHLD, NULL);
> 
> 	open("/dev/ptmx", O_RDWR|O_NOCTTY|O_NONBLOCK);
> 
> 	unlockpt(fd); grantpt(fd);
> 
> 	sleep(2);
> 	kill(pid, SIGHUP);
> 	sleep(1);
> 	return 0; /* exit before child opens /dev/console */
> }
> 
> Reported-by: Grzegorz Nosek <root-AfQBxy1nhrQ00sYp1HPQUA@public.gmane.org>
> Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

fwiw,
Tested-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
> Index: linux-2.6/fs/devpts/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/devpts/inode.c	2009-10-20 21:11:03.000000000 -0700
> +++ linux-2.6/fs/devpts/inode.c	2009-11-17 16:07:16.000000000 -0800
> @@ -517,11 +517,23 @@
> 
>  struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number)
>  {
> +	struct dentry *dentry;
> +	struct tty_struct *tty;
> +
>  	BUG_ON(pts_inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR));
> 
> +	/* Ensure dentry has not been deleted by devpts_pty_kill() */
> +	dentry = d_find_alias(pts_inode);
> +	if (!dentry)
> +		return NULL;
> +
> +	tty = NULL;
>  	if (pts_inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> -		return (struct tty_struct *)pts_inode->i_private;
> -	return NULL;
> +		tty = (struct tty_struct *)pts_inode->i_private;
> +
> +	dput(dentry);
> +
> +	return tty;
>  }
> 
>  void devpts_pty_kill(struct tty_struct *tty)
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

patch devpts_get_tty-should-validate-inode.patch added to gregkh-2.6 tree

[gregkh-l3A5Bk7waGM]

This is a note to let you know that I've just added the patch titled

    Subject: devpts_get_tty() should validate inode

to my gregkh-2.6 tree.  Its filename is

    devpts_get_tty-should-validate-inode.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


From sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org  Thu Nov 19 15:40:33 2009
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Tue, 17 Nov 2009 18:35:43 -0800
Subject: devpts_get_tty() should validate inode
To: gregkh-l3A5Bk7waGM@public.gmane.org
Cc: Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>, hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org, root-AfQBxy1nhrQ00sYp1HPQUA@public.gmane.org, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org, Containers <containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>
Message-ID: <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>


devpts_get_tty() assumes that the inode passed in is associated with a valid
pty.  But if the only reference to the pty is via a bind-mount, the inode
passed to devpts_get_tty() while valid, would refer to a pty that no longer
exists.

With a lot of debug effort, Grzegorz Nosek developed a small program (see
below) to reproduce a crash on recent kernels. This crash is a regression
introduced by the commit:

	commit 527b3e4773628b30d03323a2cb5fb0d84441990f
	Author: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
	Date:   Mon Oct 13 10:43:08 2008 +0100

To fix, ensure that the dentry associated with the inode has not yet been
deleted/unhashed by devpts_pty_kill().

See also:
https://lists.linux-foundation.org/pipermail/containers/2009-July/019273.html 

tty-bug.c:

#define _GNU_SOURCE
#include <fcntl.h>
#include <sched.h>
#include <stdlib.h>
#include <sys/mount.h>
#include <sys/signal.h>
#include <unistd.h>
#include <stdio.h>

#include <linux/fs.h>

void dummy(int sig)
{
}

static int child(void *unused)
{
	int fd;

	signal(SIGINT, dummy); signal(SIGHUP, dummy);
	pause(); /* cheesy synchronisation to wait for /dev/pts/0 to appear */

	mount("/dev/pts/0", "/dev/console", NULL, MS_BIND, NULL);
	sleep(2);

	fd = open("/dev/console", O_RDWR);
	dup(0); dup(0);
	write(1, "Hello world!\n", sizeof("Hello world!\n")-1);
	return 0;
}

int main(void)
{
	pid_t pid;
	char *stack;

	stack = malloc(16384);
	pid = clone(child, stack+16384, CLONE_NEWNS|SIGCHLD, NULL);

	open("/dev/ptmx", O_RDWR|O_NOCTTY|O_NONBLOCK);

	unlockpt(fd); grantpt(fd);

	sleep(2);
	kill(pid, SIGHUP);
	sleep(1);
	return 0; /* exit before child opens /dev/console */
}

Reported-by: Grzegorz Nosek <root-AfQBxy1nhrQ00sYp1HPQUA@public.gmane.org>
Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Tested-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: stable <stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>

---
 fs/devpts/inode.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -517,11 +517,23 @@ int devpts_pty_new(struct inode *ptmx_in
 
 struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number)
 {
+	struct dentry *dentry;
+	struct tty_struct *tty;
+
 	BUG_ON(pts_inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR));
 
+	/* Ensure dentry has not been deleted by devpts_pty_kill() */
+	dentry = d_find_alias(pts_inode);
+	if (!dentry)
+		return NULL;
+
+	tty = NULL;
 	if (pts_inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
-		return (struct tty_struct *)pts_inode->i_private;
-	return NULL;
+		tty = (struct tty_struct *)pts_inode->i_private;
+
+	dput(dentry);
+
+	return tty;
 }
 
 void devpts_pty_kill(struct tty_struct *tty)