tty: Fix WARNING in tty_set_termios()
PATCH v2 1/1 tty: 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 the problem by preventing setting the HCI line discipline for PTYs from hci_uart_setup() and hci_uart_set_flow_control().
The reproducer is used to reproduce the problem and verify the fix.
--- drivers/bluetooth/hci_ldisc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/bluetooth/hci_ldisc.c +11 -0
@@ -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 ; +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 ; +/* Init speed if any */ if (hu->init_speed) speed = hu->init_speed;
Signature · 1 line
2.17.1
Quoted 14 lines · Shuah Khan
tty_set_termios()has the followingWARN_ONwhich 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 howtty_set_termios()could end up being called for a master pty.Fix the problem by preventing setting the HCI line discipline for PTYs from
hci_uart_setup()andhci_uart_set_flow_control().
Quoted 7 lines
The reproducer is used to reproduce the problem and verify the fix.
Reported-by [email protected]Signed-off-by Shuah Khan <[email protected]>--- drivers/bluetooth/hci_ldisc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
I think the subject should be something like:
"bluetooth: hci: Fix warning in tty_set_termios()"
it isn't a tty core problem :)
thanks,
greg k-h
Quoted 23 lines · Greg KH
Quoted 14 lines · Shuah Khan
tty_set_termios()has the followingWARN_ONwhich 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 howtty_set_termios()could end up being called for a master pty.Fix the problem by preventing setting the HCI line discipline for PTYs from
hci_uart_setup()andhci_uart_set_flow_control().Quoted 7 lines
The reproducer is used to reproduce the problem and verify the fix.
Reported-by [email protected]Signed-off-by Shuah Khan <[email protected]>--- drivers/bluetooth/hci_ldisc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
Quoted 4 lines
I think the subject should be something like:
"bluetooth: hci: Fix warning intty_set_termios()"
it isn't a tty core problem :)
Yes. Sorry about that. Will get it right in my v3.
thanks,
-- Shuah
Quoted 2 lines · Shuah Khan
tty_set_termios()has the followingWARN_ONwhich 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.
Quoted 7 lines
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 howtty_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.
Quoted 2 lines
Fix the problem by preventing setting the HCI line discipline for PTYs from
hci_uart_setup()andhci_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.
Quoted 13 lines
The reproducer is used to reproduce the problem and verify the fix.
Reported-by [email protected]Signed-off-by Shuah Khan <[email protected]>--- drivers/bluetooth/hci_ldisc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)drivers/bluetooth/hci_ldisc.c +0
-0
drivers/bluetooth/hci_ldisc.c void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) · line 320
Quoted 7 lines
drivers/bluetooth/hci_ldisc.c +4
-0@@ -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.
Quoted 8 lines
drivers/bluetooth/hci_ldisc.c +1
-0+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);
drivers/bluetooth/hci_ldisc.c void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed) · line 401
Quoted 10 lines
drivers/bluetooth/hci_ldisc.c +5
-0+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.
drivers/bluetooth/hci_ldisc.c void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed) · line 405
Quoted 4 lines
drivers/bluetooth/hci_ldisc.c +1
-0+/* Init speed if any */ if (hu->init_speed) speed = hu->init_speed;
Johan
Quoted 6 lines · Johan Hovold
Quoted 2 lines · Shuah Khan
tty_set_termios()has the followingWARN_ONwhich 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,
I meant *subsequent* ioctl, of course.
Quoted 1 line
but only if the tty is a pty master, as I mentioned before.
Johan
Quoted 12 lines · Johan Hovold
Quoted 2 lines · Shuah Khan
tty_set_termios()has the followingWARN_ONwhich 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.
Quoted 4 lines
WARN_ON(tty->driver->type ==
TTY_DRIVER_TYPE_PTY&&tty->driver->subtype == PTY_TYPE_MASTER);Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
Quoted 8 lines
Quoted 3 lines
The problem started with commit
7721383f4199("Bluetooth:hci_uart: Support operational speed during setup") which introduced a new way for howtty_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.
Thanks for the feedback. I am folding in your and Marcel's input in my v3.
thanks,
-- Shuah
Marcel Holtmann
maintainerHi Shuah,
Quoted 27 lines
tty_set_termios()has the followingWARN_ONwhich 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 howtty_set_termios()could end up being called for a master pty.Fix the problem by preventing setting the HCI line discipline for PTYs from
hci_uart_setup()andhci_uart_set_flow_control().The reproducer is used to reproduce the problem and verify the fix.
Reported-by [email protected]Signed-off-by Shuah Khan <[email protected]>---
drivers/bluetooth/hci_ldisc.c| 11 +++++++++++1 file changed, 11 insertions(+)drivers/bluetooth/hci_ldisc.c +0
-0
drivers/bluetooth/hci_ldisc.c void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) · line 321
Quoted 8 lines
drivers/bluetooth/hci_ldisc.c +5
-0@@ -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 ; +
just do it once in hci_uart_tty_open. Actually we already check for ops->write and so lets just ensure all ops we ever call are present. If they are not, then bail out. I wouldn’t even bother with the TTY type. Wouldn’t that be a lot simpler?
Regards
Marcel
Quoted 32 lines · Marcel Holtmann
Hi Shuah,
Quoted 30 lines
tty_set_termios()has the followingWARN_ONwhich 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 howtty_set_termios()could end up being called for a master pty.Fix the problem by preventing setting the HCI line discipline for PTYs from
hci_uart_setup()andhci_uart_set_flow_control().The reproducer is used to reproduce the problem and verify the fix.
Reported-by [email protected]Signed-off-by Shuah Khan <[email protected]>---
drivers/bluetooth/hci_ldisc.c| 11 +++++++++++1 file changed, 11 insertions(+)drivers/bluetooth/hci_ldisc.c +0
-0@@ -314,6 +314,11 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)return ; }
Quoted 8 lines
drivers/bluetooth/hci_ldisc.c void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) · line 321
Quoted 5 lines
drivers/bluetooth/hci_ldisc.c +5
-0+/* don't set HCI line discipline on PTYs */ +if (tty->driver->type ==TTY_DRIVER_TYPE_PTY && + tty->driver->subtype ==PTY_TYPE_MASTER ) +return ; +just do it once in
hci_uart_tty_open. Actually we already check forops->writeand so lets just ensure all ops we ever call are present. If they are not, then bail out. I wouldn’t even bother with the TTY type. Wouldn’t that be a lot simpler?
As you said, hci_uart_tty_open()is a good place to check it. I think checking tty type is necessary. I couldn't find any missing ops I could base the check on to prevent the ldisc set.
I have the patch that does the tty check in hci_uart_tty_open() tested and ready for sending.
thanks,
-- Shuah