Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Shuah Khan <shuah@kernel.org>
Cc: marcel@holtmann.org, johan.hedberg@gmail.com,
	viro@zeniv.linux.org.uk, gregkh@linuxfoundation.org,
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
Date: Fri, 1 Feb 2019 10:28:43 +0100
Message-ID: <20190201092843.GA3691@localhost> (raw)
In-Reply-To: <20190131232359.27948-1-shuah@kernel.org>

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@syzkaller.appspotmail.com
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
>  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

  parent reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-31 23:23 Shuah Khan
2019-02-01  0:14 ` Greg KH
2019-02-01 23:21   ` shuah
2019-02-01  9:28 ` Johan Hovold [this message]
2019-02-01  9:36   ` Johan Hovold
2019-02-01 23:20   ` shuah
2019-02-01 10:00 ` Marcel Holtmann
2019-02-01 23:18   ` shuah

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190201092843.GA3691@localhost \
    --to=johan@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=johan.hedberg@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcel@holtmann.org \
    --cc=shuah@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox