lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 30 Mar 2011 19:29:38 -0300
From:	Herton Ronaldo Krzesinski <herton.krzesinski@...onical.com>
To:	linux-kernel@...r.kernel.org
Cc:	Alan Cox <alan@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...e.de>
Subject: [PATCH] vt: avoid BUG_ON in con_shutdown when con_open returns with error

In current vt code, if con_open fails we can end up triggering the
BUG_ON in con_shutdown as shown by the following trace:

------------[ cut here ]------------
kernel BUG at /build/buildd/linux-2.6.38/drivers/tty/vt/vt.c:2857!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:1e.0/0000:02:0b.0/resource
Modules linked in: usb_storage uas iptable_filter ip_tables x_tables binfmt_misc snd_ens1371 gameport snd_ac97_codec ac97_bus snd_bt87x snd_pcm snd_seq_midi rc_pixelview snd_rawmidi tuner_simple tuner_types nouveau snd_seq_midi_event ir_lirc_codec tuner ttm lirc_dev tvaudio tda7432 snd_seq ir_sony_decoder drm_kms_helper snd_timer bttv v4l2_common videodev ir_jvc_decoder drm videobuf_dma_sg ir_rc6_decoder snd_seq_device videobuf_core ir_rc5_decoder snd ir_nec_decoder i2c_algo_bit btcx_risc rc_core tveeprom soundcore snd_page_alloc ppdev video parport_pc shpchp psmouse serio_raw lp parport usbhid hid firewire_ohci 8139too firewire_core floppy crc_itu_t

Pid: 4325, comm: Xorg Not tainted 2.6.38-7-generic #37-Ubuntu Compaq                          Presario PC                    /D850GB
EIP: 0060:[<c1313870>] EFLAGS: 00010246 CPU: 0
EIP is at con_shutdown+0x40/0x50
EAX: c02b8800 EBX: c02b8800 ECX: c204a638 EDX: c1313830
ESI: 00000000 EDI: 00000000 EBP: ceda9d2c ESP: ceda9d24
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process Xorg (pid: 4325, ti=ceda8000 task=c17e6500 task.ti=ceda8000)
Stack:
 c02b8800 c12ffb50 ceda9d38 c12ffb6a c02b8804 ceda9d48 c1273f3d c02b8800
 00000000 ceda9d50 c12ff909 ceda9d5c c12ffa46 c02b8800 ceda9de8 c1301f12
 0000c000 000000d0 00000000 00000007 00000000 c02b891c c02b8928 00000000
Call Trace:
 [<c12ffb50>] ? queue_release_one_tty+0x0/0x60
 [<c12ffb6a>] queue_release_one_tty+0x1a/0x60
 [<c1273f3d>] kref_put+0x2d/0x60
 [<c12ff909>] tty_kref_put+0x19/0x20
 [<c12ffa46>] release_tty+0x26/0x40
 [<c1301f12>] tty_release+0x332/0x4e0
 [<c150967f>] ? _raw_spin_lock_irqsave+0x2f/0x50
 [<c1050038>] ? console_unlock+0x98/0xe0
 [<c13022d0>] tty_open+0x210/0x420
 [<c112a805>] chrdev_open+0xa5/0x1c0
 [<c1125031>] __dentry_open+0xc1/0x280
 [<c112639e>] nameidata_to_filp+0x6e/0x80
 [<c112a760>] ? chrdev_open+0x0/0x1c0
 [<c11337ff>] finish_open+0xaf/0x1a0
 [<c11330a8>] ? do_path_lookup+0x68/0x120
 [<c1133e47>] do_filp_open+0x207/0x6e0
 [<c1041387>] ? enqueue_entity+0x247/0x2c0
 [<c1126406>] do_sys_open+0x56/0x120
 [<c104ca10>] ? sched_autogroup_create_attach+0xb0/0x120
 [<c11264fe>] sys_open+0x2e/0x40
 [<c1509904>] syscall_call+0x7/0xb
Code: 01 00 00 89 c3 85 f6 74 22 e8 4d c6 d3 ff c7 06 00 00 00 00 e8 42 c7 d3 ff 89 d8 e8 ab bf fe ff 8b 1c 24 8b 74 24 04 89 ec 5d c3 <0f> 0b 8d b4
[drm] nouveau 0000:01:00.0: GPU lockup - switching to software fbcon
26 00 00 00 00 8d bc 27 00 00 00 00 55 89 e5 3e 8d
EIP: [<c1313870>] con_shutdown+0x40/0x50 SS:ESP 0068:ceda9d24
[drm] nouveau 0000:01:00.0: Setting dpms mode 3 on vga encoder (output 0)
[drm] nouveau 0000:01:00.0: Setting dpms mode 0 on vga encoder (output 0)
[drm] nouveau 0000:01:00.0: Output VGA-1 is running on CRTC 0 using output A
---[ end trace 58cc84d841200747 ]---

This happens because if con_open doesn't succeed, it doesn't set
tty->driver_data, returning an error in tty->ops->open inside tty_open,
which makes tty_release being called later. And as shown in the trace
above tty_release ends up calling con_shutdown with an unset driver_data,
triggering the BUG_ON.

The solution here is to set the shutdown callback only after we know the
con_open will return without error. In the case without shutdown set,
queue_release_one_tty will call tty_shutdown for us, so it's not a
problem not calling con_shutdown.

This fix is in vt only, and from what I see there is no other place we
can fix it (we may have tty_operations users like pty which don't want a
tty_shutdown when tty->ops->open fails). Also from a quick look, this is
a vt specific failure.

BugLink: http://bugs.launchpad.net/bugs/745025
Cc: stable@...nel.org
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@...onical.com>
---
 drivers/tty/vt/vt.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index c83cdfb..98531ba 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -145,6 +145,7 @@ struct vc vc_cons [MAX_NR_CONSOLES];
 static const struct consw *con_driver_map[MAX_NR_CONSOLES];
 #endif
 
+static inline void con_ops_set_shutdown(void);
 static int con_open(struct tty_struct *, struct file *);
 static void vc_init(struct vc_data *vc, unsigned int rows,
 		    unsigned int cols, int do_clear);
@@ -2806,6 +2807,14 @@ static int con_open(struct tty_struct *tty, struct file *filp)
 			tty->driver_data = vc;
 			vc->port.tty = tty;
 
+			/* We must set shutdown only here, otherwise
+			 * we returned from con_open with error, which
+			 * will make tty core call tty_release, that
+			 * in its call path makes con_shutdown being
+			 * called without tty->driver_data being set,
+			 * triggering the BUG_ON there */
+			con_ops_set_shutdown();
+
 			if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
 				tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
 				tty->winsize.ws_col = vc_cons[currcons].d->vc_cols;
@@ -2942,7 +2951,7 @@ static int __init con_init(void)
 }
 console_initcall(con_init);
 
-static const struct tty_operations con_ops = {
+static struct tty_operations con_ops = {
 	.open = con_open,
 	.close = con_close,
 	.write = con_write,
@@ -2958,10 +2967,15 @@ static const struct tty_operations con_ops = {
 	.start = con_start,
 	.throttle = con_throttle,
 	.unthrottle = con_unthrottle,
-	.resize = vt_resize,
-	.shutdown = con_shutdown
+	.resize = vt_resize
 };
 
+/* used by con_open to delay con_shutdown usage */
+static inline void con_ops_set_shutdown(void)
+{
+	con_ops.shutdown = con_shutdown;
+}
+
 static struct cdev vc0_cdev;
 
 static ssize_t show_tty_active(struct device *dev,
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