BUG: NULL pointer deref in tty port / uart

DISCUSSION Frederic Weisbecker 5 participants 11 messages May 17, 2011 – May 19, 2011 TTY layer and serial drivers

Share & tools

Frederic Weisbecker

author

2011-05-17 23:12 UTC #0 lore raw

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

Alan Cox

2011-05-17 23:58 UTC #1 lore raw

Quoted 8 lines

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.

Jiri Olsa

2011-05-18 14:26 UTC #2 lore raw

Quoted 14 lines · Alan Cox

Quoted 8 lines
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.

Quoted 8 lines

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

drivers/tty/serial/serial_core.c +3 -1

index 733fe8e..86a40cb 100644
@@ -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;

Signature · 1 line

1.7.1

Alan Cox

2011-05-18 14:35 UTC #3 lore raw

Quoted 3 lines

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.

Quoted 1 line

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.

Jiri Olsa

2011-05-18 14:44 UTC #4 lore raw

Quoted 9 lines · Alan Cox

Quoted 3 lines
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.

Quoted 6 lines

Quoted 1 line
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

Alan Cox

2011-05-18 15:04 UTC #5 lore raw

Quoted 10 lines

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.

Greg KH

2011-05-18 19:42 UTC #6 lore raw

Quoted 15 lines · Alan Cox

Quoted 10 lines
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.

Quoted 5 lines

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

Jiri Olsa

2011-05-19 11:19 UTC #7 lore raw

Quoted 31 lines · Greg KH

Quoted 15 lines · Alan Cox
Quoted 10 lines
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.
Quoted 5 lines
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?

Quoted 7 lines

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

Greg KH

2011-05-19 13:03 UTC #8 lore raw

Quoted 36 lines · Jiri Olsa

Quoted 35 lines · Greg KH
Quoted 15 lines · Alan Cox
Quoted 10 lines
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.
Quoted 5 lines
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.

Quoted 5 lines

Quoted 3 lines
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

Jiri Olsa

2011-05-19 13:58 UTC #9 lore raw

Quoted 37 lines · Greg KH

Quoted 36 lines · Jiri Olsa
Quoted 35 lines · Greg KH
Quoted 15 lines · Alan Cox
Quoted 10 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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

Frederic Weisbecker

author

2011-05-19 14:07 UTC #10 lore raw

Quoted 40 lines · Jiri Olsa

Quoted 39 lines · Greg KH
Quoted 38 lines · Jiri Olsa
Quoted 37 lines · Greg KH
Quoted 15 lines · Alan Cox
Quoted 10 lines
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.
Quoted 5 lines
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,

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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!