On Wed, May 31, 2017 at 5:04 PM, Alan Cox <gnomes@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 31 May 2017 20:16:12 +0900
> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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 <gnomes@xxxxxxxxxxxxxxxxxxx> 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 <alan@llwyncelyn.cymru>
> 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 <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/tty.h>
> +#include <linux/module.h>
> +
> +/*
> + * 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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
