[PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

[PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

[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

[PATCH v3 1/2] Bluetooth: hci_ath: Add NULL check for tiocmget() and tiocmset() in ath_setup()

[Myungho Jung]

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

[PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control()

[Myungho Jung]

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

Re: [PATCH v3 2/2] Bluetooth: hci_ldisc: Add NULL check for tiocmget() and tiocmset() in hci_uart_set_flow_control()

[Marcel Holtmann]

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

Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

[Marcel Holtmann]

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

Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

[Myungho Jung]

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

Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

[Marcel Holtmann]

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

Re: [PATCH v3 0/2] Bluetooth: Add NULL check for tiocmget() and tiocmset()

[Myungho Jung]

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