BUG: NULL pointer deref in tty port / uart

BUG: NULL pointer deref in tty port / uart

[Frederic Weisbecker]

Hi,

This happens in latest linus tree (v2.6.39-rc7) and I don't know the
earliest kernel that has this bug. I tested down to 2.6.36 which has
the same issue.

To reproduce, do the following steps, with a tty dev matching an 
unplugged serial line:

	echo 1 > /dev/ttyS4 # which blocks

And on another console:

	cat /dev/ttyS4 # which blocks

Then Ctrl + C the echo in the first console. This produces the
following trace:

[ 1494.395774] BUG: unable to handle kernel NULL pointer dereference at 00000000000001e0
[ 1494.400002] IP: [<ffffffff8143bb5b>] uart_dtr_rts+0x9b/0x180
[ 1494.400002] PGD 7a6ce067 PUD 761d3067 PMD 0 
[ 1494.400002] Oops: 0000 [#1] PREEMPT SMP 
[ 1494.400002] last sysfs file: /sys/devices/system/cpu/cpu3/cache/index2/shared_cpu_map
[ 1494.400002] CPU 3 
[ 1494.400002] Modules linked in:
[ 1494.400002] 
[ 1494.400002] Pid: 1336, comm: cat Not tainted 2.6.39-rc7+ #14 Dell Inc. PowerEdge SC1430/0TW856
[ 1494.400002] RIP: 0010:[<ffffffff8143bb5b>]  [<ffffffff8143bb5b>] uart_dtr_rts+0x9b/0x180
[ 1494.400002] RSP: 0018:ffff8800761a5ab8  EFLAGS: 00010297
[ 1494.400002] RAX: ffffffff82059a80 RBX: ffff88007b160aa0 RCX: 0000000000000006
[ 1494.400002] RDX: 0000000000000000 RSI: ffff88007a656588 RDI: ffffffff8143bb23
[ 1494.400002] RBP: ffff8800761a5ad8 R08: 0000000000000000 R09: 0000000000000002
[ 1494.400002] R10: 0000000000000001 R11: 0000000000000001 R12: ffffffff82acf9a0
[ 1494.400002] R13: 0000000000000000 R14: ffff88007b160af0 R15: ffff88007a655ee0
[ 1494.400002] FS:  00007f708de3c720(0000) GS:ffff88007fcc0000(0000) knlGS:0000000000000000
[ 1494.400002] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1494.400002] CR2: 00000000000001e0 CR3: 0000000079e77000 CR4: 00000000000006e0
[ 1494.400002] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1494.400002] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1494.400002] Process cat (pid: 1336, threadinfo ffff8800761a4000, task ffff88007a655ee0)
[ 1494.400002] Stack:
[ 1494.400002]  ffff88007b160aa0 ffff88007b160aa0 ffff8800794d3180 ffff88007a49d000
[ 1494.400002]  ffff8800761a5b88 ffffffff81426a84 ffff88007b160ab0 0000000081092de0
[ 1494.400002]  ffff8800761a5b18 ffff88007a655ee0 ffffffff819c13b5 ffff88007b160c18
[ 1494.400002] Call Trace:
[ 1494.400002]  [<ffffffff81426a84>] tty_port_block_til_ready+0x1d4/0x350
[ 1494.400002]  [<ffffffff819c13b5>] ? __mutex_unlock_slowpath+0xf5/0x170
[ 1494.400002]  [<ffffffff81092f4d>] ? trace_hardirqs_on_caller+0x13d/0x180
[ 1494.400002]  [<ffffffff8107a980>] ? wake_up_bit+0x40/0x40
[ 1494.400002]  [<ffffffff814390e0>] uart_open+0x160/0x1f0
[ 1494.400002]  [<ffffffff8141eb42>] tty_open+0x232/0x580
[ 1494.400002]  [<ffffffff81150d74>] chrdev_open+0x154/0x310
[ 1494.400002]  [<ffffffff81150c20>] ? cdev_put+0x30/0x30
[ 1494.400002]  [<ffffffff81149c27>] __dentry_open+0x187/0x440
[ 1494.400002]  [<ffffffff8114b531>] nameidata_to_filp+0x71/0x80
[ 1494.400002]  [<ffffffff8115a3db>] do_last+0xfb/0x970
[ 1494.400002]  [<ffffffff8115c446>] path_openat+0xc6/0x3d0
[ 1494.400002]  [<ffffffff8111423e>] ? might_fault+0x4e/0xa0
[ 1494.400002]  [<ffffffff8115c78d>] do_filp_open+0x3d/0xa0
[ 1494.400002]  [<ffffffff819c2f20>] ? _raw_spin_unlock+0x30/0x60
[ 1494.400002]  [<ffffffff8116a49d>] ? alloc_fd+0x19d/0x200
[ 1494.400002]  [<ffffffff8114b63c>] do_sys_open+0xfc/0x1d0
[ 1494.400002]  [<ffffffff8114b72b>] sys_open+0x1b/0x20
[ 1494.400002]  [<ffffffff819c8afb>] system_call_fastpath+0x16/0x1b
[ 1494.400002] Code: 75 33 4c 8b a3 a0 02 00 00 4c 8b 2b 49 8b 84 24 c8 00 00 00 48 85 c0 74 12 0f bf 50 42 41 3b 94 24 f4 00 00 00 0f 84 b5 00 00 00 
[ 1494.400002]  f6 85 e0 01 00 00 02 74 63 48 8b 5d e8 4c 8b 65 f0 4c 8b 6d 
[ 1494.400002] RIP  [<ffffffff8143bb5b>] uart_dtr_rts+0x9b/0x180
[ 1494.400002]  RSP <ffff8800761a5ab8>
[ 1494.400002] CR2: 00000000000001e0

