bluetooth: Fix WARNING in tty_set_termios()

PATCH Superseded Shuah Khan 2 participants 3 messages Feb 3, 2019 – Feb 4, 2019 Bluetooth drivers

lore

Share & tools

lore

Shuah Khan

author

2019-02-03 19:38 UTC #0 lore raw

PATCH v4 1/1 bluetooth: Fix WARNING in tty_set_termios()

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

"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."

Fix it by by preventing setting the HCI line discipline for PTYs in hci_uart_tty_open().

Reported-by [email protected]

Cc Johan Hovold <[email protected]>

Cc Marcel Holtmann <[email protected]>

Cc Al Viro <[email protected]>

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/bluetooth/hci_ldisc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/bluetooth/hci_ldisc.c +2 -2

index fbf7b4df23ab..c8faa4759cb7 100644
@@ -475,9 +475,9 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	BT_DBG("tty %p", tty);
 
 	/* Error if the tty has no write op instead of leaving an exploitable
-	 * hole
+	 * hole. In addition check if setting HCI line discipline is allowed.
 	 */
-	if (tty->ops->write == NULL)
+	if (tty->ops->write == NULL || tty->ops->set_termios == NULL)
 		return -EOPNOTSUPP;
 
 	hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL);

Signature · 1 line

2.17.1

Marcel Holtmann

maintainer

2019-02-04 05:39 UTC #1 lore raw

Hi Shuah,

Quoted 28 lines

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
"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."
Fix it by by preventing setting the HCI line discipline for PTYs in hci_uart_tty_open().
Reported-by [email protected]
Cc Johan Hovold <[email protected]>
Cc Marcel Holtmann <[email protected]>
Cc Al Viro <[email protected]>
Signed-off-by Shuah Khan <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
drivers/bluetooth/hci_ldisc.c +0 -0
index fbf7b4df23ab..c8faa4759cb7 100644

drivers/bluetooth/hci_ldisc.c static int hci_uart_tty_open(struct tty_struct *tty) · line 479

Quoted 6 lines

drivers/bluetooth/hci_ldisc.c +1 -1

@@ -475,9 +475,9 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	BT_DBG("tty %p", tty);
 
 	/* Error if the tty has no write op instead of leaving an exploitable
-	 * hole
+	 * hole. In addition check if setting HCI line discipline is allowed.
 	 */

this comment is wrong. The result of setting the HCI line discipline is a call into hci_uart_tty_open. You have to check ops->write and ops->set_termios since both are required. That is it.

drivers/bluetooth/hci_ldisc.c static int hci_uart_tty_open(struct tty_struct *tty) · line 481

Quoted 3 lines

drivers/bluetooth/hci_ldisc.c +1 -1

-	if (tty->ops->write == NULL)
+	if (tty->ops->write == NULL || tty->ops->set_termios == NULL)
 		return -EOPNOTSUPP;

And while at it, change this to (!tty->ops->write).

Regards

Marcel

shuah

author

2019-02-04 18:45 UTC #2 lore raw

Quoted 32 lines · Marcel Holtmann

Hi Shuah,
Quoted 30 lines
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
"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."
Fix it by by preventing setting the HCI line discipline for PTYs in hci_uart_tty_open().
Reported-by [email protected]
Cc Johan Hovold <[email protected]>
Cc Marcel Holtmann <[email protected]>
Cc Al Viro <[email protected]>
Signed-off-by Shuah Khan <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
drivers/bluetooth/hci_ldisc.c +0 -0
index fbf7b4df23ab..c8faa4759cb7 100644
@@ -475,9 +475,9 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	BT_DBG("tty %p", tty);
 

Quoted 6 lines

drivers/bluetooth/hci_ldisc.c static int hci_uart_tty_open(struct tty_struct *tty) · line 479
Quoted 4 lines
drivers/bluetooth/hci_ldisc.c +1 -1
 	/* Error if the tty has no write op instead of leaving an exploitable
-	 * hole
+	 * hole. In addition check if setting HCI line discipline is allowed.
 	 */
this comment is wrong. The result of setting the HCI line discipline is a call into hci_uart_tty_open. You have to check ops->write and ops->set_termios since both are required. That is it.

I will fix the comment.

Quoted 7 lines

drivers/bluetooth/hci_ldisc.c static int hci_uart_tty_open(struct tty_struct *tty) · line 481
Quoted 4 lines
drivers/bluetooth/hci_ldisc.c +1 -1
-	if (tty->ops->write == NULL)
+	if (tty->ops->write == NULL || tty->ops->set_termios == NULL)
 		return -EOPNOTSUPP;
And while at it, change this to (!tty->ops->write).

Okay. I will fix both.

thanks,
-- Shuah