linux-kernel - [PATCH v2 2/2] TTY: open/hangup race fixup

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 29 Nov 2010 10:16:54 +0100
From:	Jiri Slaby <jslaby@...e.cz>
To:	gregkh@...e.de
Cc:	linux-kernel@...r.kernel.org, jirislaby@...il.com,
	Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: [PATCH v2 2/2] TTY: open/hangup race fixup

Like in the "TTY: don't allow reopen when ldisc is changing" patch,
this one fixes a TTY WARNING as described in the option 1) there:
1) __tty_hangup from tty_ldisc_hangup to tty_ldisc_enable. During this
section tty_lock is held. However tty_lock is temporarily dropped in
the middle of the function by tty_ldisc_hangup.

The fix is to introduce a new flag which we set during the unlocked
window and check it in tty_reopen too. The flag is TTY_HUPPING and is
cleared after TTY_HUPPED is set.

While at it, remove duplicate TTY_HUPPED set_bit. The one after
calling ops->hangup seems to be more correct. But anyway, we hold
tty_lock, so there should be no difference.

Also document the function it does that kind of crap.

Nicely reproducible with two forked children:
static void do_work(const char *tty)
{
	if (signal(SIGHUP, SIG_IGN) == SIG_ERR) exit(1);
	setsid();
	while (1) {
		int fd = open(tty, O_RDWR|O_NOCTTY);
		if (fd < 0) continue;
		if (ioctl(fd, TIOCSCTTY)) continue;
		if (vhangup()) continue;
		close(fd);
	}
	exit(0);
}

Signed-off-by: Jiri Slaby <jslaby@...e.cz>
Reported-by: <Valdis.Kletnieks@...edu>
Reported-by: Kyle McMartin <kyle@...artin.ca>
Cc: Alan Cox <alan@...rguk.ukuu.org.uk>
---
I don't see a better way how to fix it, so I promoted the
proof-of-concept to a real patch.

 drivers/tty/tty_io.c |   10 +++++++++-
 include/linux/tty.h  |    1 +
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 878f6d6..35480dd 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -559,6 +559,9 @@ void __tty_hangup(struct tty_struct *tty)
 
 	tty_lock();
 
+	/* some functions below drop BTM, so we need this bit */
+	set_bit(TTY_HUPPING, &tty->flags);
+
 	/* inuse_filps is protected by the single tty lock,
 	   this really needs to change if we want to flush the
 	   workqueue with the lock held */
@@ -578,6 +581,10 @@ void __tty_hangup(struct tty_struct *tty)
 	}
 	spin_unlock(&tty_files_lock);
 
+	/*
+	 * it drops BTM and thus races with reopen
+	 * we protect the race by TTY_HUPPING
+	 */
 	tty_ldisc_hangup(tty);
 
 	read_lock(&tasklist_lock);
@@ -615,7 +622,6 @@ void __tty_hangup(struct tty_struct *tty)
 	tty->session = NULL;
 	tty->pgrp = NULL;
 	tty->ctrl_status = 0;
-	set_bit(TTY_HUPPED, &tty->flags);
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 
 	/* Account for the p->signal references we killed */
@@ -641,6 +647,7 @@ void __tty_hangup(struct tty_struct *tty)
 	 * can't yet guarantee all that.
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
+	clear_bit(TTY_HUPPING, &tty->flags);
 	tty_ldisc_enable(tty);
 
 	tty_unlock();
@@ -1311,6 +1318,7 @@ static int tty_reopen(struct tty_struct *tty)
 	struct tty_driver *driver = tty->driver;
 
 	if (test_bit(TTY_CLOSING, &tty->flags) ||
+			test_bit(TTY_HUPPING, &tty->flags) ||
 			test_bit(TTY_LDISC_CHANGING, &tty->flags))
 		return -EIO;
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 032d79f..54e4eaa 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -366,6 +366,7 @@ struct tty_file_private {
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
 #define TTY_FLUSHING		19	/* Flushing to ldisc in progress */
 #define TTY_FLUSHPENDING	20	/* Queued buffer flush pending */
+#define TTY_HUPPING 		21	/* ->hangup() in progress */
 
 #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
 
-- 
1.7.3.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/