Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751161AbdFAMGc (ORCPT ); Thu, 1 Jun 2017 08:06:32 -0400 Received: from mail-ua0-f172.google.com ([209.85.217.172]:33577 "EHLO mail-ua0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751056AbdFAMGa (ORCPT ); Thu, 1 Jun 2017 08:06:30 -0400 MIME-Version: 1.0 In-Reply-To: <20170531160420.3b173a4a@alans-desktop> References: <20170413183403.GA16022@kroah.com> <20170414123029.GA17217@kroah.com> <20170503120101.GA21119@kroah.com> <20170530130905.382487d5@alans-desktop> <20170531111612.GA6332@kroah.com> <20170531160420.3b173a4a@alans-desktop> From: Dmitry Vyukov Date: Thu, 1 Jun 2017 14:06:08 +0200 Message-ID: Subject: Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4 To: Alan Cox Cc: Greg KH , Vegard Nossum , Linus Torvalds , Jiri Slaby , Andrew Morton , LKML , linux-serial Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9015 Lines: 237 On Wed, May 31, 2017 at 5:04 PM, Alan Cox wrote: > On Wed, 31 May 2017 20:16:12 +0900 > Greg KH wrote: > >> On Wed, May 31, 2017 at 10:39:23AM +0200, Dmitry Vyukov wrote: >> > On Tue, May 30, 2017 at 2:09 PM, Alan Cox wrote: >> > >> >> I'll think about possible solutions, but I have no prior experience >> > >> >> with the tty code. In the meantime syzkaller also hit a couple of >> > >> >> other fun tty/pty bugs including a write/ioctl race that results in >> > >> >> buffer overflow :-/ >> > > >> > > There are several of those, including some of that have been documented >> > > for years but nobody ever volunteered to fix - in particular all the >> > > interfaces that push characters to the tty other than via the normal >> > > interrupt receive path are dodgy (console selection in particular) >> > > >> > > The original tty model btw was that setting the ldisc to n_tty cannot >> > > fail, and the structure allocated was smaller than a page size so was >> > > safe. >> > > >> > > The simple way to fix it is to restore that behaviour by adding a 'null' >> > > ldisc that we can fail to instead of N_TTY since the N_TTY failback path >> > > is long broken. >> > >> > Greg, what do you think about this patch? Are you ready to accept >> > something like this? >> > Definitely shorter than changing all drivers. >> >> Yes, it looks reasonable to me. > > > > Ok try this I've applied the patch and run syzkaller with it. I don't see kernel panics in tty_ldisc_restore any more. Also don't see any new tty-related crashes. Greg, will you take it from here? > commit f6db8de7eca11cfeafa92f2ec866fa75425c5f38 > Author: Alan Cox > Date: Tue May 30 12:59:45 2017 +0100 > > tty: handle the case where we cannot restore a line discipline > > Historically the N_TTY driver could never fail but this has become broken over > time. Rather than trying to rewrite half the ldisc layer to fix the breakage > introduce a second level of fallback with an N_NULL ldisc which cannot fail, > and thus restore the guarantees required by the ldisc layer. > > We still try and fail to N_TTY first. It's much more useful to find yourself > back in your old ldisc (first attempt) or in N_TTY (second attempt), and while > I'm not aware of any code out there that makes those assumptions it's good to > drive(r) defensively. > > diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile > index f02becd..8689279 100644 > --- a/drivers/tty/Makefile > +++ b/drivers/tty/Makefile > @@ -1,6 +1,7 @@ > obj-$(CONFIG_TTY) += tty_io.o n_tty.o tty_ioctl.o tty_ldisc.o \ > tty_buffer.o tty_port.o tty_mutex.o \ > - tty_ldsem.o tty_baudrate.o tty_jobctrl.o > + tty_ldsem.o tty_baudrate.o tty_jobctrl.o \ > + n_null.o > obj-$(CONFIG_LEGACY_PTYS) += pty.o > obj-$(CONFIG_UNIX98_PTYS) += pty.o > obj-$(CONFIG_AUDIT) += tty_audit.o > diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c > new file mode 100644 > index 0000000..d63261c > --- /dev/null > +++ b/drivers/tty/n_null.c > @@ -0,0 +1,80 @@ > +#include > +#include > +#include > +#include > + > +/* > + * n_null.c - Null line discipline used in the failure path > + * > + * Copyright (C) Intel 2017 > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + */ > + > +static int n_null_open(struct tty_struct *tty) > +{ > + return 0; > +} > + > +static void n_null_close(struct tty_struct *tty) > +{ > +} > + > +static ssize_t n_null_read(struct tty_struct *tty, struct file *file, > + unsigned char __user * buf, size_t nr) > +{ > + return -EOPNOTSUPP; > +} > + > +static ssize_t n_null_write(struct tty_struct *tty, struct file *file, > + const unsigned char *buf, size_t nr) > +{ > + return -EOPNOTSUPP; > +} > + > +static void n_null_receivebuf(struct tty_struct *tty, > + const unsigned char *cp, char *fp, > + int cnt) > +{ > +} > + > +static struct tty_ldisc_ops null_ldisc = { > + .owner = THIS_MODULE, > + .magic = TTY_LDISC_MAGIC, > + .name = "n_null", > + .open = n_null_open, > + .close = n_null_close, > + .read = n_null_read, > + .write = n_null_write, > + .receive_buf = n_null_receivebuf > +}; > + > +static int __init n_null_init(void) > +{ > + BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc)); > + return 0; > +} > + > +static void __exit n_null_exit(void) > +{ > + tty_unregister_ldisc(N_NULL); > +} > + > +module_init(n_null_init); > +module_exit(n_null_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Alan Cox"); > +MODULE_ALIAS_LDISC(N_NULL); > +MODULE_DESCRIPTION("Null ldisc driver"); > diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c > index f6ffe28..2fe216b 100644 > --- a/drivers/tty/tty_ldisc.c > +++ b/drivers/tty/tty_ldisc.c > @@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld) > } > > /** > + * tty_ldisc_failto - helper for ldisc failback > + * @tty: tty to open the ldisc on > + * @ld: ldisc we are trying to fail back to > + * > + * Helper to try and recover a tty when switching back to the old > + * ldisc fails and we need something attached. > + */ > + > +static int tty_ldisc_failto(struct tty_struct *tty, int ld) > +{ > + struct tty_ldisc *disc = tty_ldisc_get(tty, ld); > + int r; > + > + if (IS_ERR(disc)) > + return PTR_ERR(disc); > + tty->ldisc = disc; > + tty_set_termios_ldisc(tty, ld); > + if ((r = tty_ldisc_open(tty, disc)) < 0) > + tty_ldisc_put(disc); > + return r; > +} > + > +/** > * tty_ldisc_restore - helper for tty ldisc change > * @tty: tty to recover > * @old: previous ldisc > @@ -502,9 +525,6 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld) > > static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old) > { > - struct tty_ldisc *new_ldisc; > - int r; > - > /* There is an outstanding reference here so this is safe */ > old = tty_ldisc_get(tty, old->ops->num); > WARN_ON(IS_ERR(old)); > @@ -512,17 +532,13 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old) > tty_set_termios_ldisc(tty, old->ops->num); > if (tty_ldisc_open(tty, old) < 0) { > tty_ldisc_put(old); > - /* This driver is always present */ > - new_ldisc = tty_ldisc_get(tty, N_TTY); > - if (IS_ERR(new_ldisc)) > - panic("n_tty: get"); > - tty->ldisc = new_ldisc; > - tty_set_termios_ldisc(tty, N_TTY); > - r = tty_ldisc_open(tty, new_ldisc); > - if (r < 0) > - panic("Couldn't open N_TTY ldisc for " > - "%s --- error %d.", > - tty_name(tty), r); > + /* The traditional behaviour is to fall back to N_TTY, we > + want to avoid falling back to N_NULL unless we have no > + choice to avoid the risk of breaking anything */ > + if (tty_ldisc_failto(tty, N_TTY) < 0 && > + tty_ldisc_failto(tty, N_NULL) < 0) > + panic("Couldn't open N_NULL ldisc for %s.", > + tty_name(tty)); > } > } > > diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h > index e7855df..cf14553 100644 > --- a/include/uapi/linux/tty.h > +++ b/include/uapi/linux/tty.h > @@ -36,5 +36,6 @@ > #define N_TRACEROUTER 24 /* Trace data routing for MIPI P1149.7 */ > #define N_NCI 25 /* NFC NCI UART */ > #define N_SPEAKUP 26 /* Speakup communication with synths */ > +#define N_NULL 27 /* Null ldisc used for error handling */ > > #endif /* _UAPI_LINUX_TTY_H */