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
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: <[email protected]> # 4.1
Signed-off-by: Myungho Jung <[email protected]>
---
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
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: <[email protected]> # 4.2
Signed-off-by: Myungho Jung <[email protected]>
---
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
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
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
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
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
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: <[email protected]> # 4.2
> Signed-off-by: Myungho Jung <[email protected]>
> ---
> 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