* [PATCH] devpts_get_tty() should validate inode
@ 2009-11-18 2:35 Sukadev Bhattiprolu
2009-11-18 3:44 ` Greg KH
[not found] ` <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
0 siblings, 2 replies; 6+ messages in thread
From: Sukadev Bhattiprolu @ 2009-11-18 2:35 UTC (permalink / raw)
To: gregkh; +Cc: Alan Cox, hpa, root, sukadev, Containers, linux-kernel
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@us.ibm.com>
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@localdomain.pl>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] devpts_get_tty() should validate inode
2009-11-18 2:35 [PATCH] devpts_get_tty() should validate inode Sukadev Bhattiprolu
@ 2009-11-18 3:44 ` Greg KH
[not found] ` <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2009-11-18 3:44 UTC (permalink / raw)
To: Sukadev Bhattiprolu; +Cc: Alan Cox, hpa, root, Containers, linux-kernel
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@us.ibm.com>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread[parent not found: <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH] devpts_get_tty() should validate inode
[not found] ` <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
@ 2009-11-18 3:44 ` Greg KH
2009-11-18 4:03 ` Serge E. Hallyn
2009-11-19 23:59 ` patch devpts_get_tty-should-validate-inode.patch added to gregkh-2.6 tree gregkh-l3A5Bk7waGM
2 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2009-11-18 3:44 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Containers, Alan Cox,
hpa-YMNOUZJC4hwAvxtiuMwx3w
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] devpts_get_tty() should validate inode
2009-11-18 2:35 [PATCH] devpts_get_tty() should validate inode Sukadev Bhattiprolu
@ 2009-11-18 4:03 ` Serge E. Hallyn
[not found] ` <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
1 sibling, 0 replies; 6+ messages in thread
From: Serge E. Hallyn @ 2009-11-18 4:03 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: Containers, gregkh-l3A5Bk7waGM,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Alan Cox,
hpa-YMNOUZJC4hwAvxtiuMwx3w
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] devpts_get_tty() should validate inode
@ 2009-11-18 4:03 ` Serge E. Hallyn
0 siblings, 0 replies; 6+ messages in thread
From: Serge E. Hallyn @ 2009-11-18 4:03 UTC (permalink / raw)
To: Sukadev Bhattiprolu; +Cc: gregkh, Containers, linux-kernel, hpa, Alan Cox
Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
>
> 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@us.ibm.com>
> 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@localdomain.pl>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
fwiw,
Tested-by: Serge Hallyn <serue@us.ibm.com>
> ---
> 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@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers
^ permalink raw reply [flat|nested] 6+ messages in thread
* patch devpts_get_tty-should-validate-inode.patch added to gregkh-2.6 tree
[not found] ` <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2009-11-18 3:44 ` Greg KH
2009-11-18 4:03 ` Serge E. Hallyn
@ 2009-11-19 23:59 ` gregkh-l3A5Bk7waGM
2 siblings, 0 replies; 6+ messages in thread
From: gregkh-l3A5Bk7waGM @ 2009-11-19 23:59 UTC (permalink / raw)
To: sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
alan-qBU/x9rampVanCEyBjwyrvXRex20P6io,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
gregkh-l3A5Bk7waGM, root-AfQBxy1nhrQ00sYp1HPQUA,
serue-r/Jw6+rmf7HQT0dZR+AlfA, stable-DgEjT+Ai2ygdnm+yROfE0A
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-11-19 23:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-11-18 2:35 [PATCH] devpts_get_tty() should validate inode Sukadev Bhattiprolu
2009-11-18 3:44 ` Greg KH
[not found] ` <20091118023541.GA14430-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
2009-11-18 3:44 ` Greg KH
2009-11-18 4:03 ` Serge E. Hallyn
2009-11-18 4:03 ` Serge E. Hallyn
2009-11-19 23:59 ` patch devpts_get_tty-should-validate-inode.patch added to gregkh-2.6 tree gregkh-l3A5Bk7waGM
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.