On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote: > tty_set_termios() has the following WARMN_ON which can be triggered with a > syscall to invoke TIOCGETD __NR_ioctl. > > WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY && > tty->driver->subtype == PTY_TYPE_MASTER); > Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d > > A simple change would have been to print error message instead of WARN_ON. > However, the callers assume that tty_set_termios() always returns 0 and > don't check return value. The complete solution is fixing all the callers > to check error and bail out to fix the WARN_ON. > > This fix changes tty_set_termios() to return error and all the callers > to check error and bail out. The reproducer is used to reproduce the > problem and verify the fix. > --- a/drivers/bluetooth/hci_ldisc.c > +++ b/drivers/bluetooth/hci_ldisc.c > @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) > status = tty_set_termios(tty, &ktermios); > BT_DBG("Disabling hardware flow control: %s", > status ? "failed" : "success"); > + if (status) > + return; Can that ldisc end up set on pty master? And does it make any sense there? > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > index fa1672993b4c..29b51370deac 100644 > --- a/drivers/tty/serdev/serdev-ttyport.c > +++ b/drivers/tty/serdev/serdev-ttyport.c > @@ -136,7 +136,9 @@ static int ttyport_open(struct serdev_controller *ctrl) > ktermios.c_cflag |= CRTSCTS; > /* Hangups are not supported so make sure to ignore carrier detect. */ > ktermios.c_cflag |= CLOCAL; > - tty_set_termios(tty, &ktermios); > + ret = tty_set_termios(tty, &ktermios); How can _that_ happen to pty master? > diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c > index 78fe622eba65..9978c21ce34d 100644 > --- a/net/nfc/nci/uart.c > +++ b/net/nfc/nci/uart.c > @@ -447,6 +447,7 @@ void nci_uart_set_config(struct nci_uart *nu, int baudrate, int flow_ctrl) > else > new_termios.c_cflag &= ~CRTSCTS; > > + /* FIXME tty_set_termios() could return error */ Ditto for this one. IOW, I don't believe that this patch makes any sense. If anything, we need to prevent unconditional tty_set_termios() on the path that *does* lead to calling it for pty.
