Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[Johan Hovold]

On Thu, Jan 31, 2019 at 04:23:59PM -0700, Shuah Khan wrote:
> tty_set_termios() has the following WARN_ON which can be triggered with a
> syscall to invoke TIOCSETD __NR_ioctl.

That's the only way to set the hci line discipline. And it's the
consequent ioctl that sets the uart protocol that triggers the warning,
but only if the tty is a pty master, as I mentioned before.

> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
>                 tty->driver->subtype == PTY_TYPE_MASTER);
> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
> 
> The problem started with commit 7721383f4199 ("Bluetooth: hci_uart: Support
> operational speed during setup") which introduced a new way for how
> tty_set_termios() could end up being called for a master pty.

Please always include reviewers on CC, and especially if you end up
citing them directly as you do here. Perhaps add quotation marks or at
least a reference to the discussion where this solution was suggested.

> Fix the problem by preventing setting the HCI line discipline for PTYs
> from hci_uart_setup() and hci_uart_set_flow_control().

This makes no sense. You've already set the line discpline, you're just
making the uart protocol ioctl fail when it tries to register the hci
device.

> The reproducer is used to reproduce the problem and verify the fix.
> 
> Reported-by: syzbot+a950165cbb86bdd023a4@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Shuah Khan <shuah@xxxxxxxxxx>
> ---
>  drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..ce84ca91ca70 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -314,6 +314,11 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>  		return;
>  	}
>  
> +	/* don't set HCI line discipline on PTYs */
> +	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> +	    tty->driver->subtype == PTY_TYPE_MASTER)
> +		return;

I don't think you can ever end up here with the below change.

> +
>  	if (enable) {
>  		/* Disable hardware flow control */
>  		ktermios = tty->termios;
> @@ -384,11 +389,17 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  static int hci_uart_setup(struct hci_dev *hdev)
>  {
>  	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct tty_struct *tty = hu->tty;
>  	struct hci_rp_read_local_version *ver;
>  	struct sk_buff *skb;
>  	unsigned int speed;
>  	int err;
>  
> +	/* don't set HCI line discipline on PTYs */
> +	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> +	    tty->driver->subtype == PTY_TYPE_MASTER)
> +		return -EINVAL;

This is too late for what your patch claim that you try to do. This
would fail the hci device registration when setting the uart protocol,
but the line discipline has already been set.

> +
>  	/* Init speed if any */
>  	if (hu->init_speed)
>  		speed = hu->init_speed;

Johan