Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] tty: Fix WARNING in tty_set_termios()
@ 2019-01-31 23:23 Shuah Khan
  2019-02-01  0:14 ` Greg KH
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Shuah Khan @ 2019-01-31 23:23 UTC (permalink / raw)
  To: marcel, johan.hedberg, viro
  Cc: Shuah Khan, gregkh, linux-bluetooth, linux-kernel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-01-31 23:23 [PATCH v2] tty: Fix WARNING in tty_set_termios() Shuah Khan
@ 2019-02-01  0:14 ` Greg KH
  2019-02-01 23:21   ` shuah
  2019-02-01  9:28 ` Johan Hovold
  2019-02-01 10:00 ` Marcel Holtmann
  2 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2019-02-01  0:14 UTC (permalink / raw)
  To: Shuah Khan; +Cc: marcel, johan.hedberg, viro, linux-bluetooth, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-01-31 23:23 [PATCH v2] tty: Fix WARNING in tty_set_termios() Shuah Khan
  2019-02-01  0:14 ` Greg KH
@ 2019-02-01  9:28 ` Johan Hovold
  2019-02-01  9:36   ` Johan Hovold
  2019-02-01 23:20   ` shuah
  2019-02-01 10:00 ` Marcel Holtmann
  2 siblings, 2 replies; 8+ messages in thread
From: Johan Hovold @ 2019-02-01  9:28 UTC (permalink / raw)
  To: Shuah Khan
  Cc: marcel, johan.hedberg, viro, gregkh, linux-bluetooth, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-02-01  9:28 ` Johan Hovold
@ 2019-02-01  9:36   ` Johan Hovold
  2019-02-01 23:20   ` shuah
  1 sibling, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2019-02-01  9:36 UTC (permalink / raw)
  To: Shuah Khan
  Cc: marcel, johan.hedberg, viro, gregkh, linux-bluetooth, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-01-31 23:23 [PATCH v2] tty: Fix WARNING in tty_set_termios() Shuah Khan
  2019-02-01  0:14 ` Greg KH
  2019-02-01  9:28 ` Johan Hovold
@ 2019-02-01 10:00 ` Marcel Holtmann
  2019-02-01 23:18   ` shuah
  2 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2019-02-01 10:00 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Johan Hedberg, Al Viro, Greg KH, Bluez mailing list, linux-kernel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-02-01 10:00 ` Marcel Holtmann
@ 2019-02-01 23:18   ` shuah
  0 siblings, 0 replies; 8+ messages in thread
From: shuah @ 2019-02-01 23:18 UTC (permalink / raw)
  To: Marcel Holtmann, Greg KH
  Cc: Johan Hedberg, Al Viro, Bluez mailing list, linux-kernel,
	Johan Hovold, 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






^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-02-01  9:28 ` Johan Hovold
  2019-02-01  9:36   ` Johan Hovold
@ 2019-02-01 23:20   ` shuah
  1 sibling, 0 replies; 8+ messages in thread
From: shuah @ 2019-02-01 23:20 UTC (permalink / raw)
  To: Johan Hovold
  Cc: marcel, johan.hedberg, viro, gregkh, linux-bluetooth,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] tty: Fix WARNING in tty_set_termios()
  2019-02-01  0:14 ` Greg KH
@ 2019-02-01 23:21   ` shuah
  0 siblings, 0 replies; 8+ messages in thread
From: shuah @ 2019-02-01 23:21 UTC (permalink / raw)
  To: Greg KH; +Cc: marcel, johan.hedberg, viro, linux-bluetooth, linux-kernel, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-31 23:23 [PATCH v2] tty: Fix WARNING in tty_set_termios() Shuah Khan
2019-02-01  0:14 ` Greg KH
2019-02-01 23:21   ` shuah
2019-02-01  9:28 ` Johan Hovold
2019-02-01  9:36   ` Johan Hovold
2019-02-01 23:20   ` shuah
2019-02-01 10:00 ` Marcel Holtmann
2019-02-01 23:18   ` shuah

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox