From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751295AbdFEEwv convert rfc822-to-8bit (ORCPT ); Mon, 5 Jun 2017 00:52:51 -0400 Received: from mout.gmx.net ([212.227.17.20]:55276 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751231AbdFEEwu (ORCPT ); Mon, 5 Jun 2017 00:52:50 -0400 Message-ID: <1496638347.27038.52.camel@gmx.de> Subject: [patch] tty: fix port buffer locking V2 From: Mike Galbraith To: Vegard Nossum , Greg Kroah-Hartman Cc: Dave Jones , Linux Kernel , Jiri Slaby Date: Mon, 05 Jun 2017 06:52:27 +0200 In-Reply-To: <6b025884-8fbc-1ae2-3119-63e601453ad6@oracle.com> References: <20170531172117.nj5fwgqspibg7rs6@codemonkey.org.uk> <1496471632.11868.1.camel@gmx.de> <20170604083225.GA2959@kroah.com> <1496566972.16375.10.camel@gmx.de> <6b025884-8fbc-1ae2-3119-63e601453ad6@oracle.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 Mime-Version: 1.0 Content-Transfer-Encoding: 8BIT X-Provags-ID: V03:K0:Z6ek5nofHyB37Vdbrqo2gug63WDd2ZRwfM53jFyDNvHUaCLUOm4 zTWBChqw25LMsWEPedK7MLqsQGGLEgurAEF8HGqT2PEF/PZR+z0vzcegGYyMkoAbnzxO0l0 GWR9oAwJgQSLs4eX3xPBENIEbMLwPxMgD49uAO8ofseblwXvesyy5cL5gse6vFbWVsUSOhC NQO0DQ5QZOySEitTBrA3g== X-UI-Out-Filterresults: notjunk:1;V01:K0:oOKqUWCWIz0=:+5dSrUQV78Y52M6TiEapo9 osSsTE0+iQoe4yFJ8SDuKGkOfg6LVZsoOX22uoYY5qvKbKLkmSDaeJzkRyFvLePt12cwNd3NY d95dUK3KDe+7POIkPa+OltaFQ7Y1MOrZJ6IQxgvbZJ3QaYwKDbrVpHgRNkBD/ClfUeAs/Xo+2 1t3bKaJmyJP6EOdZ6IAt3XaKg3XcU2JM3iY15BAQckLKtVMVVWHsNglchCQktpWLYCe5InCUS vNOhE1JwhJluQUMJVV4jUm/O4HIc2/QCp+gx0lLgMBIgi2IHGqV+R/2pD+G+9z9XTGPTP5CnG qI9tWfZ4D7qD2wUqpk+aViDjwyqKYp4X4qseSCmtMb+R2/vVvNRMKir5sGTo8ODIBSHRz18qu 2hfp9a3+3MdfV8RGSWmRdAm2WrMdKp5Wb6Imw5CFKU0CYUVphNamVBwrM4G9nkCOT2snRY6Oo jwxxBB+YG4XSZ8ietegi2gd6Rd8sZ2Vh7W0MbPofjEcM4/kcCnoUnHIoAUTMkkCy/B+zG+IaD E6jbq6RkxJ12xJeWz4h1o3BXhqQpNSDCg/F/t3DF11n7i5ys0Cz1yaAgcd3zu11OzKewOKgvF ubIYeTL6MLVlgb8FwDVQ+nmTj8YlGlfGX0sU6wx+la0DcI28wUSS/MZnJyyjiiNS1rTxS32jw wkvhKU+z6N4I8iBWaQgikXiaV1wvGeU33tPsZhzBTEl9ZovVZPHvg5qR6cuxuTc4s6evqowTH fJWn0sH7pUnOTfj1+Ra8Y6+5+h/5Stnst4XbQTzZ4fA+DarGMdkWfZzEKHxUMd2s7fCQsw69z 7Cpadr7 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Cc: Vegard Nossum Cc: --- 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); + } }