Re: BUG: NULL pointer deref in tty port / uart

[Alan Cox]

> 	echo 1 > /dev/ttyS4 # which blocks
> 
> And on another console:
> 
> 	cat /dev/ttyS4 # which blocks
> 
> Then Ctrl + C the echo in the first console. This produces the
> following trace:

First cat is in tty_port_block_til_ready, second cat joins it there. ^C
causes one to close, which wakes the second which goes around the loop
again, tries to raise the carrier and explodes, it seems because
someone trashed memory it is using.

Not quite sure why at this point

On the first exit of the open path port->count is 1 which is as we want
it. Close takes it down to zero which triggers the port shutdown path
which is as we want. We clean up port->tty and shut down the port.
Seeing the second pending open we wake it which is when it goes kaboom

Nothing obvious strikes me from reading the code.

Re: BUG: NULL pointer deref in tty port / uart

[Jiri Olsa]

On Wed, May 18, 2011 at 12:44:20AM +0100, Alan Cox wrote:
> > 	echo 1 > /dev/ttyS4 # which blocks
> > 
> > And on another console:
> > 
> > 	cat /dev/ttyS4 # which blocks
> > 
> > Then Ctrl + C the echo in the first console. This produces the
> > following trace:
> 
> First cat is in tty_port_block_til_ready, second cat joins it there. ^C
> causes one to close, which wakes the second which goes around the loop
> again, tries to raise the carrier and explodes, it seems because
> someone trashed memory it is using.
> 
> Not quite sure why at this point
> 
> On the first exit of the open path port->count is 1 which is as we want
> it. Close takes it down to zero which triggers the port shutdown path
> which is as we want. We clean up port->tty and shut down the port.
> Seeing the second pending open we wake it which is when it goes kaboom
> 
> Nothing obvious strikes me from reading the code.

hi,

have the same issue.. looks like we should not NULL the port->tty
if there's blocked open, but not sure what's exactly the logic
behind "port's block_open and count" ..

attached patch fixes it for me

wbr,
jirka

