Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()
@ 2019-02-05  6:41 Myungho Jung
  2019-02-05  6:41 ` [PATCH v3 1/2] Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in ath_setup() Myungho Jung
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Myungho Jung @ 2019-02-05  6:41 UTC (permalink / raw)
  To: marcel; +Cc: johan.hedberg, linux-bluetooth, linux-kernel, Myungho Jung

tiocmget() and tiocmset() operations are optional and some tty drivers
like pty miss the operations. Add NULL checks to prevent from
dereference.

Myungho Jung (2):
  Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in
    ath_setup()
  Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in
    hci_uart_set_flow_control()

 drivers/bluetooth/hci_ath.c   | 6 ++++++
 drivers/bluetooth/hci_ldisc.c | 4 ++++
 2 files changed, 10 insertions(+)

-- 
2.17.1


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

* [PATCH v3 1/2] Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in ath_setup()
  2019-02-05  6:41 [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Myungho Jung
@ 2019-02-05  6:41 ` Myungho Jung
  2019-02-05  6:41 ` [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control() Myungho Jung
  2019-02-05 13:55 ` [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Marcel Holtmann
  2 siblings, 0 replies; 8+ messages in thread
From: Myungho Jung @ 2019-02-05  6:41 UTC (permalink / raw)
  To: marcel; +Cc: johan.hedberg, linux-bluetooth, linux-kernel, Myungho Jung, stable

tiocmget() and tiocmset() operations are optional so they are not
guaranteed to be set. Return ENODEV in ath_setup() if tty driver doesn't
support the operations.

Fixes: 4c876c0edbdc ("hci_uart: Add Atheros support for address config")
Cc: <stable@vger.kernel.org> # 4.1
Signed-off-by: Myungho Jung <mhjungk@gmail.com>
---
Changes in v2:
  - Add NULL check and return error in ath_setup() instead of
    ath_hci_uart_work()

Changes in v3:
  - Fix to return -ENODEV
  - Split into 2 patches
  - Add stable CC and fixes tags

 drivers/bluetooth/hci_ath.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index d568fbd94d6c..9f1ac1805d23 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -185,8 +185,14 @@ static int ath_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 
 static int ath_setup(struct hci_uart *hu)
 {
+	struct tty_struct *tty = hu->tty;
+
 	BT_DBG("hu %p", hu);
 
+	/* tty driver should support operations to set RTS */
+	if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
+		return -ENODEV;
+
 	hu->hdev->set_bdaddr = ath_set_bdaddr;
 
 	return 0;
-- 
2.17.1


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

* [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control()
  2019-02-05  6:41 [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Myungho Jung
  2019-02-05  6:41 ` [PATCH v3 1/2] Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in ath_setup() Myungho Jung
@ 2019-02-05  6:41 ` Myungho Jung
  2019-07-06 10:49   ` Marcel Holtmann
  2019-02-05 13:55 ` [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Marcel Holtmann
  2 siblings, 1 reply; 8+ messages in thread
From: Myungho Jung @ 2019-02-05  6:41 UTC (permalink / raw)
  To: marcel; +Cc: johan.hedberg, linux-bluetooth, linux-kernel, Myungho Jung, stable

tiocmget() or tiocmset() operations are optional. Just return from
hci_uart_set_flow_control() if tiocmget() or tiocmset() operation is
NULL.

Fixes: 2a973dfada2b ("hci_uart: Add new line discipline enhancements")
Cc: <stable@vger.kernel.org> # 4.2
Signed-off-by: Myungho Jung <mhjungk@gmail.com>
---
Changes in v2:
  - Remove braces in if statment

Changes in v3:
  - Split into 2 patches
  - Add stable CC and fixes tags

 drivers/bluetooth/hci_ldisc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..cb31c2d8d826 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -314,6 +314,10 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
 		return;
 	}
 
+	/* tiocmget() and tiocmset() operations are optional */
+	if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
+		return;
+
 	if (enable) {
 		/* Disable hardware flow control */
 		ktermios = tty->termios;
-- 
2.17.1


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

* Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()
  2019-02-05  6:41 [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Myungho Jung
  2019-02-05  6:41 ` [PATCH v3 1/2] Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in ath_setup() Myungho Jung
  2019-02-05  6:41 ` [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control() Myungho Jung
@ 2019-02-05 13:55 ` Marcel Holtmann
  2019-02-06  6:35   ` Myungho Jung
  2 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2019-02-05 13:55 UTC (permalink / raw)
  To: Myungho Jung; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel

Hi Myungho,

> tiocmget() and tiocmset() operations are optional and some tty drivers
> like pty miss the operations. Add NULL checks to prevent from
> dereference.
> 
> Myungho Jung (2):
>  Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in
>    ath_setup()
>  Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in
>    hci_uart_set_flow_control()
> 
> drivers/bluetooth/hci_ath.c   | 6 ++++++
> drivers/bluetooth/hci_ldisc.c | 4 ++++
> 2 files changed, 10 insertions(+)

why are we not enforcing the availability of these in the hci_uart_tty_open?

Regards

Marcel


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

* Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()
  2019-02-05 13:55 ` [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Marcel Holtmann
@ 2019-02-06  6:35   ` Myungho Jung
  2019-02-06  7:06     ` Marcel Holtmann
  0 siblings, 1 reply; 8+ messages in thread
From: Myungho Jung @ 2019-02-06  6:35 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel

On Tue, Feb 05, 2019 at 02:55:50PM +0100, Marcel Holtmann wrote:
> Hi Myungho,
> 
> > tiocmget() and tiocmset() operations are optional and some tty drivers
> > like pty miss the operations. Add NULL checks to prevent from
> > dereference.
> > 
> > Myungho Jung (2):
> >  Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in
> >    ath_setup()
> >  Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in
> >    hci_uart_set_flow_control()
> > 
> > drivers/bluetooth/hci_ath.c   | 6 ++++++
> > drivers/bluetooth/hci_ldisc.c | 4 ++++
> > 2 files changed, 10 insertions(+)
> 
> why are we not enforcing the availability of these in the hci_uart_tty_open?
> 
> Regards
> 
> Marcel
> 

Hi Marcel,

Are the operations required on any HCI UART drivers? For now, I found only 5
drivers (ath, bcm, intel, mrvl, and qca) are explicitly calling them. So, I'm
not sure whether it breaks any existing code with other drivers if returning
error in open().

Thanks,
Myungho

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

* Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()
  2019-02-06  6:35   ` Myungho Jung
@ 2019-02-06  7:06     ` Marcel Holtmann
  2019-02-07 17:34       ` Myungho Jung
  0 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2019-02-06  7:06 UTC (permalink / raw)
  To: Myungho Jung; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel

Hi Myungho,

>>> tiocmget() and tiocmset() operations are optional and some tty drivers
>>> like pty miss the operations. Add NULL checks to prevent from
>>> dereference.
>>> 
>>> Myungho Jung (2):
>>> Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in
>>>   ath_setup()
>>> Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in
>>>   hci_uart_set_flow_control()
>>> 
>>> drivers/bluetooth/hci_ath.c   | 6 ++++++
>>> drivers/bluetooth/hci_ldisc.c | 4 ++++
>>> 2 files changed, 10 insertions(+)
>> 
>> why are we not enforcing the availability of these in the hci_uart_tty_open?
> 
> Are the operations required on any HCI UART drivers? For now, I found only 5
> drivers (ath, bcm, intel, mrvl, and qca) are explicitly calling them. So, I'm
> not sure whether it breaks any existing code with other drivers if returning
> error in open().

the H:4 spec requires setting flow control. In some cases this is done by the hciattach or btattach utility, but it still means that it is required. So failing on TTYs that don’t support it is just fine.

Regards

Marcel


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

* Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()
  2019-02-06  7:06     ` Marcel Holtmann
@ 2019-02-07 17:34       ` Myungho Jung
  0 siblings, 0 replies; 8+ messages in thread
From: Myungho Jung @ 2019-02-07 17:34 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel

On Wed, Feb 06, 2019 at 08:06:54AM +0100, Marcel Holtmann wrote:
> Hi Myungho,
> 
> >>> tiocmget() and tiocmset() operations are optional and some tty drivers
> >>> like pty miss the operations. Add NULL checks to prevent from
> >>> dereference.
> >>> 
> >>> Myungho Jung (2):
> >>> Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in
> >>>   ath_setup()
> >>> Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in
> >>>   hci_uart_set_flow_control()
> >>> 
> >>> drivers/bluetooth/hci_ath.c   | 6 ++++++
> >>> drivers/bluetooth/hci_ldisc.c | 4 ++++
> >>> 2 files changed, 10 insertions(+)
> >> 
> >> why are we not enforcing the availability of these in the hci_uart_tty_open?
> > 
> > Are the operations required on any HCI UART drivers? For now, I found only 5
> > drivers (ath, bcm, intel, mrvl, and qca) are explicitly calling them. So, I'm
> > not sure whether it breaks any existing code with other drivers if returning
> > error in open().
> 
> the H:4 spec requires setting flow control. In some cases this is done by the hciattach or btattach utility, but it still means that it is required. So failing on TTYs that don’t support it is just fine.
> 
> Regards
> 
> Marcel
> 
Ok, let me make a change on hci_uart_tty_open().

Thanks,
Myungho

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

* Re: [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control()
  2019-02-05  6:41 ` [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control() Myungho Jung
@ 2019-07-06 10:49   ` Marcel Holtmann
  0 siblings, 0 replies; 8+ messages in thread
From: Marcel Holtmann @ 2019-07-06 10:49 UTC (permalink / raw)
  To: Myungho Jung; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel, stable

Hi Myungho,

> tiocmget() or tiocmset() operations are optional. Just return from
> hci_uart_set_flow_control() if tiocmget() or tiocmset() operation is
> NULL.
> 
> Fixes: 2a973dfada2b ("hci_uart: Add new line discipline enhancements")
> Cc: <stable@vger.kernel.org> # 4.2
> Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> ---
> Changes in v2:
>  - Remove braces in if statment
> 
> Changes in v3:
>  - Split into 2 patches
>  - Add stable CC and fixes tags
> 
> drivers/bluetooth/hci_ldisc.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..cb31c2d8d826 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -314,6 +314,10 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> 		return;
> 	}
> 
> +	/* tiocmget() and tiocmset() operations are optional */
> +	if (!tty->driver->ops->tiocmget || !tty->driver->ops->tiocmset)
> +		return;
> +

lets just fail setting the line discipline if these ops are not available.  Doing some silent ignoring is not going to help.

Regards

Marcel


^ 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-02-05  6:41 [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Myungho Jung
2019-02-05  6:41 ` [PATCH v3 1/2] Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in ath_setup() Myungho Jung
2019-02-05  6:41 ` [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control() Myungho Jung
2019-07-06 10:49   ` Marcel Holtmann
2019-02-05 13:55 ` [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset() Marcel Holtmann
2019-02-06  6:35   ` Myungho Jung
2019-02-06  7:06     ` Marcel Holtmann
2019-02-07 17:34       ` Myungho Jung

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