Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] bluetooth: Fix WARNING in tty_set_termios()
@ 2019-02-03 16:48 Shuah Khan
  2019-02-03 17:31 ` Marcel Holtmann
  0 siblings, 1 reply; 3+ messages in thread
From: Shuah Khan @ 2019-02-03 16:48 UTC (permalink / raw)
  To: marcel, johan.hedberg, johan, viro
  Cc: Shuah Khan, linux-bluetooth, linux-kernel

tty_set_termios() has the following WARN_ON which can be triggered with a
syscall to invoke TIOCSETD __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

Johan Hovold said: "The problemm 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."

Fix it by by preventing setting the HCI line discipline for PTYs in
hci_uart_tty_open(). Looked into keying off of tty and ldisc ops, and
couldn't find any that would be conclusive. Checking tty as such clearly
tags the reason for rejecting the request to set ldisc.

Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
Cc: Johan Hovold <johan@kernel.org>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Shuah Khan <shuah@kernel.org>
---
 drivers/bluetooth/hci_ldisc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..a3d313fcc0f2 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -480,6 +480,11 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	if (tty->ops->write == NULL)
 		return -EOPNOTSUPP;
 
+	/* don't set HCI line discipline on PTYs */
+	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+	    tty->driver->subtype == PTY_TYPE_MASTER)
+		return -EINVAL;
+
 	hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL);
 	if (!hu) {
 		BT_ERR("Can't allocate control structure");
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] bluetooth: Fix WARNING in tty_set_termios()
  2019-02-03 16:48 [PATCH v3] bluetooth: Fix WARNING in tty_set_termios() Shuah Khan
@ 2019-02-03 17:31 ` Marcel Holtmann
  2019-02-03 19:36   ` shuah
  0 siblings, 1 reply; 3+ messages in thread
From: Marcel Holtmann @ 2019-02-03 17:31 UTC (permalink / raw)
  To: Shuah Khan; +Cc: Johan Hedberg, johan, viro, linux-bluetooth, linux-kernel

Hi Shuah,

> tty_set_termios() has the following WARN_ON which can be triggered with a
> syscall to invoke TIOCSETD __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
> 
> Johan Hovold said: "The problemm 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."
> 
> Fix it by by preventing setting the HCI line discipline for PTYs in
> hci_uart_tty_open(). Looked into keying off of tty and ldisc ops, and
> couldn't find any that would be conclusive. Checking tty as such clearly
> tags the reason for rejecting the request to set ldisc.
> 
> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
> drivers/bluetooth/hci_ldisc.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..a3d313fcc0f2 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -480,6 +480,11 @@ static int hci_uart_tty_open(struct tty_struct *tty)
> 	if (tty->ops->write == NULL)
> 		return -EOPNOTSUPP;
> 
> +	/* 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 turning in circles. What is wrong with checking !tty->ops->set_termios here?

Regards

Marcel


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] bluetooth: Fix WARNING in tty_set_termios()
  2019-02-03 17:31 ` Marcel Holtmann
@ 2019-02-03 19:36   ` shuah
  0 siblings, 0 replies; 3+ messages in thread
From: shuah @ 2019-02-03 19:36 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, johan, viro, linux-bluetooth, linux-kernel, shuah

On 2/3/19 10:31 AM, Marcel Holtmann wrote:
> Hi Shuah,
> 
>> tty_set_termios() has the following WARN_ON which can be triggered with a
>> syscall to invoke TIOCSETD __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
>>
>> Johan Hovold said: "The problemm 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."
>>
>> Fix it by by preventing setting the HCI line discipline for PTYs in
>> hci_uart_tty_open(). Looked into keying off of tty and ldisc ops, and
>> couldn't find any that would be conclusive. Checking tty as such clearly
>> tags the reason for rejecting the request to set ldisc.
>>
>> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
>> Cc: Johan Hovold <johan@kernel.org>
>> Cc: Marcel Holtmann <marcel@holtmann.org>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index fbf7b4df23ab..a3d313fcc0f2 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -480,6 +480,11 @@ static int hci_uart_tty_open(struct tty_struct *tty)
>> 	if (tty->ops->write == NULL)
>> 		return -EOPNOTSUPP;
>>
>> +	/* 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 turning in circles. What is wrong with checking !tty->ops->set_termios here?

Yeah. I looked into set_termios and thought that it is set in this path.
My bad. okay v4 is on its way. Sorry it took so long to get on the same
page with you. :(

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-03 16:48 [PATCH v3] bluetooth: Fix WARNING in tty_set_termios() Shuah Khan
2019-02-03 17:31 ` Marcel Holtmann
2019-02-03 19:36   ` shuah

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