---
 drivers/tty/serial/serial_core.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 733fe8e..86a40cb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1346,7 +1346,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 
 	tty_ldisc_flush(tty);
 
-	tty_port_tty_set(port, NULL);
+	if (!tty_port_users(port))
+		tty_port_tty_set(port, NULL);
+
 	spin_lock_irqsave(&port->lock, flags);
 	tty->closing = 0;
 
-- 
1.7.1

Re: BUG: NULL pointer deref in tty port / uart

[Alan Cox]

> have the same issue.. looks like we should not NULL the port->tty
> if there's blocked open, but not sure what's exactly the logic
> behind "port's block_open and count" ..

A pending open is not a user of the tty as far as the rest of the stack
is concerned. I also don't see why clearing port->tty is causing this
crash because nothing on that path should ever be going via port->tty and
it isn't safe to do so.

> attached patch fixes it for me

But still breaks on hangup where we can't do that.

Where is port->tty getting misused to cause the crash, that is the bit
I'm missing somewhere.

Re: BUG: NULL pointer deref in tty port / uart

[Jiri Olsa]

On Wed, May 18, 2011 at 03:36:36PM +0100, Alan Cox wrote:
> > have the same issue.. looks like we should not NULL the port->tty
> > if there's blocked open, but not sure what's exactly the logic
> > behind "port's block_open and count" ..
> 
> A pending open is not a user of the tty as far as the rest of the stack
> is concerned. I also don't see why clearing port->tty is causing this
> crash because nothing on that path should ever be going via port->tty and
> it isn't safe to do so.
> 
> > attached patch fixes it for me
> 
> But still breaks on hangup where we can't do that.
> 
> Where is port->tty getting misused to cause the crash, that is the bit
> I'm missing somewhere.

I think it's the 

uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)

called path:

tty_port_block_til_ready
	tty_port_raise_dtr_rts
		uart_dtr_rts
			uart_update_termios

jirka

Re: BUG: NULL pointer deref in tty port / uart

[Alan Cox]

> I think it's the 
> 
> uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)
> 
> called path:
> 
> tty_port_block_til_ready
> 	tty_port_raise_dtr_rts
> 		uart_dtr_rts
> 			uart_update_termios

Ah that would explain why I can't find and dup it - it isn't found in
the current -next tree.


c7d7abff40c27f82fe78b1091ab3fad69b2546f9 (and thereafter)

Jiri Slaby fixed it in sorting out uart_startup

I guess these should now get tagged for -stable.

Re: BUG: NULL pointer deref in tty port / uart

[Greg KH]

On Wed, May 18, 2011 at 03:50:33PM +0100, Alan Cox wrote:
> > I think it's the 
> > 
> > uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)
> > 
> > called path:
> > 
> > tty_port_block_til_ready
> > 	tty_port_raise_dtr_rts
> > 		uart_dtr_rts
> > 			uart_update_termios
> 
> Ah that would explain why I can't find and dup it - it isn't found in
> the current -next tree.
> 
> 
> c7d7abff40c27f82fe78b1091ab3fad69b2546f9 (and thereafter)
> 
> Jiri Slaby fixed it in sorting out uart_startup
> 
> I guess these should now get tagged for -stable.

So that would be the following patches in the linux-next tree:
	c7d7abff40c27f82fe78b1091ab3fad69b2546f9 serial: core, move termios handling to uart_startup
	303a7a1199c20f7c9452f024a6e17bf348b6b398 serial: core, do not set DTR/RTS twice on startup
	6f5c24ad0f7619502199185a026a228174a27e68 serial: core, remove uart_update_termios

right?

Anything else need to be backported?

Frederic, can you test that these 3 patches solve the problem for you?
If you want, I can send them to you separately if you don't have a copy
of linux-next around anywhere.

thanks,

greg k-h

Re: BUG: NULL pointer deref in tty port / uart

[Jiri Olsa]

On Wed, May 18, 2011 at 12:42:12PM -0700, Greg KH wrote:
> On Wed, May 18, 2011 at 03:50:33PM +0100, Alan Cox wrote:
> > > I think it's the 
> > > 
> > > uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)
> > > 
> > > called path:
> > > 
> > > tty_port_block_til_ready
> > > 	tty_port_raise_dtr_rts
> > > 		uart_dtr_rts
> > > 			uart_update_termios
> > 
> > Ah that would explain why I can't find and dup it - it isn't found in
> > the current -next tree.
> > 
> > 
> > c7d7abff40c27f82fe78b1091ab3fad69b2546f9 (and thereafter)
> > 
> > Jiri Slaby fixed it in sorting out uart_startup
> > 
> > I guess these should now get tagged for -stable.
> 
> So that would be the following patches in the linux-next tree:
> 	c7d7abff40c27f82fe78b1091ab3fad69b2546f9 serial: core, move termios handling to uart_startup
> 	303a7a1199c20f7c9452f024a6e17bf348b6b398 serial: core, do not set DTR/RTS twice on startup
> 	6f5c24ad0f7619502199185a026a228174a27e68 serial: core, remove uart_update_termios
> 
> right?
> 
> Anything else need to be backported?
> 
> Frederic, can you test that these 3 patches solve the problem for you?
> If you want, I can send them to you separately if you don't have a copy
> of linux-next around anywhere.
> 
> thanks,
> 
> greg k-h

I tried linux-next and cannot hit the issue anymore

thanks,
jirka

Re: BUG: NULL pointer deref in tty port / uart

[Greg KH]

On Thu, May 19, 2011 at 01:19:01PM +0200, Jiri Olsa wrote:
> On Wed, May 18, 2011 at 12:42:12PM -0700, Greg KH wrote:
> > On Wed, May 18, 2011 at 03:50:33PM +0100, Alan Cox wrote:
> > > > I think it's the 
> > > > 
> > > > uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)
> > > > 
> > > > called path:
> > > > 
> > > > tty_port_block_til_ready
> > > > 	tty_port_raise_dtr_rts
> > > > 		uart_dtr_rts
> > > > 			uart_update_termios
> > > 
> > > Ah that would explain why I can't find and dup it - it isn't found in
> > > the current -next tree.
> > > 
> > > 
> > > c7d7abff40c27f82fe78b1091ab3fad69b2546f9 (and thereafter)
> > > 
> > > Jiri Slaby fixed it in sorting out uart_startup
> > > 
> > > I guess these should now get tagged for -stable.
> > 
> > So that would be the following patches in the linux-next tree:
> > 	c7d7abff40c27f82fe78b1091ab3fad69b2546f9 serial: core, move termios handling to uart_startup
> > 	303a7a1199c20f7c9452f024a6e17bf348b6b398 serial: core, do not set DTR/RTS twice on startup
> > 	6f5c24ad0f7619502199185a026a228174a27e68 serial: core, remove uart_update_termios
> > 
> > right?
> > 
> > Anything else need to be backported?
> > 
> > Frederic, can you test that these 3 patches solve the problem for you?
> > If you want, I can send them to you separately if you don't have a copy
> > of linux-next around anywhere.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I tried linux-next and cannot hit the issue anymore

That's great, but can you try .38 or .39 with those patches above and
verify that they solve the problem?

thanks,

greg k-h

Re: BUG: NULL pointer deref in tty port / uart

[Jiri Olsa]

