Linux-Bluetooth Archive on lore.kernel.org
 help / Atom feed
* [PATCH v5] bluetooth: Fix WARNING in tty_set_termios()
@ 2019-02-08 23:06 Shuah Khan
  2019-02-09  9:42 ` Johan Hovold
  0 siblings, 1 reply; 3+ messages in thread
From: Shuah Khan @ 2019-02-08 23:06 UTC (permalink / raw)
  To: marcel, johan.hedberg, johan, viro
  Cc: Shuah Khan, 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 hci_uart_tty_open() to check if tty supports set_termios in addition
to write and error out, if it doesn't.

Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
Cc: Johan Hovold <johan@kernel.org>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Shuah Khan <shuah@kernel.org>
---
 drivers/bluetooth/hci_ldisc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..087ec2268744 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -474,10 +474,10 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 
 	BT_DBG("tty %p", tty);
 
-	/* Error if the tty has no write op instead of leaving an exploitable
-	 * hole
+	/* Error if the tty has no write/set_termios ops instead of leaving
+	 * an exploitable hole.
 	 */
-	if (tty->ops->write == NULL)
+	if (!tty->ops->write || !tty->ops->set_termios)
 		return -EOPNOTSUPP;
 
 	hu = kzalloc(sizeof(struct hci_uart), GFP_KERNEL);
-- 
2.17.1


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

* Re: [PATCH v5] bluetooth: Fix WARNING in tty_set_termios()
  2019-02-08 23:06 [PATCH v5] bluetooth: Fix WARNING in tty_set_termios() Shuah Khan
@ 2019-02-09  9:42 ` Johan Hovold
  2019-02-12 18:04   ` shuah
  0 siblings, 1 reply; 3+ messages in thread
From: Johan Hovold @ 2019-02-09  9:42 UTC (permalink / raw)
  To: Shuah Khan
  Cc: marcel, johan.hedberg, johan, viro, linux-bluetooth, linux-kernel

On Fri, Feb 08, 2019 at 04:06:09PM -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 hci_uart_tty_open() to check if tty supports set_termios in addition
> to write and error out, if it doesn't.
> 
> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
>  drivers/bluetooth/hci_ldisc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..087ec2268744 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -474,10 +474,10 @@ static int hci_uart_tty_open(struct tty_struct *tty)
>  
>  	BT_DBG("tty %p", tty);
>  
> -	/* Error if the tty has no write op instead of leaving an exploitable
> -	 * hole
> +	/* Error if the tty has no write/set_termios ops instead of leaving
> +	 * an exploitable hole.

Why do you think that a tty driver not implementing set_termios() is
exploitable?

Note that tty_set_termios() handles a missing set_termios() callback
just fine without dereferencing a NULL-pointer.

>  	 */
> -	if (tty->ops->write == NULL)
> +	if (!tty->ops->write || !tty->ops->set_termios)
>  		return -EOPNOTSUPP;

I know Marcel asked you do implement things like this, but what you're
really doing is to try to uphold the invariant that tty_set_termios()
should never be called for a pty master. The WARN_ON in that function
was put in place to make sure line-disciplines use the correct ioctl
helpers (and the commit I pointed you to earlier in the paragraph you
quote in the commit message broke that invariant).

I'm afraid this is totally obscured by the above change and misleading
comment update.

Johan

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

* Re: [PATCH v5] bluetooth: Fix WARNING in tty_set_termios()
  2019-02-09  9:42 ` Johan Hovold
@ 2019-02-12 18:04   ` shuah
  0 siblings, 0 replies; 3+ messages in thread
From: shuah @ 2019-02-12 18:04 UTC (permalink / raw)
  To: Johan Hovold
  Cc: marcel, johan.hedberg, viro, linux-bluetooth, linux-kernel, shuah

On 2/9/19 2:42 AM, Johan Hovold wrote:
> On Fri, Feb 08, 2019 at 04:06:09PM -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 hci_uart_tty_open() to check if tty supports set_termios in addition
>> to write and error out, if it doesn't.
>>
>> Reported-by: syzbot+a950165cbb86bdd023a4@syzkaller.appspotmail.com
>> Cc: Johan Hovold <johan@kernel.org>
>> Cc: Marcel Holtmann <marcel@holtmann.org>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>> ---
>>   drivers/bluetooth/hci_ldisc.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index fbf7b4df23ab..087ec2268744 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -474,10 +474,10 @@ static int hci_uart_tty_open(struct tty_struct *tty)
>>   
>>   	BT_DBG("tty %p", tty);
>>   
>> -	/* Error if the tty has no write op instead of leaving an exploitable
>> -	 * hole
>> +	/* Error if the tty has no write/set_termios ops instead of leaving
>> +	 * an exploitable hole.
> 
> Why do you think that a tty driver not implementing set_termios() is
> exploitable?
> 
> Note that tty_set_termios() handles a missing set_termios() callback
> just fine without dereferencing a NULL-pointer. >
>>   	 */
>> -	if (tty->ops->write == NULL)
>> +	if (!tty->ops->write || !tty->ops->set_termios)
>>   		return -EOPNOTSUPP;
> 
> I know Marcel asked you do implement things like this, but what you're
> really doing is to try to uphold the invariant that tty_set_termios()
> should never be called for a pty master. The WARN_ON in that function
> was put in place to make sure line-disciplines use the correct ioctl
> helpers (and the commit I pointed you to earlier in the paragraph you
> quote in the commit message broke that invariant).
> 
> I'm afraid this is totally obscured by the above change and misleading
> comment update.
> 

We are discussing the right fix for a while now. :)

I did two versions of the patch

1. Check set_termios in hci_uart_tty_open() based on
Marcel's suggestion
2. Checks tty type in hci_uart_tty_open()

Both fix the problem I can reproduce on my system, however,
I think checking tty type is the right way to fix this problem.

The reason is that tty type is the direct and definitive way to
check if ldisc sets are supported ir unsupported.

Checking set_termios is an indirect way and I am not sure that it
would work in all cases.

That being said, I will wait for some consensus on how to fix the
problem, before I send another version of this patch.

thanks,
-- Shuah


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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08 23:06 [PATCH v5] bluetooth: Fix WARNING in tty_set_termios() Shuah Khan
2019-02-09  9:42 ` Johan Hovold
2019-02-12 18:04   ` 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