linux-kernel - [patch] tty: fix port buffer locking V2

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]

Message-ID: <1496638347.27038.52.camel@gmx.de>
Date:   Mon, 05 Jun 2017 06:52:27 +0200
From:   Mike Galbraith <efault@....de>
To:     Vegard Nossum <vegard.nossum@...cle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Dave Jones <davej@...emonkey.org.uk>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Jiri Slaby <jslaby@...e.com>
Subject: [patch] tty: fix port buffer locking V2

This is just in case.  While it works, I consider it to be diagnostic
data for those unfortunate enough to be intimate with tty locking :)
---

V1 (925bb1ce47f4) changelog:
tty_insert_flip_string_fixed_flag() is racy against itself when called
from the ioctl(TCXONC, TCION/TCIOFF) path [1] and the flush_to_ldisc()
workqueue path [2].

The problem is that port->buf.tail->used is modified without consistent
locking; the ioctl path takes tty->atomic_write_lock, whereas the workqueue
path takes ldata->output_lock.

We cannot simply take ldata->output_lock, since that is specific to the
N_TTY line discipline.

It might seem natural to try to take port->buf.lock inside
tty_insert_flip_string_fixed_flag() and friends (where port->buf is
actually used/modified), but this creates problems for flush_to_ldisc()
which takes it before grabbing tty->ldisc_sem, o_tty->termios_rwsem,
and ldata->output_lock.

Therefore, the simplest solution for now seems to be to take
tty->atomic_write_lock inside tty_port_default_receive_buf(). This lock
is also used in the write path [3] with a consistent ordering.

[1]: Call Trace:
 tty_insert_flip_string_fixed_flag
 pty_write
 tty_send_xchar                     // down_read(&o_tty->termios_rwsem)
                                    // mutex_lock(&tty->atomic_write_lock)
 n_tty_ioctl_helper
 n_tty_ioctl
 tty_ioctl                          // down_read(&tty->ldisc_sem)
 do_vfs_ioctl
 SyS_ioctl

[2]: Workqueue: events_unbound flush_to_ldisc
Call Trace:
 tty_insert_flip_string_fixed_flag
 pty_write
 tty_put_char
 __process_echoes
 commit_echoes                      // mutex_lock(&ldata->output_lock)
 n_tty_receive_buf_common
 n_tty_receive_buf2
 tty_ldisc_receive_buf              // down_read(&o_tty->termios_rwsem)
 tty_port_default_receive_buf       // down_read(&tty->ldisc_sem)
 flush_to_ldisc                     // mutex_lock(&port->buf.lock)
 process_one_work

[3]: Call Trace:
 tty_insert_flip_string_fixed_flag
 pty_write
 n_tty_write                        // mutex_lock(&ldata->output_lock)
                                    // down_read(&tty->termios_rwsem)
 do_tty_write (inline)              // mutex_lock(&tty->atomic_write_lock)
 tty_write                          // down_read(&tty->ldisc_sem)
 __vfs_write
 vfs_write
 SyS_write

The bug can result in about a dozen different crashes depending on what
exactly gets corrupted when port->buf.tail->used points outside the
buffer.

The patch passes my LOCKDEP/PROVE_LOCKING testing but more testing is
always welcome.

Found using syzkaller.

V2: The V1 solution induced an ordering issue, holding buf->lock while
acquiring tty->atomic_write_lock.  Resolve it by moving acquisition to
flush_to_ldisc(), prior to acquisition of buf->lock.

Credit to Vegard Nossum for problem analysis/resolution, blame to me for
trivial adaptation thereof.

Signed-off-by: Mike Galbraith <efault@....de>
Cc: Vegard Nossum <vegard.nossum@...cle.com>
Cc: <stable@...r.kernel.org>
---
 drivers/tty/tty_buffer.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -465,7 +465,13 @@ static void flush_to_ldisc(struct work_s
 {
 	struct tty_port *port = container_of(work, struct tty_port, buf.work);
 	struct tty_bufhead *buf = &port->buf;
+	struct tty_struct *tty = READ_ONCE(port->itty);
+	struct tty_ldisc *disc = NULL;

+	if (tty)
+		disc = tty_ldisc_ref(tty);
+	if (disc)
+		mutex_lock(&tty->atomic_write_lock);
 	mutex_lock(&buf->lock);

 	while (1) {
@@ -501,6 +507,10 @@ static void flush_to_ldisc(struct work_s
 	}

 	mutex_unlock(&buf->lock);
+	if (disc) {
+		mutex_unlock(&tty->atomic_write_lock);
+		tty_ldisc_deref(disc);
+	}

 }