[PATCH v2] tty: Fix WARNING in tty_set_termios()

[PATCH v2] tty: Fix WARNING in tty_set_termios()

[Shuah Khan]

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.

Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
Signed-off-by: Shuah Khan <shuah@kernel.org>
---
 drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..ce84ca91ca70 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -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;
-- 
2.17.1

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[Greg KH]

On Thu, Jan 31, 2019 at 04:23:59PM -0700, Shuah Khan wrote:
> 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.
> 
> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
>  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

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[Johan Hovold]

On Thu, Jan 31, 2019 at 04:23:59PM -0700, Shuah Khan wrote:
> tty_set_termios() has the following WARN_ON which 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.

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

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.

> Fix the problem by preventing setting the HCI line discipline for PTYs
> from hci_uart_setup() and hci_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.

> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..ce84ca91ca70 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -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.

> +
>  	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;

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.

> +
>  	/* Init speed if any */
>  	if (hu->init_speed)
>  		speed = hu->init_speed;

Johan

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[Johan Hovold]

On Fri, Feb 01, 2019 at 10:28:43AM +0100, Johan Hovold wrote:
> On Thu, Jan 31, 2019 at 04:23:59PM -0700, Shuah Khan wrote:
> > tty_set_termios() has the following WARN_ON which 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.

> but only if the tty is a pty master, as I mentioned before.

Johan

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[Marcel Holtmann]

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
> 
> 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.
> 
> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
> drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..ce84ca91ca70 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -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

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[shuah]

On 2/1/19 3:00 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
>>
>> 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.
>>
>> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index fbf7b4df23ab..ce84ca91ca70 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -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?
> 

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

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[shuah]

On 2/1/19 2:28 AM, Johan Hovold wrote:
> On Thu, Jan 31, 2019 at 04:23:59PM -0700, Shuah Khan wrote:
>> tty_set_termios() has the following WARN_ON which 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.
> 
>> 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.
> 
> 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

Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()

[shuah]

On 1/31/19 5:14 PM, Greg KH wrote:
> On Thu, Jan 31, 2019 at 04:23:59PM -0700, Shuah Khan wrote:
>> 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.
>>
>> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>> ---
>>   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 :)
> 

Yes. Sorry about that. Will get it right in my v3.

thanks,
-- Shuah