| Message ID | 20190125232905.21727-1-shuah@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | tty: Fix WARNING in tty_set_termios | expand |
On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote: > tty_set_termios() has the following WARMN_ON which can be triggered with a > syscall to invoke TIOCGETD __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 > > A simple change would have been to print error message instead of WARN_ON. > However, the callers assume that tty_set_termios() always returns 0 and > don't check return value. The complete solution is fixing all the callers > to check error and bail out to fix the WARN_ON. > > This fix changes tty_set_termios() to return error and all the callers > to check error and bail out. The reproducer is used to reproduce the > problem and verify the fix. > --- a/drivers/bluetooth/hci_ldisc.c > +++ b/drivers/bluetooth/hci_ldisc.c > @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) > status = tty_set_termios(tty, &ktermios); > BT_DBG("Disabling hardware flow control: %s", > status ? "failed" : "success"); > + if (status) > + return; Can that ldisc end up set on pty master? And does it make any sense there? > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > index fa1672993b4c..29b51370deac 100644 > --- a/drivers/tty/serdev/serdev-ttyport.c > +++ b/drivers/tty/serdev/serdev-ttyport.c > @@ -136,7 +136,9 @@ static int ttyport_open(struct serdev_controller *ctrl) > ktermios.c_cflag |= CRTSCTS; > /* Hangups are not supported so make sure to ignore carrier detect. */ > ktermios.c_cflag |= CLOCAL; > - tty_set_termios(tty, &ktermios); > + ret = tty_set_termios(tty, &ktermios); How can _that_ happen to pty master? > diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c > index 78fe622eba65..9978c21ce34d 100644 > --- a/net/nfc/nci/uart.c > +++ b/net/nfc/nci/uart.c > @@ -447,6 +447,7 @@ void nci_uart_set_config(struct nci_uart *nu, int baudrate, int flow_ctrl) > else > new_termios.c_cflag &= ~CRTSCTS; > > + /* FIXME tty_set_termios() could return error */ Ditto for this one. IOW, I don't believe that this patch makes any sense. If anything, we need to prevent unconditional tty_set_termios() on the path that *does* lead to calling it for pty.
On 1/25/19 9:14 PM, Al Viro wrote: > On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote: >> tty_set_termios() has the following WARMN_ON which can be triggered with a >> syscall to invoke TIOCGETD __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 >> >> A simple change would have been to print error message instead of WARN_ON. >> However, the callers assume that tty_set_termios() always returns 0 and >> don't check return value. The complete solution is fixing all the callers >> to check error and bail out to fix the WARN_ON. >> >> This fix changes tty_set_termios() to return error and all the callers >> to check error and bail out. The reproducer is used to reproduce the >> problem and verify the fix. > >> --- a/drivers/bluetooth/hci_ldisc.c >> +++ b/drivers/bluetooth/hci_ldisc.c >> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) >> status = tty_set_termios(tty, &ktermios); >> BT_DBG("Disabling hardware flow control: %s", >> status ? "failed" : "success"); >> + if (status) >> + return; > > Can that ldisc end up set on pty master? And does it make any sense there? The initial objective of the patch is to prevent the WARN_ON by making the change to return error instead of WARN_ON. However, without changes to places that don't check the return and keep making progress, there will be secondary problems. Without this change to return here, instead of WARN_ON, it will fail with the following NULL pointer dereference at the next thing hci_uart_set_flow_control() attempts. status = tty->driver->ops->tiocmget(tty); kernel: [10140.649783] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000 kernel: [10140.649786] #PF error: [INSTR] kernel: [10140.649787] PGD 0 P4D 0 kernel: [10140.649790] Oops: 0010 [#1] SMP PTI Jan 24 15:33:35 deneb kernel: [10140.649793] CPU: 2 PID: 55 Comm: kworker/u33:0 Tainted: G W 5.0.0-rc3+ #5 kernel: [10140.649794] Hardware name: Dell Inc. OptiPlex 790/0HY9JP, BIOS A18 09/24/2013 Workqueue: hci0 hci_power_on [bluetooth] kernel: [10140.649805] RIP: 0010: (null) kernel: [10140.649809] Code: Bad RIP value. kernel: [10140.649810] RSP: 0018:ffffa01a8153fd28 EFLAGS: 00010282 kernel: [10140.649812] RAX: 0000000000000000 RBX: ffff8958d6bc4800 RCX: 35ad8b0300000000 kernel: [10140.649814] RDX: ffffffff00000001 RSI: 0000000000000000 RDI: ffff8958d6bc4800 kernel: [10140.649816] RBP: ffffa01a8153fd78 R08: 0000000091773f09 R09: 0000000000000003 kernel: [10140.649817] R10: ffff8958d6bc4a98 R11: 0000000000000720 R12: ffff895814500c00 kernel: [10140.649819] R13: ffff8958a858e000 R14: 0000000000000000 R15: ffff8958af1af440 kernel: [10140.649821] FS: 0000000000000000(0000) GS:ffff895925880000(0000) knlGS:0000000000000000 kernel: [10140.649823] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 kernel: [10140.649824] CR2: ffffffffffffffd6 CR3: 0000000083f46002 CR4: 00000000000606e0 kernel: [10140.649826] Call Trace: kernel: [10140.649830] ? hci_uart_set_flow_control+0x20e/0x2c0 [hci_uart] kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart] kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart] kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth] kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth] > > IOW, I don't believe that this patch makes any sense. If anything, > we need to prevent unconditional tty_set_termios() on the path > that *does* lead to calling it for pty. > I don't think preventing unconditional tty_set_termios() is enough to prevent secondary problems such as the one above. For example, the following call chain leads to the WARN_ON that was reported. Even if void hci_uart_set_baudrate() prevents the very first tty_set_termios() call, its caller hci_uart_setup() continues with more tty setup. It goes ahead to call driver setup callback. The driver callback goes on to do more setup calling tty_set_termios(). WARN_ON call path: hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378 hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401 hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423 Once this WARN_ON is changed to return error, the following happens, when hci_uart_setup() does driver setup callback. kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart] kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart] kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth] kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth] I think continuing to catch the invalid condition in tty_set_termios() and preventing progress by checking return value is a straight forward change to avoid secondary problems, and it might be difficult to catch all the cases where it could fail. Here is the reproducer for reference: #define _GNU_SOURCE #include <endian.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/syscall.h> #include <sys/types.h> #include <unistd.h> uint64_t r[1] = {0xffffffffffffffff}; int main(void) { syscall(__NR_mmap, 0x20000000, 0x1000000, 3, 0x32, -1, 0); long res = 0; memcpy((void*)0x20000100, "/dev/ptmx\x00", 10); res = syscall(__NR_openat, 0xffffffffffffff9c, 0x20000100, 0, 0); if (res != -1) r[0] = res; *(uint32_t*)0x200000c0 = 0xf; syscall(__NR_ioctl, r[0], 0x5423, 0x200000c0); syscall(__NR_ioctl, r[0], 0x400455c8, 0xb); return 0; } thanks, -- Shuah
On Mon, Jan 28, 2019 at 02:29:22PM -0700, shuah wrote: > On 1/25/19 9:14 PM, Al Viro wrote: > > On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote: > >> tty_set_termios() has the following WARMN_ON which can be triggered with a > >> syscall to invoke TIOCGETD __NR_ioctl. You meant TIOCSETD here, and in fact its the call which sets the uart protocol that triggers the warning. > >> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY && > >> tty->driver->subtype == PTY_TYPE_MASTER); > >> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d > >> > >> A simple change would have been to print error message instead of WARN_ON. > >> However, the callers assume that tty_set_termios() always returns 0 and > >> don't check return value. The complete solution is fixing all the callers > >> to check error and bail out to fix the WARN_ON. > >> > >> This fix changes tty_set_termios() to return error and all the callers > >> to check error and bail out. The reproducer is used to reproduce the > >> problem and verify the fix. > > > >> --- a/drivers/bluetooth/hci_ldisc.c > >> +++ b/drivers/bluetooth/hci_ldisc.c > >> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) > >> status = tty_set_termios(tty, &ktermios); > >> BT_DBG("Disabling hardware flow control: %s", > >> status ? "failed" : "success"); > >> + if (status) > >> + return; > > > > Can that ldisc end up set on pty master? And does it make any sense there? > > The initial objective of the patch is to prevent the WARN_ON by making > the change to return error instead of WARN_ON. However, without changes > to places that don't check the return and keep making progress, there > will be secondary problems. > > Without this change to return here, instead of WARN_ON, it will fail > with the following NULL pointer dereference at the next thing > hci_uart_set_flow_control() attempts. > > status = tty->driver->ops->tiocmget(tty); > > kernel: [10140.649783] BUG: unable to handle kernel NULL pointer That's a separate issue, which is being fixed: https://lkml.kernel.org/r/20190130095938.GP3691@localhost > > IOW, I don't believe that this patch makes any sense. If anything, > > we need to prevent unconditional tty_set_termios() on the path > > that *does* lead to calling it for pty. > > I don't think preventing unconditional tty_set_termios() is enough to > prevent secondary problems such as the one above. > > For example, the following call chain leads to the WARN_ON that was > reported. Even if void hci_uart_set_baudrate() prevents the very first > tty_set_termios() call, its caller hci_uart_setup() continues with > more tty setup. It goes ahead to call driver setup callback. The > driver callback goes on to do more setup calling tty_set_termios(). > > WARN_ON call path: > hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378 > hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401 > hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423 > > Once this WARN_ON is changed to return error, the following > happens, when hci_uart_setup() does driver setup callback. > > kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart] > kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart] > kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth] > kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth] > > I think continuing to catch the invalid condition in tty_set_termios() > and preventing progress by checking return value is a straight forward > change to avoid secondary problems, and it might be difficult to catch > all the cases where it could fail. I agree with Al that this change doesn't make much sense. The WARN_ON is there to catch any bugs leading to the termios being changed for a master side pty. Those should bugs should be fixed, and not worked around in order to silence a WARN_ON. The problem started with 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. As Al hinted at, setting these ldiscs for a master pty really makes no sense and perhaps that is what we should prevent unless simply making sure they do not call tty_set_termios() is sufficient for the time being. Finally, note that serdev never operates on a pty, and that this is only an issue for (the three) line disciplines. Johan
On 1/30/19 3:32 AM, Johan Hovold wrote: > On Mon, Jan 28, 2019 at 02:29:22PM -0700, shuah wrote: >> On 1/25/19 9:14 PM, Al Viro wrote: >>> On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote: >>>> tty_set_termios() has the following WARMN_ON which can be triggered with a >>>> syscall to invoke TIOCGETD __NR_ioctl. > > You meant TIOCSETD here, and in fact its the call which sets the uart > protocol that triggers the warning. Right. It is a TIOCSETD. > >>>> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY && >>>> tty->driver->subtype == PTY_TYPE_MASTER); >>>> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d >>>> >>>> A simple change would have been to print error message instead of WARN_ON. >>>> However, the callers assume that tty_set_termios() always returns 0 and >>>> don't check return value. The complete solution is fixing all the callers >>>> to check error and bail out to fix the WARN_ON. >>>> >>>> This fix changes tty_set_termios() to return error and all the callers >>>> to check error and bail out. The reproducer is used to reproduce the >>>> problem and verify the fix. >>> >>>> --- a/drivers/bluetooth/hci_ldisc.c >>>> +++ b/drivers/bluetooth/hci_ldisc.c >>>> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) >>>> status = tty_set_termios(tty, &ktermios); >>>> BT_DBG("Disabling hardware flow control: %s", >>>> status ? "failed" : "success"); >>>> + if (status) >>>> + return; >>> >>> Can that ldisc end up set on pty master? And does it make any sense there? >> >> The initial objective of the patch is to prevent the WARN_ON by making >> the change to return error instead of WARN_ON. However, without changes >> to places that don't check the return and keep making progress, there >> will be secondary problems. >> >> Without this change to return here, instead of WARN_ON, it will fail >> with the following NULL pointer dereference at the next thing >> hci_uart_set_flow_control() attempts. >> >> status = tty->driver->ops->tiocmget(tty); >> >> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer > > That's a separate issue, which is being fixed: > > https://lkml.kernel.org/r/20190130095938.GP3691@localhost > Ah good to know. >>> IOW, I don't believe that this patch makes any sense. If anything, >>> we need to prevent unconditional tty_set_termios() on the path >>> that *does* lead to calling it for pty. >> >> I don't think preventing unconditional tty_set_termios() is enough to >> prevent secondary problems such as the one above. >> >> For example, the following call chain leads to the WARN_ON that was >> reported. Even if void hci_uart_set_baudrate() prevents the very first >> tty_set_termios() call, its caller hci_uart_setup() continues with >> more tty setup. It goes ahead to call driver setup callback. The >> driver callback goes on to do more setup calling tty_set_termios(). >> >> WARN_ON call path: >> hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378 >> hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401 >> hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423 >> >> Once this WARN_ON is changed to return error, the following >> happens, when hci_uart_setup() does driver setup callback. >> >> kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart] >> kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart] >> kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth] >> kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth] >> >> I think continuing to catch the invalid condition in tty_set_termios() >> and preventing progress by checking return value is a straight forward >> change to avoid secondary problems, and it might be difficult to catch >> all the cases where it could fail. > > I agree with Al that this change doesn't make much sense. The WARN_ON > is there to catch any bugs leading to the termios being changed for a > master side pty. Those should bugs should be fixed, and not worked > around in order to silence a WARN_ON. > > The problem started with 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. > Ah. Thanks for the context. > As Al hinted at, setting these ldiscs for a master pty really makes no > sense and perhaps that is what we should prevent unless simply making > sure they do not call tty_set_termios() is sufficient for the time > being. > I will take a look to see if not calling tty_set_termios() is enough. > Finally, note that serdev never operates on a pty, and that this is only > an issue for (the three) line disciplines. > Thanks for the detailed message explaining the evolution. Now it makes sense. -- Shuah
Hi Johan, >>> On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote: >>>> tty_set_termios() has the following WARMN_ON which can be triggered with a >>>> syscall to invoke TIOCGETD __NR_ioctl. > > You meant TIOCSETD here, and in fact its the call which sets the uart > protocol that triggers the warning. > >>>> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY && >>>> tty->driver->subtype == PTY_TYPE_MASTER); >>>> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d >>>> >>>> A simple change would have been to print error message instead of WARN_ON. >>>> However, the callers assume that tty_set_termios() always returns 0 and >>>> don't check return value. The complete solution is fixing all the callers >>>> to check error and bail out to fix the WARN_ON. >>>> >>>> This fix changes tty_set_termios() to return error and all the callers >>>> to check error and bail out. The reproducer is used to reproduce the >>>> problem and verify the fix. >>> >>>> --- a/drivers/bluetooth/hci_ldisc.c >>>> +++ b/drivers/bluetooth/hci_ldisc.c >>>> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) >>>> status = tty_set_termios(tty, &ktermios); >>>> BT_DBG("Disabling hardware flow control: %s", >>>> status ? "failed" : "success"); >>>> + if (status) >>>> + return; >>> >>> Can that ldisc end up set on pty master? And does it make any sense there? >> >> The initial objective of the patch is to prevent the WARN_ON by making >> the change to return error instead of WARN_ON. However, without changes >> to places that don't check the return and keep making progress, there >> will be secondary problems. >> >> Without this change to return here, instead of WARN_ON, it will fail >> with the following NULL pointer dereference at the next thing >> hci_uart_set_flow_control() attempts. >> >> status = tty->driver->ops->tiocmget(tty); >> >> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer > > That's a separate issue, which is being fixed: > > https://lkml.kernel.org/r/20190130095938.GP3691@localhost > >>> IOW, I don't believe that this patch makes any sense. If anything, >>> we need to prevent unconditional tty_set_termios() on the path >>> that *does* lead to calling it for pty. >> >> I don't think preventing unconditional tty_set_termios() is enough to >> prevent secondary problems such as the one above. >> >> For example, the following call chain leads to the WARN_ON that was >> reported. Even if void hci_uart_set_baudrate() prevents the very first >> tty_set_termios() call, its caller hci_uart_setup() continues with >> more tty setup. It goes ahead to call driver setup callback. The >> driver callback goes on to do more setup calling tty_set_termios(). >> >> WARN_ON call path: >> hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378 >> hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401 >> hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423 >> >> Once this WARN_ON is changed to return error, the following >> happens, when hci_uart_setup() does driver setup callback. >> >> kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart] >> kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart] >> kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth] >> kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth] >> >> I think continuing to catch the invalid condition in tty_set_termios() >> and preventing progress by checking return value is a straight forward >> change to avoid secondary problems, and it might be difficult to catch >> all the cases where it could fail. > > I agree with Al that this change doesn't make much sense. The WARN_ON > is there to catch any bugs leading to the termios being changed for a > master side pty. Those should bugs should be fixed, and not worked > around in order to silence a WARN_ON. > > The problem started with 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. > > As Al hinted at, setting these ldiscs for a master pty really makes no > sense and perhaps that is what we should prevent unless simply making > sure they do not call tty_set_termios() is sufficient for the time > being. > > Finally, note that serdev never operates on a pty, and that this is only > an issue for (the three) line disciplines. I think for PTYs we should just fail setting the HCI line discipline. Fail early and just move on with life. Regards Marcel
On Thu, Jan 31, 2019 at 04:18:33PM +0100, Marcel Holtmann wrote: > > I agree with Al that this change doesn't make much sense. The WARN_ON > > is there to catch any bugs leading to the termios being changed for a > > master side pty. Those should bugs should be fixed, and not worked > > around in order to silence a WARN_ON. > > > > The problem started with 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. > > > > As Al hinted at, setting these ldiscs for a master pty really makes no > > sense and perhaps that is what we should prevent unless simply making > > sure they do not call tty_set_termios() is sufficient for the time > > being. > > > > Finally, note that serdev never operates on a pty, and that this is only > > an issue for (the three) line disciplines. > > I think for PTYs we should just fail setting the HCI line discipline. > Fail early and just move on with life. Sounds good to me. At least for the pty master. There may be some people trying to use a bluetooth device connected to a remote serial port (I've seen descriptions of such setups at least), and maybe we need not prevent that. Johan
On 1/31/19 8:33 AM, Johan Hovold wrote: > On Thu, Jan 31, 2019 at 04:18:33PM +0100, Marcel Holtmann wrote: > >>> I agree with Al that this change doesn't make much sense. The WARN_ON >>> is there to catch any bugs leading to the termios being changed for a >>> master side pty. Those should bugs should be fixed, and not worked >>> around in order to silence a WARN_ON. >>> >>> The problem started with 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. >>> >>> As Al hinted at, setting these ldiscs for a master pty really makes no >>> sense and perhaps that is what we should prevent unless simply making >>> sure they do not call tty_set_termios() is sufficient for the time >>> being. >>> >>> Finally, note that serdev never operates on a pty, and that this is only >>> an issue for (the three) line disciplines. >> >> I think for PTYs we should just fail setting the HCI line discipline. >> Fail early and just move on with life. > > Sounds good to me. At least for the pty master. There may be some people > trying to use a bluetooth device connected to a remote serial port (I've > seen descriptions of such setups at least), and maybe we need not prevent > that. > Thanks for the feedback on the patch. Changes to prevent setting the HCI line discipline from hci_uart fixes the problem. I am sending v2 in a just a bit. thanks, -- Shuah
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index fbf7b4df23ab..643c4c75f86d 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable) status = tty_set_termios(tty, &ktermios); BT_DBG("Disabling hardware flow control: %s", status ? "failed" : "success"); + if (status) + return; /* Clear RTS to prevent the device from sending */ /* Most UARTs need OUT2 to enable interrupts */ @@ -369,13 +371,15 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed) { struct tty_struct *tty = hu->tty; struct ktermios ktermios; + int ret; ktermios = tty->termios; ktermios.c_cflag &= ~CBAUD; tty_termios_encode_baud_rate(&ktermios, speed, speed); - /* tty_set_termios() return not checked as it is always 0 */ - tty_set_termios(tty, &ktermios); + ret = tty_set_termios(tty, &ktermios); + if (ret) + return; BT_DBG("%s: New tty speeds: %d/%d", hu->hdev->name, tty->termios.c_ispeed, tty->termios.c_ospeed); diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c index c92bbd05516e..ded6f8089fc8 100644 --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -165,7 +165,9 @@ static int spk_ttyio_initialise_ldisc(struct spk_synth *synth) get_termios(tty, &tmp_termios); if (!(tmp_termios.c_cflag & CRTSCTS)) { tmp_termios.c_cflag |= CRTSCTS; - tty_set_termios(tty, &tmp_termios); + ret = tty_set_termios(tty, &tmp_termios); + if (ret) + return ret; /* * check c_cflag to see if it's updated as tty_set_termios may not return * error even when no tty bits are changed by the request. diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c index fa1672993b4c..29b51370deac 100644 --- a/drivers/tty/serdev/serdev-ttyport.c +++ b/drivers/tty/serdev/serdev-ttyport.c @@ -136,7 +136,9 @@ static int ttyport_open(struct serdev_controller *ctrl) ktermios.c_cflag |= CRTSCTS; /* Hangups are not supported so make sure to ignore carrier detect. */ ktermios.c_cflag |= CLOCAL; - tty_set_termios(tty, &ktermios); + ret = tty_set_termios(tty, &ktermios); + if (ret) + return ret; set_bit(SERPORT_ACTIVE, &serport->flags); @@ -171,12 +173,14 @@ static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigne struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; struct ktermios ktermios = tty->termios; + int retval; ktermios.c_cflag &= ~CBAUD; tty_termios_encode_baud_rate(&ktermios, speed, speed); - /* tty_set_termios() return not checked as it is always 0 */ - tty_set_termios(tty, &ktermios); + retval = tty_set_termios(tty, &ktermios); + if (retval) + return retval; return ktermios.c_ospeed; } @@ -185,13 +189,16 @@ static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; struct ktermios ktermios = tty->termios; + int retval; if (enable) ktermios.c_cflag |= CRTSCTS; else ktermios.c_cflag &= ~CRTSCTS; - tty_set_termios(tty, &ktermios); + retval = tty_set_termios(tty, &ktermios); + if (retval) + return; } static int ttyport_set_parity(struct serdev_controller *ctrl, @@ -200,6 +207,7 @@ static int ttyport_set_parity(struct serdev_controller *ctrl, struct serport *serport = serdev_controller_get_drvdata(ctrl); struct tty_struct *tty = serport->tty; struct ktermios ktermios = tty->termios; + int retval; ktermios.c_cflag &= ~(PARENB | PARODD | CMSPAR); if (parity != SERDEV_PARITY_NONE) { @@ -208,7 +216,9 @@ static int ttyport_set_parity(struct serdev_controller *ctrl, ktermios.c_cflag |= PARODD; } - tty_set_termios(tty, &ktermios); + retval = tty_set_termios(tty, &ktermios); + if (retval) + return retval; if ((tty->termios.c_cflag & (PARENB | PARODD | CMSPAR)) != (ktermios.c_cflag & (PARENB | PARODD | CMSPAR))) diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c index 9245fffdbceb..93e6531573ad 100644 --- a/drivers/tty/tty_ioctl.c +++ b/drivers/tty/tty_ioctl.c @@ -316,8 +316,9 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios) struct ktermios old_termios; struct tty_ldisc *ld; - WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY && - tty->driver->subtype == PTY_TYPE_MASTER); + if (tty->driver->type == TTY_DRIVER_TYPE_PTY && + tty->driver->subtype == PTY_TYPE_MASTER) + return -EINVAL; /* * Perform the actual termios internal changes under lock. */ @@ -411,7 +412,9 @@ static int set_termios(struct tty_struct *tty, void __user *arg, int opt) return -ERESTARTSYS; } - tty_set_termios(tty, &tmp_termios); + retval = tty_set_termios(tty, &tmp_termios); + if (retval) + return retval; /* FIXME: Arguably if tmp_termios == tty->termios AND the actual requested termios was not tmp_termios then we may @@ -588,7 +591,10 @@ static int set_sgttyb(struct tty_struct *tty, struct sgttyb __user *sgttyb) termios.c_ospeed); #endif up_write(&tty->termios_rwsem); - tty_set_termios(tty, &termios); + retval = tty_set_termios(tty, &termios); + if (retval) + return retval; + return 0; } #endif diff --git a/net/nfc/nci/uart.c b/net/nfc/nci/uart.c index 78fe622eba65..9978c21ce34d 100644 --- a/net/nfc/nci/uart.c +++ b/net/nfc/nci/uart.c @@ -447,6 +447,7 @@ void nci_uart_set_config(struct nci_uart *nu, int baudrate, int flow_ctrl) else new_termios.c_cflag &= ~CRTSCTS; + /* FIXME tty_set_termios() could return error */ tty_set_termios(nu->tty, &new_termios); } EXPORT_SYMBOL_GPL(nci_uart_set_config);
tty_set_termios() has the following WARMN_ON which can be triggered with a syscall to invoke TIOCGETD __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 A simple change would have been to print error message instead of WARN_ON. However, the callers assume that tty_set_termios() always returns 0 and don't check return value. The complete solution is fixing all the callers to check error and bail out to fix the WARN_ON. This fix changes tty_set_termios() to return error and all the callers to check error and bail out. 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 | 8 ++++++-- drivers/staging/speakup/spk_ttyio.c | 4 +++- drivers/tty/serdev/serdev-ttyport.c | 20 +++++++++++++++----- drivers/tty/tty_ioctl.c | 14 ++++++++++---- net/nfc/nci/uart.c | 1 + 5 files changed, 35 insertions(+), 12 deletions(-)