On Thu, May 19, 2011 at 05:51:20AM -0700, Greg KH wrote:
> On Thu, May 19, 2011 at 01:19:01PM +0200, Jiri Olsa wrote:
> > On Wed, May 18, 2011 at 12:42:12PM -0700, Greg KH wrote:
> > > On Wed, May 18, 2011 at 03:50:33PM +0100, Alan Cox wrote:
> > > > > I think it's the 
> > > > > 
> > > > > uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)
> > > > > 
> > > > > called path:
> > > > > 
> > > > > tty_port_block_til_ready
> > > > > 	tty_port_raise_dtr_rts
> > > > > 		uart_dtr_rts
> > > > > 			uart_update_termios
> > > > 
> > > > Ah that would explain why I can't find and dup it - it isn't found in
> > > > the current -next tree.
> > > > 
> > > > 
> > > > c7d7abff40c27f82fe78b1091ab3fad69b2546f9 (and thereafter)
> > > > 
> > > > Jiri Slaby fixed it in sorting out uart_startup
> > > > 
> > > > I guess these should now get tagged for -stable.
> > > 
> > > So that would be the following patches in the linux-next tree:
> > > 	c7d7abff40c27f82fe78b1091ab3fad69b2546f9 serial: core, move termios handling to uart_startup
> > > 	303a7a1199c20f7c9452f024a6e17bf348b6b398 serial: core, do not set DTR/RTS twice on startup
> > > 	6f5c24ad0f7619502199185a026a228174a27e68 serial: core, remove uart_update_termios
> > > 
> > > right?
> > > 
> > > Anything else need to be backported?
> > > 
> > > Frederic, can you test that these 3 patches solve the problem for you?
> > > If you want, I can send them to you separately if you don't have a copy
> > > of linux-next around anywhere.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > I tried linux-next and cannot hit the issue anymore
> 
> That's great, but can you try .38 or .39 with those patches above and
> verify that they solve the problem?

yes, this works for me as well

jirka

Re: BUG: NULL pointer deref in tty port / uart

[Frederic Weisbecker]

On Thu, May 19, 2011 at 03:58:29PM +0200, Jiri Olsa wrote:
> On Thu, May 19, 2011 at 05:51:20AM -0700, Greg KH wrote:
> > On Thu, May 19, 2011 at 01:19:01PM +0200, Jiri Olsa wrote:
> > > On Wed, May 18, 2011 at 12:42:12PM -0700, Greg KH wrote:
> > > > On Wed, May 18, 2011 at 03:50:33PM +0100, Alan Cox wrote:
> > > > > > I think it's the 
> > > > > > 
> > > > > > uart_update_termios in uart_dtr_rts (drivers/tty/serial/serial_core.c)
> > > > > > 
> > > > > > called path:
> > > > > > 
> > > > > > tty_port_block_til_ready
> > > > > > 	tty_port_raise_dtr_rts
> > > > > > 		uart_dtr_rts
> > > > > > 			uart_update_termios
> > > > > 
> > > > > Ah that would explain why I can't find and dup it - it isn't found in
> > > > > the current -next tree.
> > > > > 
> > > > > 
> > > > > c7d7abff40c27f82fe78b1091ab3fad69b2546f9 (and thereafter)
> > > > > 
> > > > > Jiri Slaby fixed it in sorting out uart_startup
> > > > > 
> > > > > I guess these should now get tagged for -stable.
> > > > 
> > > > So that would be the following patches in the linux-next tree:
> > > > 	c7d7abff40c27f82fe78b1091ab3fad69b2546f9 serial: core, move termios handling to uart_startup
> > > > 	303a7a1199c20f7c9452f024a6e17bf348b6b398 serial: core, do not set DTR/RTS twice on startup
> > > > 	6f5c24ad0f7619502199185a026a228174a27e68 serial: core, remove uart_update_termios
> > > > 
> > > > right?
> > > > 
> > > > Anything else need to be backported?
> > > > 
> > > > Frederic, can you test that these 3 patches solve the problem for you?
> > > > If you want, I can send them to you separately if you don't have a copy
> > > > of linux-next around anywhere.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > I tried linux-next and cannot hit the issue anymore
> > 
> > That's great, but can you try .38 or .39 with those patches above and
> > verify that they solve the problem?
> 
> yes, this works for me as well

Thanks for your test!