netdev - Re: [PATCH] tty: Fix WARNING in tty_set

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 26 Jan 2019 04:14:16 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Shuah Khan <shuah@...nel.org>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com, w.d.hubbs@...il.com,
        chris@...-brannons.com, kirk@...sers.ca,
        samuel.thibault@...-lyon.org, gregkh@...uxfoundation.org,
        robh@...nel.org, jslaby@...e.com, sameo@...ux.intel.com,
        davem@...emloft.net, arnd@...db.de,
        nishka.dasgupta_ug18@...oka.edu.in, m.maya.nakamura@...il.com,
        santhameena13@...il.com, zhongjiang@...wei.com,
        linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
        speakup@...ux-speakup.org, devel@...verdev.osuosl.org,
        linux-serial@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] tty: Fix WARNING in tty_set_termios

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.