* [PATCH] tty: Fix WARNING in tty_set_termios
@ 2019-01-25 23:29 Shuah Khan
2019-01-26 4:14 ` Al Viro
0 siblings, 1 reply; 8+ messages in thread
From: Shuah Khan @ 2019-01-25 23:29 UTC (permalink / raw)
To: marcel, johan.hedberg, w.d.hubbs, chris, kirk, samuel.thibault,
gregkh, robh, jslaby, sameo, davem, arnd, nishka.dasgupta_ug18,
m.maya.nakamura, santhameena13, shuah, zhongjiang, viro
Cc: linux-bluetooth, linux-kernel, speakup, devel, linux-serial,
linux-wireless, netdev
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(-)
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-25 23:29 [PATCH] tty: Fix WARNING in tty_set_termios Shuah Khan
@ 2019-01-26 4:14 ` Al Viro
2019-01-28 21:29 ` shuah
0 siblings, 1 reply; 8+ messages in thread
From: Al Viro @ 2019-01-26 4:14 UTC (permalink / raw)
To: Shuah Khan
Cc: marcel, johan.hedberg, w.d.hubbs, chris, kirk, samuel.thibault,
gregkh, robh, jslaby, sameo, davem, arnd, nishka.dasgupta_ug18,
m.maya.nakamura, santhameena13, zhongjiang, linux-bluetooth,
linux-kernel, speakup, devel, linux-serial, linux-wireless,
netdev
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-26 4:14 ` Al Viro
@ 2019-01-28 21:29 ` shuah
2019-01-30 10:32 ` Johan Hovold
0 siblings, 1 reply; 8+ messages in thread
From: shuah @ 2019-01-28 21:29 UTC (permalink / raw)
To: Al Viro
Cc: marcel, johan.hedberg, w.d.hubbs, chris, kirk, samuel.thibault,
gregkh, robh, jslaby, sameo, davem, arnd, nishka.dasgupta_ug18,
m.maya.nakamura, santhameena13, zhongjiang, linux-bluetooth,
linux-kernel, speakup, devel, linux-serial, linux-wireless,
netdev, shuah
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-28 21:29 ` shuah
@ 2019-01-30 10:32 ` Johan Hovold
2019-01-31 0:35 ` shuah
2019-01-31 15:18 ` Marcel Holtmann
0 siblings, 2 replies; 8+ messages in thread
From: Johan Hovold @ 2019-01-30 10:32 UTC (permalink / raw)
To: shuah
Cc: Al Viro, linux-wireless, chris, devel, robh, sameo, linux-serial,
jslaby, santhameena13, kirk, johan.hedberg, arnd, marcel,
samuel.thibault, m.maya.nakamura, zhongjiang, gregkh, speakup,
linux-kernel, linux-bluetooth, netdev, nishka.dasgupta_ug18,
davem
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-30 10:32 ` Johan Hovold
@ 2019-01-31 0:35 ` shuah
2019-01-31 15:18 ` Marcel Holtmann
1 sibling, 0 replies; 8+ messages in thread
From: shuah @ 2019-01-31 0:35 UTC (permalink / raw)
To: Johan Hovold
Cc: Al Viro, linux-wireless, chris, devel, robh, sameo, linux-serial,
jslaby, santhameena13, kirk, johan.hedberg, arnd, marcel,
samuel.thibault, m.maya.nakamura, zhongjiang, gregkh, speakup,
linux-kernel, linux-bluetooth, netdev, nishka.dasgupta_ug18,
davem, shuah
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-30 10:32 ` Johan Hovold
2019-01-31 0:35 ` shuah
@ 2019-01-31 15:18 ` Marcel Holtmann
2019-01-31 15:33 ` Johan Hovold
1 sibling, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2019-01-31 15:18 UTC (permalink / raw)
To: Johan Hovold
Cc: shuah, Al Viro, open list:NFC SUBSYSTEM, chris, devel,
Rob Herring, Samuel Ortiz, open list:SERIAL DRIVERS, Jiri Slaby,
santhameena13, kirk, Johan Hedberg, Arnd Bergmann,
samuel.thibault, m.maya.nakamura, zhongjiang, Greg KH, speakup,
Linux Kernel Mailing List, Bluez mailing list, netdev,
nishka.dasgupta_ug18, David S. Miller
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-31 15:18 ` Marcel Holtmann
@ 2019-01-31 15:33 ` Johan Hovold
2019-01-31 23:23 ` shuah
0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2019-01-31 15:33 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, shuah, Al Viro, open list:NFC SUBSYSTEM, chris,
devel, Rob Herring, Samuel Ortiz, open list:SERIAL DRIVERS,
Jiri Slaby, santhameena13, kirk, Johan Hedberg, Arnd Bergmann,
samuel.thibault, m.maya.nakamura, zhongjiang, Greg KH, speakup,
Linux Kernel Mailing List, Bluez mailing list, netdev,
nishka.dasgupta_ug18, David S. Miller
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
2019-01-31 15:33 ` Johan Hovold
@ 2019-01-31 23:23 ` shuah
0 siblings, 0 replies; 8+ messages in thread
From: shuah @ 2019-01-31 23:23 UTC (permalink / raw)
To: Johan Hovold, Marcel Holtmann
Cc: Al Viro, open list:NFC SUBSYSTEM, chris, devel, Rob Herring,
Samuel Ortiz, open list:SERIAL DRIVERS, Jiri Slaby,
santhameena13, kirk, Johan Hedberg, Arnd Bergmann,
samuel.thibault, m.maya.nakamura, zhongjiang, Greg KH, speakup,
Linux Kernel Mailing List, Bluez mailing list, netdev,
nishka.dasgupta_ug18, David S. Miller, shuah
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-31 23:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-25 23:29 [PATCH] tty: Fix WARNING in tty_set_termios Shuah Khan
2019-01-26 4:14 ` Al Viro
2019-01-28 21:29 ` shuah
2019-01-30 10:32 ` Johan Hovold
2019-01-31 0:35 ` shuah
2019-01-31 15:18 ` Marcel Holtmann
2019-01-31 15:33 ` Johan Hovold
2019-01-31 23:23 ` shuah
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).