LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tty: cleanup release_mem
@ 2007-01-31  7:16 Pekka J Enberg
  2007-01-31 14:20 ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Pekka J Enberg @ 2007-01-31  7:16 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel, hch, alan

From: Christoph Hellwig <hch@lst.de>

release_mem contains two copies of exactly the same code.  Refactor
these into a new helper, release_tty.  The only change in behaviour
is that the driver reference count is now decremented after the
master tty has been freed instead of before.

[penberg@cs.helsinki.fi: fix use-after-free in release_tty.]
Cc: Alan Cox <alan@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 47a6eac..3bfe6fe 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -154,7 +154,7 @@ static int tty_release(struct inode *, s
 int tty_ioctl(struct inode * inode, struct file * file,
 	      unsigned int cmd, unsigned long arg);
 static int tty_fasync(int fd, struct file * filp, int on);
-static void release_mem(struct tty_struct *tty, int idx);
+static void release_tty(struct tty_struct *tty, int idx);
 
 /**
  *	alloc_tty_struct	-	allocate a tty object
@@ -2003,7 +2003,7 @@ static int init_dev(struct tty_driver *d
 
 	/* 
 	 * All structures have been allocated, so now we install them.
-	 * Failures after this point use release_mem to clean up, so 
+	 * Failures after this point use release_tty to clean up, so 
 	 * there's no need to null out the local pointers.
 	 */
 	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM)) {
@@ -2024,8 +2024,8 @@ static int init_dev(struct tty_driver *d
 
 	/* 
 	 * Structures all installed ... call the ldisc open routines.
-	 * If we fail here just call release_mem to clean up.  No need
-	 * to decrement the use counts, as release_mem doesn't care.
+	 * If we fail here just call release_tty to clean up.  No need
+	 * to decrement the use counts, as release_tty doesn't care.
 	 */
 
 	if (tty->ldisc.open) {
@@ -2095,17 +2095,17 @@ fail_no_mem:
 	retval = -ENOMEM;
 	goto end_init;
 
-	/* call the tty release_mem routine to clean out this slot */
+	/* call the tty release_tty routine to clean out this slot */
 release_mem_out:
 	if (printk_ratelimit())
 		printk(KERN_INFO "init_dev: ldisc open failed, "
 				 "clearing slot %d\n", idx);
-	release_mem(tty, idx);
+	release_tty(tty, idx);
 	goto end_init;
 }
 
 /**
- *	release_mem		-	release tty structure memory
+ *	release_one_tty		-	release tty structure memory
  *
  *	Releases memory associated with a tty structure, and clears out the
  *	driver table slots. This function is called when a device is no longer
@@ -2117,37 +2117,14 @@ release_mem_out:
  *	of ttys that the driver keeps.
  *		FIXME: should we require tty_mutex is held here ??
  */
-
-static void release_mem(struct tty_struct *tty, int idx)
+static void release_one_tty(struct tty_struct *tty, int idx)
 {
-	struct tty_struct *o_tty;
-	struct ktermios *tp;
 	int devpts = tty->driver->flags & TTY_DRIVER_DEVPTS_MEM;
-
-	if ((o_tty = tty->link) != NULL) {
-		if (!devpts)
-			o_tty->driver->ttys[idx] = NULL;
-		if (o_tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
-			tp = o_tty->termios;
-			if (!devpts)
-				o_tty->driver->termios[idx] = NULL;
-			kfree(tp);
-
-			tp = o_tty->termios_locked;
-			if (!devpts)
-				o_tty->driver->termios_locked[idx] = NULL;
-			kfree(tp);
-		}
-		o_tty->magic = 0;
-		o_tty->driver->refcount--;
-		file_list_lock();
-		list_del_init(&o_tty->tty_files);
-		file_list_unlock();
-		free_tty_struct(o_tty);
-	}
+	struct ktermios *tp;
 
 	if (!devpts)
 		tty->driver->ttys[idx] = NULL;
+
 	if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
 		tp = tty->termios;
 		if (!devpts)
@@ -2160,15 +2137,39 @@ static void release_mem(struct tty_struc
 		kfree(tp);
 	}
 
+
 	tty->magic = 0;
 	tty->driver->refcount--;
+
 	file_list_lock();
 	list_del_init(&tty->tty_files);
 	file_list_unlock();
-	module_put(tty->driver->owner);
+
 	free_tty_struct(tty);
 }
 
+/**
+ *	release_tty		-	release tty structure memory
+ *
+ *	Release both @tty and a possible linked partner (think pty pair),
+ *	and decrement the refcount of the backing module.
+ *
+ *	Locking:
+ *		tty_mutex - sometimes only
+ *		takes the file list lock internally when working on the list
+ *	of ttys that the driver keeps.
+ *		FIXME: should we require tty_mutex is held here ??
+ */
+static void release_tty(struct tty_struct *tty, int idx)
+{
+	struct tty_driver *driver = tty->driver;
+
+	if (tty->link)
+		release_one_tty(tty->link, idx);
+	release_one_tty(tty, idx);
+	module_put(driver->owner);
+}
+
 /*
  * Even releasing the tty structures is a tricky business.. We have
  * to be very careful that the structures are all released at the
@@ -2436,10 +2437,10 @@ #endif
 		tty_set_termios_ldisc(o_tty,N_TTY); 
 	}
 	/*
-	 * The release_mem function takes care of the details of clearing
+	 * The release_tty function takes care of the details of clearing
 	 * the slots and preserving the termios structure.
 	 */
-	release_mem(tty, idx);
+	release_tty(tty, idx);
 
 #ifdef CONFIG_UNIX98_PTYS
 	/* Make this pty number available for reallocation */

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

* Re: [PATCH] tty: cleanup release_mem
  2007-01-31  7:16 [PATCH] tty: cleanup release_mem Pekka J Enberg
@ 2007-01-31 14:20 ` Christoph Hellwig
  0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2007-01-31 14:20 UTC (permalink / raw)
  To: Pekka J Enberg; +Cc: akpm, linux-kernel, hch, alan

Looks good to me.  I'd have only kept a pointer to the struct module,
but that shouldn't matter for the actual code generated.

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

* Re: [PATCH] tty: cleanup release_mem
  2007-01-30  3:56       ` Andrew Morton
@ 2007-01-30  6:33         ` Pekka Enberg
  0 siblings, 0 replies; 8+ messages in thread
From: Pekka Enberg @ 2007-01-30  6:33 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Alan Cox, Christoph Hellwig, linux-kernel

On 1/30/07, Andrew Morton <akpm@osdl.org> wrote:
> BUG: unable to handle kernel paging request at virtual address 6b6b6bdf

Use after free. The new code does module_put() _after_
free_tty_struct() which is obviously wrong.

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

* Re: [PATCH] tty: cleanup release_mem
  2007-01-29 19:06     ` Alan Cox
@ 2007-01-30  3:56       ` Andrew Morton
  2007-01-30  6:33         ` Pekka Enberg
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2007-01-30  3:56 UTC (permalink / raw)
  To: Alan Cox; +Cc: Christoph Hellwig, linux-kernel

On Mon, 29 Jan 2007 14:06:04 -0500
Alan Cox <alan@redhat.com> wrote:

> On Mon, Jan 29, 2007 at 07:12:35PM +0100, Christoph Hellwig wrote:
> > Okay.  Now that we get into the details I've also added some renaming,
> > release_mem becomes release_tty and the new factored out function is
> > release_one_tty.  The difference is documented in the kdoc comments.
> > 
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Alan Cox <alan@redhat.com>

VFS: Mounted root (ext3 filesystem) readonly.
Freeing unused kernel memory: 300k freed
Write protecting the kernel read-only data: 920k
BUG: unable to handle kernel paging request at virtual address 6b6b6bdf
 printing eip:
c02a91ec
*pde = 00000000
Oops: 0000 [#1]
SMP 
last sysfs file: /block/hdc/range
Modules linked in:
CPU:    0
EIP:    0060:[<c02a91ec>]    Not tainted VLI
EFLAGS: 00010286   (2.6.20-rc6-mm3 #1)
EIP is at release_tty+0x2c/0x40
eax: 6b6b6b6b   ebx: f71c860c   ecx: c0175e24   edx: 00000000
esi: 00000000   edi: 00000001   ebp: c222fe88   esp: c222fe80
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process init (pid: 1, ti=c222e000 task=c222cae0 task.ti=c222e000)
Stack: f71c860c 00000000 c222ff3c c02aac29 00000000 c22e453c c222fea0 00000006 
       c222fec4 002ad15c 00000000 00000001 00000000 00000000 00000000 00000001 
       c06650a0 000001d8 00000000 c222ff24 c013d53f c0178d34 f7c383c4 c222ff20 
Call Trace:
 [<c0103f7a>] show_trace_log_lvl+0x1a/0x30
 [<c0104038>] show_stack_log_lvl+0xa8/0xe0
 [<c0104259>] show_registers+0x1e9/0x2f0
 [<c010447a>] die+0x11a/0x250
 [<c0115580>] do_page_fault+0x2f0/0x5f0
 [<c03d74b4>] error_code+0x7c/0x84
 [<c02aac29>] release_dev+0x519/0x750
 [<c02aae72>] tty_release+0x12/0x20
 [<c017b8e4>] __fput+0xb4/0x180
 [<c017bb02>] fput+0x22/0x40
 [<c0178db7>] filp_close+0x47/0x70
 [<c0179f4d>] sys_close+0x6d/0xc0
 [<c0102edc>] sysenter_past_esp+0x5d/0x99
 =======================
Code: 89 e5 83 ec 08 89 1c 24 89 c3 89 74 24 04 89 d6 8b 80 fc 00 00 00 85 c0 74 05 e8 00 ff ff ff 89 f2 89 d8 e8 f7 fe ff ff 8b 43 04 <8b> 40 74 e8 8c b3 e9 ff 8b 1c 24 8b 74 24 04 89 ec 5d c3 90 55 
EIP: [<c02a91ec>] release_tty+0x2c/0x40 SS:ESP 0068:c222fe80
Kernel panic - not syncing: Attempted to kill init!

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

* Re: [PATCH] tty: cleanup release_mem
  2007-01-29 18:12   ` Christoph Hellwig
@ 2007-01-29 19:06     ` Alan Cox
  2007-01-30  3:56       ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Alan Cox @ 2007-01-29 19:06 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Alan Cox, linux-kernel

On Mon, Jan 29, 2007 at 07:12:35PM +0100, Christoph Hellwig wrote:
> Okay.  Now that we get into the details I've also added some renaming,
> release_mem becomes release_tty and the new factored out function is
> release_one_tty.  The difference is documented in the kdoc comments.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Alan Cox <alan@redhat.com>



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

* Re: [PATCH] tty: cleanup release_mem
  2007-01-29 12:01 ` Alan Cox
@ 2007-01-29 18:12   ` Christoph Hellwig
  2007-01-29 19:06     ` Alan Cox
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2007-01-29 18:12 UTC (permalink / raw)
  To: Alan Cox; +Cc: Christoph Hellwig, linux-kernel

On Mon, Jan 29, 2007 at 07:01:00AM -0500, Alan Cox wrote:
> On Sun, Jan 28, 2007 at 06:12:44PM +0100, Christoph Hellwig wrote:
> > release_mem contains two copies of exactly the same code.  Refactor
> > these into a new helper, release_tty.  The only change in behaviour
> > is that the driver reference count is now decremented after the
> > master tty has been freed instead of before.
> > 
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Acked-by: Alan Cox <alan@redhat.com>
> 
> Please propogate the mutex_ FIXME comment to both functions as it may
> well need to cover tty->link

Okay.  Now that we get into the details I've also added some renaming,
release_mem becomes release_tty and the new factored out function is
release_one_tty.  The difference is documented in the kdoc comments.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/char/tty_io.c
===================================================================
--- linux-2.6.orig/drivers/char/tty_io.c	2007-01-29 10:01:46.000000000 +0100
+++ linux-2.6/drivers/char/tty_io.c	2007-01-29 19:08:06.000000000 +0100
@@ -154,7 +154,7 @@
 int tty_ioctl(struct inode * inode, struct file * file,
 	      unsigned int cmd, unsigned long arg);
 static int tty_fasync(int fd, struct file * filp, int on);
-static void release_mem(struct tty_struct *tty, int idx);
+static void release_tty(struct tty_struct *tty, int idx);
 
 /**
  *	alloc_tty_struct	-	allocate a tty object
@@ -2003,7 +2003,7 @@
 
 	/* 
 	 * All structures have been allocated, so now we install them.
-	 * Failures after this point use release_mem to clean up, so 
+	 * Failures after this point use release_tty to clean up, so 
 	 * there's no need to null out the local pointers.
 	 */
 	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM)) {
@@ -2024,8 +2024,8 @@
 
 	/* 
 	 * Structures all installed ... call the ldisc open routines.
-	 * If we fail here just call release_mem to clean up.  No need
-	 * to decrement the use counts, as release_mem doesn't care.
+	 * If we fail here just call release_tty to clean up.  No need
+	 * to decrement the use counts, as release_tty doesn't care.
 	 */
 
 	if (tty->ldisc.open) {
@@ -2095,17 +2095,17 @@
 	retval = -ENOMEM;
 	goto end_init;
 
-	/* call the tty release_mem routine to clean out this slot */
+	/* call the tty release_tty routine to clean out this slot */
 release_mem_out:
 	if (printk_ratelimit())
 		printk(KERN_INFO "init_dev: ldisc open failed, "
 				 "clearing slot %d\n", idx);
-	release_mem(tty, idx);
+	release_tty(tty, idx);
 	goto end_init;
 }
 
 /**
- *	release_mem		-	release tty structure memory
+ *	release_one_tty		-	release tty structure memory
  *
  *	Releases memory associated with a tty structure, and clears out the
  *	driver table slots. This function is called when a device is no longer
@@ -2117,37 +2117,14 @@
  *	of ttys that the driver keeps.
  *		FIXME: should we require tty_mutex is held here ??
  */
-
-static void release_mem(struct tty_struct *tty, int idx)
+static void release_one_tty(struct tty_struct *tty, int idx)
 {
-	struct tty_struct *o_tty;
-	struct ktermios *tp;
 	int devpts = tty->driver->flags & TTY_DRIVER_DEVPTS_MEM;
-
-	if ((o_tty = tty->link) != NULL) {
-		if (!devpts)
-			o_tty->driver->ttys[idx] = NULL;
-		if (o_tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
-			tp = o_tty->termios;
-			if (!devpts)
-				o_tty->driver->termios[idx] = NULL;
-			kfree(tp);
-
-			tp = o_tty->termios_locked;
-			if (!devpts)
-				o_tty->driver->termios_locked[idx] = NULL;
-			kfree(tp);
-		}
-		o_tty->magic = 0;
-		o_tty->driver->refcount--;
-		file_list_lock();
-		list_del_init(&o_tty->tty_files);
-		file_list_unlock();
-		free_tty_struct(o_tty);
-	}
+	struct ktermios *tp;
 
 	if (!devpts)
 		tty->driver->ttys[idx] = NULL;
+
 	if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
 		tp = tty->termios;
 		if (!devpts)
@@ -2160,15 +2137,37 @@
 		kfree(tp);
 	}
 
+
 	tty->magic = 0;
 	tty->driver->refcount--;
+
 	file_list_lock();
 	list_del_init(&tty->tty_files);
 	file_list_unlock();
-	module_put(tty->driver->owner);
+
 	free_tty_struct(tty);
 }
 
+/**
+ *	release_one_tty		-	release tty structure memory
+ *
+ *	Release both @tty and a possible linked partner (think pty pair),
+ *	and decrement the refcount of the backing module.
+ *
+ *	Locking:
+ *		tty_mutex - sometimes only
+ *		takes the file list lock internally when working on the list
+ *	of ttys that the driver keeps.
+ *		FIXME: should we require tty_mutex is held here ??
+ */
+static void release_tty(struct tty_struct *tty, int idx)
+{
+	if (tty->link)
+		release_one_tty(tty->link, idx);
+	release_one_tty(tty, idx);
+	module_put(tty->driver->owner);
+}
+
 /*
  * Even releasing the tty structures is a tricky business.. We have
  * to be very careful that the structures are all released at the
@@ -2436,10 +2435,10 @@
 		tty_set_termios_ldisc(o_tty,N_TTY); 
 	}
 	/*
-	 * The release_mem function takes care of the details of clearing
+	 * The release_tty function takes care of the details of clearing
 	 * the slots and preserving the termios structure.
 	 */
-	release_mem(tty, idx);
+	release_tty(tty, idx);
 
 #ifdef CONFIG_UNIX98_PTYS
 	/* Make this pty number available for reallocation */

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

* Re: [PATCH] tty: cleanup release_mem
  2007-01-28 17:12 Christoph Hellwig
@ 2007-01-29 12:01 ` Alan Cox
  2007-01-29 18:12   ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Alan Cox @ 2007-01-29 12:01 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: alan, linux-kernel

On Sun, Jan 28, 2007 at 06:12:44PM +0100, Christoph Hellwig wrote:
> release_mem contains two copies of exactly the same code.  Refactor
> these into a new helper, release_tty.  The only change in behaviour
> is that the driver reference count is now decremented after the
> master tty has been freed instead of before.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Alan Cox <alan@redhat.com>

Please propogate the mutex_ FIXME comment to both functions as it may
well need to cover tty->link


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

* [PATCH] tty: cleanup release_mem
@ 2007-01-28 17:12 Christoph Hellwig
  2007-01-29 12:01 ` Alan Cox
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2007-01-28 17:12 UTC (permalink / raw)
  To: alan; +Cc: linux-kernel

release_mem contains two copies of exactly the same code.  Refactor
these into a new helper, release_tty.  The only change in behaviour
is that the driver reference count is now decremented after the
master tty has been freed instead of before.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/char/tty_io.c
===================================================================
--- linux-2.6.orig/drivers/char/tty_io.c	2007-01-28 16:44:08.000000000 +0100
+++ linux-2.6/drivers/char/tty_io.c	2007-01-28 17:01:37.000000000 +0100
@@ -2105,7 +2105,7 @@
 }
 
 /**
- *	release_mem		-	release tty structure memory
+ *	release_tty		-	release tty structure memory
  *
  *	Releases memory associated with a tty structure, and clears out the
  *	driver table slots. This function is called when a device is no longer
@@ -2117,37 +2117,14 @@
  *	of ttys that the driver keeps.
  *		FIXME: should we require tty_mutex is held here ??
  */
-
-static void release_mem(struct tty_struct *tty, int idx)
+static void release_tty(struct tty_struct *tty, int idx)
 {
-	struct tty_struct *o_tty;
-	struct ktermios *tp;
 	int devpts = tty->driver->flags & TTY_DRIVER_DEVPTS_MEM;
-
-	if ((o_tty = tty->link) != NULL) {
-		if (!devpts)
-			o_tty->driver->ttys[idx] = NULL;
-		if (o_tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
-			tp = o_tty->termios;
-			if (!devpts)
-				o_tty->driver->termios[idx] = NULL;
-			kfree(tp);
-
-			tp = o_tty->termios_locked;
-			if (!devpts)
-				o_tty->driver->termios_locked[idx] = NULL;
-			kfree(tp);
-		}
-		o_tty->magic = 0;
-		o_tty->driver->refcount--;
-		file_list_lock();
-		list_del_init(&o_tty->tty_files);
-		file_list_unlock();
-		free_tty_struct(o_tty);
-	}
+	struct ktermios *tp;
 
 	if (!devpts)
 		tty->driver->ttys[idx] = NULL;
+
 	if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS) {
 		tp = tty->termios;
 		if (!devpts)
@@ -2160,15 +2137,25 @@
 		kfree(tp);
 	}
 
+
 	tty->magic = 0;
 	tty->driver->refcount--;
+
 	file_list_lock();
 	list_del_init(&tty->tty_files);
 	file_list_unlock();
-	module_put(tty->driver->owner);
+
 	free_tty_struct(tty);
 }
 
+static void release_mem(struct tty_struct *tty, int idx)
+{
+	if (tty->link)
+		release_tty(tty->link, idx);
+	release_tty(tty, idx);
+	module_put(tty->driver->owner);
+}
+
 /*
  * Even releasing the tty structures is a tricky business.. We have
  * to be very careful that the structures are all released at the

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

end of thread, other threads:[~2007-01-31 14:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-31  7:16 [PATCH] tty: cleanup release_mem Pekka J Enberg
2007-01-31 14:20 ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2007-01-28 17:12 Christoph Hellwig
2007-01-29 12:01 ` Alan Cox
2007-01-29 18:12   ` Christoph Hellwig
2007-01-29 19:06     ` Alan Cox
2007-01-30  3:56       ` Andrew Morton
2007-01-30  6:33         ` Pekka Enberg

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).