[X/D][SRU][CVE-2019-10207] check for missing tty operations

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[X/D][SRU][CVE-2019-10207] check for missing tty operations

Connor Kuehl
https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-10207.html

From the fix description:

"Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
functions which are called by the certain HCI UART protocols (hci_ath,
hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
or directly. This leads to an execution at NULL and can be triggered by
an unprivileged user. Fix this by adding a check for the missing tty
operations to the protocols which use them."

Since the fix required the serdev patch for it to make its checks, I have
included that commit for Xenial as well.

Disco was a clean cherry pick.

Rob Herring (1):
  Bluetooth: hci_uart: add serdev driver support library

Vladis Dronov (1):
  Bluetooth: hci_uart: check for missing tty operations

 drivers/bluetooth/Makefile     |   1 +
 drivers/bluetooth/hci_ath.c    |   3 +
 drivers/bluetooth/hci_bcm.c    |   3 +
 drivers/bluetooth/hci_intel.c  |   3 +
 drivers/bluetooth/hci_ldisc.c  |  13 ++
 drivers/bluetooth/hci_qca.c    |   3 +
 drivers/bluetooth/hci_serdev.c | 361 +++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_uart.h   |   5 +
 8 files changed, 392 insertions(+)
 create mode 100644 drivers/bluetooth/hci_serdev.c

--
2.20.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Xenial][SRU][CVE-2019-10207][PATCH 1/2] Bluetooth: hci_uart: add serdev driver support library

Connor Kuehl
From: Rob Herring <[hidden email]>

CVE-2019-10207

This adds library functions for serdev based BT drivers. This is largely
copied from hci_ldisc.c and modified to use serdev calls. There's a little
bit of duplication, but I avoided intermixing this as the ldisc code should
eventually go away.

Signed-off-by: Rob Herring <[hidden email]>
Cc: Marcel Holtmann <[hidden email]>
Cc: Gustavo Padovan <[hidden email]>
Cc: Johan Hedberg <[hidden email]>
Cc: [hidden email]
Acked-by: Pavel Machek <[hidden email]>
[Fix style issues reported by Pavel]
Signed-off-by: Sebastian Reichel <[hidden email]>
Signed-off-by: Marcel Holtmann <[hidden email]>
(cherry picked from commit 82f5169bf3d3b5d8f7f7c437d2d83435173cb638)
Signed-off-by: Connor Kuehl <[hidden email]>
---
 drivers/bluetooth/Makefile     |   1 +
 drivers/bluetooth/hci_serdev.c | 361 +++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_uart.h   |   4 +
 3 files changed, 366 insertions(+)
 create mode 100644 drivers/bluetooth/hci_serdev.c

diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 07c9cf381e5a..67f1987c4e97 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -28,6 +28,7 @@ btmrvl-y := btmrvl_main.o
 btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
 
 hci_uart-y := hci_ldisc.o
+hci_uart-$(CONFIG_SERIAL_DEV_BUS) += hci_serdev.o
 hci_uart-$(CONFIG_BT_HCIUART_H4) += hci_h4.o
 hci_uart-$(CONFIG_BT_HCIUART_BCSP) += hci_bcsp.o
 hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o
diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
new file mode 100644
index 000000000000..f5ccb2c7ef92
--- /dev/null
+++ b/drivers/bluetooth/hci_serdev.c
@@ -0,0 +1,361 @@
+/*
+ *  Bluetooth HCI serdev driver lib
+ *
+ *  Copyright (C) 2017  Linaro, Ltd., Rob Herring <[hidden email]>
+ *
+ *  Based on hci_ldisc.c:
+ *
+ *  Copyright (C) 2000-2001  Qualcomm Incorporated
+ *  Copyright (C) 2002-2003  Maxim Krasnyansky <[hidden email]>
+ *  Copyright (C) 2004-2005  Marcel Holtmann <[hidden email]>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/serdev.h>
+#include <linux/skbuff.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "hci_uart.h"
+
+struct serdev_device_ops hci_serdev_client_ops;
+
+static inline void hci_uart_tx_complete(struct hci_uart *hu, int pkt_type)
+{
+ struct hci_dev *hdev = hu->hdev;
+
+ /* Update HCI stat counters */
+ switch (pkt_type) {
+ case HCI_COMMAND_PKT:
+ hdev->stat.cmd_tx++;
+ break;
+
+ case HCI_ACLDATA_PKT:
+ hdev->stat.acl_tx++;
+ break;
+
+ case HCI_SCODATA_PKT:
+ hdev->stat.sco_tx++;
+ break;
+ }
+}
+
+static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
+{
+ struct sk_buff *skb = hu->tx_skb;
+
+ if (!skb)
+ skb = hu->proto->dequeue(hu);
+ else
+ hu->tx_skb = NULL;
+
+ return skb;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+ struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+ struct serdev_device *serdev = hu->serdev;
+ struct hci_dev *hdev = hu->hdev;
+ struct sk_buff *skb;
+
+ /* REVISIT:
+ * should we cope with bad skbs or ->write() returning an error value?
+ */
+ do {
+ clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
+
+ while ((skb = hci_uart_dequeue(hu))) {
+ int len;
+
+ len = serdev_device_write_buf(serdev,
+      skb->data, skb->len);
+ hdev->stat.byte_tx += len;
+
+ skb_pull(skb, len);
+ if (skb->len) {
+ hu->tx_skb = skb;
+ break;
+ }
+
+ hci_uart_tx_complete(hu, hci_skb_pkt_type(skb));
+ kfree_skb(skb);
+ }
+ } while(test_bit(HCI_UART_TX_WAKEUP, &hu->tx_state));
+
+ clear_bit(HCI_UART_SENDING, &hu->tx_state);
+}
+
+/* ------- Interface to HCI layer ------ */
+
+/* Initialize device */
+static int hci_uart_open(struct hci_dev *hdev)
+{
+ struct hci_uart *hu  = hci_get_drvdata(hdev);
+
+ BT_DBG("%s %p", hdev->name, hdev);
+
+ serdev_device_set_client_ops(hu->serdev, &hci_serdev_client_ops);
+
+ return serdev_device_open(hu->serdev);
+}
+
+/* Reset device */
+static int hci_uart_flush(struct hci_dev *hdev)
+{
+ struct hci_uart *hu  = hci_get_drvdata(hdev);
+
+ BT_DBG("hdev %p serdev %p", hdev, hu->serdev);
+
+ if (hu->tx_skb) {
+ kfree_skb(hu->tx_skb); hu->tx_skb = NULL;
+ }
+
+ /* Flush any pending characters in the driver and discipline. */
+ serdev_device_write_flush(hu->serdev);
+
+ if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ hu->proto->flush(hu);
+
+ return 0;
+}
+
+/* Close device */
+static int hci_uart_close(struct hci_dev *hdev)
+{
+ struct hci_uart *hu  = hci_get_drvdata(hdev);
+
+ BT_DBG("hdev %p", hdev);
+
+ hci_uart_flush(hdev);
+ hdev->flush = NULL;
+
+ serdev_device_close(hu->serdev);
+
+ return 0;
+}
+
+/* Send frames from HCI layer */
+static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+
+ BT_DBG("%s: type %d len %d", hdev->name, hci_skb_pkt_type(skb),
+       skb->len);
+
+ hu->proto->enqueue(hu, skb);
+
+ hci_uart_tx_wakeup(hu);
+
+ return 0;
+}
+
+static int hci_uart_setup(struct hci_dev *hdev)
+{
+ struct hci_uart *hu = hci_get_drvdata(hdev);
+ struct hci_rp_read_local_version *ver;
+ struct sk_buff *skb;
+ unsigned int speed;
+ int err;
+
+ /* Init speed if any */
+ if (hu->init_speed)
+ speed = hu->init_speed;
+ else if (hu->proto->init_speed)
+ speed = hu->proto->init_speed;
+ else
+ speed = 0;
+
+ if (speed)
+ serdev_device_set_baudrate(hu->serdev, speed);
+
+ /* Operational speed if any */
+ if (hu->oper_speed)
+ speed = hu->oper_speed;
+ else if (hu->proto->oper_speed)
+ speed = hu->proto->oper_speed;
+ else
+ speed = 0;
+
+ if (hu->proto->set_baudrate && speed) {
+ err = hu->proto->set_baudrate(hu, speed);
+ if (err)
+ BT_ERR("%s: failed to set baudrate", hdev->name);
+ else
+ serdev_device_set_baudrate(hu->serdev, speed);
+ }
+
+ if (hu->proto->setup)
+ return hu->proto->setup(hu);
+
+ if (!test_bit(HCI_UART_VND_DETECT, &hu->hdev_flags))
+ return 0;
+
+ skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
+     HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ BT_ERR("%s: Reading local version information failed (%ld)",
+       hdev->name, PTR_ERR(skb));
+ return 0;
+ }
+
+ if (skb->len != sizeof(*ver)) {
+ BT_ERR("%s: Event length mismatch for version information",
+       hdev->name);
+ }
+
+ kfree_skb(skb);
+ return 0;
+}
+
+/** hci_uart_write_wakeup - transmit buffer wakeup
+ * @serdev: serial device
+ *
+ * This function is called by the serdev framework when it accepts
+ * more data being sent.
+ */
+static void hci_uart_write_wakeup(struct serdev_device *serdev)
+{
+ struct hci_uart *hu = serdev_device_get_drvdata(serdev);
+
+ BT_DBG("");
+
+ if (!hu || serdev != hu->serdev) {
+ WARN_ON(1);
+ return;
+ }
+
+ if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ hci_uart_tx_wakeup(hu);
+}
+
+/** hci_uart_receive_buf - receive buffer wakeup
+ * @serdev: serial device
+ * @data:   pointer to received data
+ * @count:  count of received data in bytes
+ *
+ * This function is called by the serdev framework when it received data
+ * in the RX buffer.
+ *
+ * Return: number of processed bytes
+ */
+static int hci_uart_receive_buf(struct serdev_device *serdev, const u8 *data,
+   size_t count)
+{
+ struct hci_uart *hu = serdev_device_get_drvdata(serdev);
+
+ if (!hu || serdev != hu->serdev) {
+ WARN_ON(1);
+ return 0;
+ }
+
+ if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
+ return 0;
+
+ /* It does not need a lock here as it is already protected by a mutex in
+ * tty caller
+ */
+ hu->proto->recv(hu, data, count);
+
+ if (hu->hdev)
+ hu->hdev->stat.byte_rx += count;
+
+ return count;
+}
+
+struct serdev_device_ops hci_serdev_client_ops = {
+ .receive_buf = hci_uart_receive_buf,
+ .write_wakeup = hci_uart_write_wakeup,
+};
+
+int hci_uart_register_device(struct hci_uart *hu,
+     const struct hci_uart_proto *p)
+{
+ int err;
+ struct hci_dev *hdev;
+
+ BT_DBG("");
+
+ err = p->open(hu);
+ if (err)
+ return err;
+
+ hu->proto = p;
+ set_bit(HCI_UART_PROTO_READY, &hu->flags);
+
+ /* Initialize and register HCI device */
+ hdev = hci_alloc_dev();
+ if (!hdev) {
+ BT_ERR("Can't allocate HCI device");
+ err = -ENOMEM;
+ goto err_alloc;
+ }
+
+ hu->hdev = hdev;
+
+ hdev->bus = HCI_UART;
+ hci_set_drvdata(hdev, hu);
+
+ INIT_WORK(&hu->write_work, hci_uart_write_work);
+
+ /* Only when vendor specific setup callback is provided, consider
+ * the manufacturer information valid. This avoids filling in the
+ * value for Ericsson when nothing is specified.
+ */
+ if (hu->proto->setup)
+ hdev->manufacturer = hu->proto->manufacturer;
+
+ hdev->open  = hci_uart_open;
+ hdev->close = hci_uart_close;
+ hdev->flush = hci_uart_flush;
+ hdev->send  = hci_uart_send_frame;
+ hdev->setup = hci_uart_setup;
+ SET_HCIDEV_DEV(hdev, &hu->serdev->dev);
+
+ if (test_bit(HCI_UART_RAW_DEVICE, &hu->hdev_flags))
+ set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
+
+ if (test_bit(HCI_UART_EXT_CONFIG, &hu->hdev_flags))
+ set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks);
+
+ if (!test_bit(HCI_UART_RESET_ON_INIT, &hu->hdev_flags))
+ set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
+
+ if (test_bit(HCI_UART_CREATE_AMP, &hu->hdev_flags))
+ hdev->dev_type = HCI_AMP;
+ else
+ hdev->dev_type = HCI_PRIMARY;
+
+ if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
+ return 0;
+
+ if (hci_register_dev(hdev) < 0) {
+ BT_ERR("Can't register HCI device");
+ err = -ENODEV;
+ goto err_register;
+ }
+
+ set_bit(HCI_UART_REGISTERED, &hu->flags);
+
+ return 0;
+
+err_register:
+ hci_free_dev(hdev);
+err_alloc:
+ clear_bit(HCI_UART_PROTO_READY, &hu->flags);
+ p->close(hu);
+ return err;
+}
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 82c92f1b65b4..168aaaa2e58c 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -55,6 +55,7 @@
 #define HCI_UART_VND_DETECT 5
 
 struct hci_uart;
+struct serdev_device;
 
 struct hci_uart_proto {
  unsigned int id;
@@ -74,6 +75,7 @@ struct hci_uart_proto {
 
 struct hci_uart {
  struct tty_struct *tty;
+ struct serdev_device *serdev;
  struct hci_dev *hdev;
  unsigned long flags;
  unsigned long hdev_flags;
@@ -101,6 +103,8 @@ struct hci_uart {
 
 int hci_uart_register_proto(const struct hci_uart_proto *p);
 int hci_uart_unregister_proto(const struct hci_uart_proto *p);
+int hci_uart_register_device(struct hci_uart *hu, const struct hci_uart_proto *p);
+
 int hci_uart_tx_wakeup(struct hci_uart *hu);
 int hci_uart_init_ready(struct hci_uart *hu);
 void hci_uart_init_tty(struct hci_uart *hu);
--
2.20.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Xenial][SRU][CVE-2019-10207][PATCH 2/2] Bluetooth: hci_uart: check for missing tty operations

Connor Kuehl
In reply to this post by Connor Kuehl
From: Vladis Dronov <[hidden email]>

CVE-2019-10207

Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
functions which are called by the certain HCI UART protocols (hci_ath,
hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
or directly. This leads to an execution at NULL and can be triggered by
an unprivileged user. Fix this by adding a helper function and a check
for the missing tty operations in the protocols code.

This fixes CVE-2019-10207. The Fixes: lines list commits where calls to
tiocm[gs]et() or hci_uart_set_flow_control() were added to the HCI UART
protocols.

Link: https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
Reported-by: [hidden email]
Cc: [hidden email] # v2.6.36+
Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate configuration support")
Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990")
Signed-off-by: Vladis Dronov <[hidden email]>
Signed-off-by: Marcel Holtmann <[hidden email]>
Reviewed-by: Yu-Chen, Cho <[hidden email]>
Tested-by: Yu-Chen, Cho <[hidden email]>
Signed-off-by: Linus Torvalds <[hidden email]>
(backported from commit b36a1552d7319bbfd5cf7f08726c23c5c66d4f73)
[ Connor Kuehl: drivers/bluetooth/hci_mrvl.c does not exist in Xenial,
  so that hunk was dropped. The qca_open() function had a very minor
  merge conflict but that's just because the kzalloc invocation used
  GFP_KERNEL in the patch but Xenial uses GFP_ATOMIC. ]
Signed-off-by: Connor Kuehl <[hidden email]>
---
 drivers/bluetooth/hci_ath.c   |  3 +++
 drivers/bluetooth/hci_bcm.c   |  3 +++
 drivers/bluetooth/hci_intel.c |  3 +++
 drivers/bluetooth/hci_ldisc.c | 13 +++++++++++++
 drivers/bluetooth/hci_qca.c   |  3 +++
 drivers/bluetooth/hci_uart.h  |  1 +
 6 files changed, 26 insertions(+)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index d776dfd51478..16f2131687e5 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -101,6 +101,9 @@ static int ath_open(struct hci_uart *hu)
 
  BT_DBG("hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  ath = kzalloc(sizeof(*ath), GFP_KERNEL);
  if (!ath)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index f9b569ef3dd7..20a1b4d1fd09 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -279,6 +279,9 @@ static int bcm_open(struct hci_uart *hu)
 
  bt_dev_dbg(hu->hdev, "hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
  if (!bcm)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f40a86960fde..772c91d843ff 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -407,6 +407,9 @@ static int intel_open(struct hci_uart *hu)
 
  BT_DBG("hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  intel = kzalloc(sizeof(*intel), GFP_KERNEL);
  if (!intel)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 96bcec5598c2..8e4362361769 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -257,6 +257,19 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
  return 0;
 }
 
+/* Check the underlying device or tty has flow control support */
+bool hci_uart_has_flow_control(struct hci_uart *hu)
+{
+ /* serdev nodes check if the needed operations are present */
+ if (hu->serdev)
+ return true;
+
+ if (hu->tty->driver->ops->tiocmget && hu->tty->driver->ops->tiocmset)
+ return true;
+
+ return false;
+}
+
 /* Flow control or un-flow control the device */
 void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
 {
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index ecfb9ed2cff6..6b5b9ae6e809 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -390,6 +390,9 @@ static int qca_open(struct hci_uart *hu)
 
  BT_DBG("hu %p qca_open", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  qca = kzalloc(sizeof(struct qca_data), GFP_ATOMIC);
  if (!qca)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 168aaaa2e58c..0f4bd63b94ee 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -109,6 +109,7 @@ int hci_uart_tx_wakeup(struct hci_uart *hu);
 int hci_uart_init_ready(struct hci_uart *hu);
 void hci_uart_init_tty(struct hci_uart *hu);
 void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
+bool hci_uart_has_flow_control(struct hci_uart *hu);
 void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);
 void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
  unsigned int oper_speed);
--
2.20.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Disco][SRU][CVE-2019-10207][PATCH] Bluetooth: hci_uart: check for missing tty operations

Connor Kuehl
In reply to this post by Connor Kuehl
From: Vladis Dronov <[hidden email]>

CVE-2019-10207

Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
functions which are called by the certain HCI UART protocols (hci_ath,
hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
or directly. This leads to an execution at NULL and can be triggered by
an unprivileged user. Fix this by adding a helper function and a check
for the missing tty operations in the protocols code.

This fixes CVE-2019-10207. The Fixes: lines list commits where calls to
tiocm[gs]et() or hci_uart_set_flow_control() were added to the HCI UART
protocols.

Link: https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
Reported-by: [hidden email]
Cc: [hidden email] # v2.6.36+
Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate configuration support")
Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990")
Signed-off-by: Vladis Dronov <[hidden email]>
Signed-off-by: Marcel Holtmann <[hidden email]>
Reviewed-by: Yu-Chen, Cho <[hidden email]>
Tested-by: Yu-Chen, Cho <[hidden email]>
Signed-off-by: Linus Torvalds <[hidden email]>
(cherry picked from commit b36a1552d7319bbfd5cf7f08726c23c5c66d4f73)
Signed-off-by: Connor Kuehl <[hidden email]>
---
 drivers/bluetooth/hci_ath.c   |  3 +++
 drivers/bluetooth/hci_bcm.c   |  3 +++
 drivers/bluetooth/hci_intel.c |  3 +++
 drivers/bluetooth/hci_ldisc.c | 13 +++++++++++++
 drivers/bluetooth/hci_mrvl.c  |  3 +++
 drivers/bluetooth/hci_qca.c   |  3 +++
 drivers/bluetooth/hci_uart.h  |  1 +
 7 files changed, 29 insertions(+)

diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index d568fbd94d6c..20235925344d 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -112,6 +112,9 @@ static int ath_open(struct hci_uart *hu)
 
  BT_DBG("hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  ath = kzalloc(sizeof(*ath), GFP_KERNEL);
  if (!ath)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index b5d31d583d60..3d5376d81ebd 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -421,6 +421,9 @@ static int bcm_open(struct hci_uart *hu)
 
  bt_dev_dbg(hu->hdev, "hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
  if (!bcm)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
index f31410526c57..c6d2ef75ae61 100644
--- a/drivers/bluetooth/hci_intel.c
+++ b/drivers/bluetooth/hci_intel.c
@@ -406,6 +406,9 @@ static int intel_open(struct hci_uart *hu)
 
  BT_DBG("hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  intel = kzalloc(sizeof(*intel), GFP_KERNEL);
  if (!intel)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 9562e72c1ae5..2154c18ad1f8 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -299,6 +299,19 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
  return 0;
 }
 
+/* Check the underlying device or tty has flow control support */
+bool hci_uart_has_flow_control(struct hci_uart *hu)
+{
+ /* serdev nodes check if the needed operations are present */
+ if (hu->serdev)
+ return true;
+
+ if (hu->tty->driver->ops->tiocmget && hu->tty->driver->ops->tiocmset)
+ return true;
+
+ return false;
+}
+
 /* Flow control or un-flow control the device */
 void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
 {
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index ffb00669346f..23791df081ba 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -66,6 +66,9 @@ static int mrvl_open(struct hci_uart *hu)
 
  BT_DBG("hu %p", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  mrvl = kzalloc(sizeof(*mrvl), GFP_KERNEL);
  if (!mrvl)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f036c8f98ea3..ff9c2d2a01dc 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -455,6 +455,9 @@ static int qca_open(struct hci_uart *hu)
 
  BT_DBG("hu %p qca_open", hu);
 
+ if (!hci_uart_has_flow_control(hu))
+ return -EOPNOTSUPP;
+
  qca = kzalloc(sizeof(struct qca_data), GFP_KERNEL);
  if (!qca)
  return -ENOMEM;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 00cab2fd7a1b..067a610f1372 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -118,6 +118,7 @@ int hci_uart_tx_wakeup(struct hci_uart *hu);
 int hci_uart_init_ready(struct hci_uart *hu);
 void hci_uart_init_work(struct work_struct *work);
 void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
+bool hci_uart_has_flow_control(struct hci_uart *hu);
 void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);
 void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
  unsigned int oper_speed);
--
2.20.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [Disco][SRU][CVE-2019-10207][PATCH] Bluetooth: hci_uart: check for missing tty operations

Tyler Hicks-2
On 2019-08-12 16:08:45, Connor Kuehl wrote:

> From: Vladis Dronov <[hidden email]>
>
> CVE-2019-10207
>
> Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
> functions which are called by the certain HCI UART protocols (hci_ath,
> hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
> or directly. This leads to an execution at NULL and can be triggered by
> an unprivileged user. Fix this by adding a helper function and a check
> for the missing tty operations in the protocols code.
>
> This fixes CVE-2019-10207. The Fixes: lines list commits where calls to
> tiocm[gs]et() or hci_uart_set_flow_control() were added to the HCI UART
> protocols.
>
> Link: https://syzkaller.appspot.com/bug?id=1b42faa2848963564a5b1b7f8c837ea7b55ffa50
> Reported-by: [hidden email]
> Cc: [hidden email] # v2.6.36+
> Fixes: b3190df62861 ("Bluetooth: Support for Atheros AR300x serial chip")
> Fixes: 118612fb9165 ("Bluetooth: hci_bcm: Add suspend/resume PM functions")
> Fixes: ff2895592f0f ("Bluetooth: hci_intel: Add Intel baudrate configuration support")
> Fixes: 162f812f23ba ("Bluetooth: hci_uart: Add Marvell support")
> Fixes: fa9ad876b8e0 ("Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990")
> Signed-off-by: Vladis Dronov <[hidden email]>
> Signed-off-by: Marcel Holtmann <[hidden email]>
> Reviewed-by: Yu-Chen, Cho <[hidden email]>
> Tested-by: Yu-Chen, Cho <[hidden email]>
> Signed-off-by: Linus Torvalds <[hidden email]>
> (cherry picked from commit b36a1552d7319bbfd5cf7f08726c23c5c66d4f73)
> Signed-off-by: Connor Kuehl <[hidden email]>

For Disco only,

 Acked-by: Tyler Hicks <[hidden email]>

Tyler

> ---
>  drivers/bluetooth/hci_ath.c   |  3 +++
>  drivers/bluetooth/hci_bcm.c   |  3 +++
>  drivers/bluetooth/hci_intel.c |  3 +++
>  drivers/bluetooth/hci_ldisc.c | 13 +++++++++++++
>  drivers/bluetooth/hci_mrvl.c  |  3 +++
>  drivers/bluetooth/hci_qca.c   |  3 +++
>  drivers/bluetooth/hci_uart.h  |  1 +
>  7 files changed, 29 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
> index d568fbd94d6c..20235925344d 100644
> --- a/drivers/bluetooth/hci_ath.c
> +++ b/drivers/bluetooth/hci_ath.c
> @@ -112,6 +112,9 @@ static int ath_open(struct hci_uart *hu)
>  
>   BT_DBG("hu %p", hu);
>  
> + if (!hci_uart_has_flow_control(hu))
> + return -EOPNOTSUPP;
> +
>   ath = kzalloc(sizeof(*ath), GFP_KERNEL);
>   if (!ath)
>   return -ENOMEM;
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index b5d31d583d60..3d5376d81ebd 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -421,6 +421,9 @@ static int bcm_open(struct hci_uart *hu)
>  
>   bt_dev_dbg(hu->hdev, "hu %p", hu);
>  
> + if (!hci_uart_has_flow_control(hu))
> + return -EOPNOTSUPP;
> +
>   bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
>   if (!bcm)
>   return -ENOMEM;
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index f31410526c57..c6d2ef75ae61 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -406,6 +406,9 @@ static int intel_open(struct hci_uart *hu)
>  
>   BT_DBG("hu %p", hu);
>  
> + if (!hci_uart_has_flow_control(hu))
> + return -EOPNOTSUPP;
> +
>   intel = kzalloc(sizeof(*intel), GFP_KERNEL);
>   if (!intel)
>   return -ENOMEM;
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 9562e72c1ae5..2154c18ad1f8 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -299,6 +299,19 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>   return 0;
>  }
>  
> +/* Check the underlying device or tty has flow control support */
> +bool hci_uart_has_flow_control(struct hci_uart *hu)
> +{
> + /* serdev nodes check if the needed operations are present */
> + if (hu->serdev)
> + return true;
> +
> + if (hu->tty->driver->ops->tiocmget && hu->tty->driver->ops->tiocmset)
> + return true;
> +
> + return false;
> +}
> +
>  /* Flow control or un-flow control the device */
>  void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>  {
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> index ffb00669346f..23791df081ba 100644
> --- a/drivers/bluetooth/hci_mrvl.c
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -66,6 +66,9 @@ static int mrvl_open(struct hci_uart *hu)
>  
>   BT_DBG("hu %p", hu);
>  
> + if (!hci_uart_has_flow_control(hu))
> + return -EOPNOTSUPP;
> +
>   mrvl = kzalloc(sizeof(*mrvl), GFP_KERNEL);
>   if (!mrvl)
>   return -ENOMEM;
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f036c8f98ea3..ff9c2d2a01dc 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -455,6 +455,9 @@ static int qca_open(struct hci_uart *hu)
>  
>   BT_DBG("hu %p qca_open", hu);
>  
> + if (!hci_uart_has_flow_control(hu))
> + return -EOPNOTSUPP;
> +
>   qca = kzalloc(sizeof(struct qca_data), GFP_KERNEL);
>   if (!qca)
>   return -ENOMEM;
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 00cab2fd7a1b..067a610f1372 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -118,6 +118,7 @@ int hci_uart_tx_wakeup(struct hci_uart *hu);
>  int hci_uart_init_ready(struct hci_uart *hu);
>  void hci_uart_init_work(struct work_struct *work);
>  void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
> +bool hci_uart_has_flow_control(struct hci_uart *hu);
>  void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);
>  void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
>   unsigned int oper_speed);
> --
> 2.20.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Xenial][SRU][CVE-2019-10207][PATCH 1/2] Bluetooth: hci_uart: add serdev driver support library

Tyler Hicks-2
In reply to this post by Connor Kuehl
On 2019-08-12 16:08:43, Connor Kuehl wrote:

> From: Rob Herring <[hidden email]>
>
> CVE-2019-10207
>
> This adds library functions for serdev based BT drivers. This is largely
> copied from hci_ldisc.c and modified to use serdev calls. There's a little
> bit of duplication, but I avoided intermixing this as the ldisc code should
> eventually go away.
>
> Signed-off-by: Rob Herring <[hidden email]>
> Cc: Marcel Holtmann <[hidden email]>
> Cc: Gustavo Padovan <[hidden email]>
> Cc: Johan Hedberg <[hidden email]>
> Cc: [hidden email]
> Acked-by: Pavel Machek <[hidden email]>
> [Fix style issues reported by Pavel]
> Signed-off-by: Sebastian Reichel <[hidden email]>
> Signed-off-by: Marcel Holtmann <[hidden email]>
> (cherry picked from commit 82f5169bf3d3b5d8f7f7c437d2d83435173cb638)
> Signed-off-by: Connor Kuehl <[hidden email]>
> ---
>  drivers/bluetooth/Makefile     |   1 +
>  drivers/bluetooth/hci_serdev.c | 361 +++++++++++++++++++++++++++++++++
>  drivers/bluetooth/hci_uart.h   |   4 +
>  3 files changed, 366 insertions(+)
>  create mode 100644 drivers/bluetooth/hci_serdev.c
>
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 07c9cf381e5a..67f1987c4e97 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -28,6 +28,7 @@ btmrvl-y := btmrvl_main.o
>  btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
>  
>  hci_uart-y := hci_ldisc.o
> +hci_uart-$(CONFIG_SERIAL_DEV_BUS) += hci_serdev.o
>  hci_uart-$(CONFIG_BT_HCIUART_H4) += hci_h4.o
>  hci_uart-$(CONFIG_BT_HCIUART_BCSP) += hci_bcsp.o
>  hci_uart-$(CONFIG_BT_HCIUART_LL) += hci_ll.o
> diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
> new file mode 100644
> index 000000000000..f5ccb2c7ef92
> --- /dev/null
> +++ b/drivers/bluetooth/hci_serdev.c
> @@ -0,0 +1,361 @@
> +/*
> + *  Bluetooth HCI serdev driver lib
> + *
> + *  Copyright (C) 2017  Linaro, Ltd., Rob Herring <[hidden email]>
> + *
> + *  Based on hci_ldisc.c:
> + *
> + *  Copyright (C) 2000-2001  Qualcomm Incorporated
> + *  Copyright (C) 2002-2003  Maxim Krasnyansky <[hidden email]>
> + *  Copyright (C) 2004-2005  Marcel Holtmann <[hidden email]>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/serdev.h>
> +#include <linux/skbuff.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "hci_uart.h"
> +
> +struct serdev_device_ops hci_serdev_client_ops;
> +
> +static inline void hci_uart_tx_complete(struct hci_uart *hu, int pkt_type)
> +{
> + struct hci_dev *hdev = hu->hdev;
> +
> + /* Update HCI stat counters */
> + switch (pkt_type) {
> + case HCI_COMMAND_PKT:
> + hdev->stat.cmd_tx++;
> + break;
> +
> + case HCI_ACLDATA_PKT:
> + hdev->stat.acl_tx++;
> + break;
> +
> + case HCI_SCODATA_PKT:
> + hdev->stat.sco_tx++;
> + break;
> + }
> +}
> +
> +static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
> +{
> + struct sk_buff *skb = hu->tx_skb;
> +
> + if (!skb)
> + skb = hu->proto->dequeue(hu);
> + else
> + hu->tx_skb = NULL;
> +
> + return skb;
> +}
> +
> +static void hci_uart_write_work(struct work_struct *work)
> +{
> + struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
> + struct serdev_device *serdev = hu->serdev;
> + struct hci_dev *hdev = hu->hdev;
> + struct sk_buff *skb;
> +
> + /* REVISIT:
> + * should we cope with bad skbs or ->write() returning an error value?
> + */
> + do {
> + clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
> +
> + while ((skb = hci_uart_dequeue(hu))) {
> + int len;
> +
> + len = serdev_device_write_buf(serdev,
> +      skb->data, skb->len);
> + hdev->stat.byte_tx += len;
> +
> + skb_pull(skb, len);
> + if (skb->len) {
> + hu->tx_skb = skb;
> + break;
> + }
> +
> + hci_uart_tx_complete(hu, hci_skb_pkt_type(skb));
> + kfree_skb(skb);
> + }
> + } while(test_bit(HCI_UART_TX_WAKEUP, &hu->tx_state));
> +
> + clear_bit(HCI_UART_SENDING, &hu->tx_state);
> +}
> +
> +/* ------- Interface to HCI layer ------ */
> +
> +/* Initialize device */
> +static int hci_uart_open(struct hci_dev *hdev)
> +{
> + struct hci_uart *hu  = hci_get_drvdata(hdev);
> +
> + BT_DBG("%s %p", hdev->name, hdev);
> +
> + serdev_device_set_client_ops(hu->serdev, &hci_serdev_client_ops);
> +
> + return serdev_device_open(hu->serdev);
> +}
> +
> +/* Reset device */
> +static int hci_uart_flush(struct hci_dev *hdev)
> +{
> + struct hci_uart *hu  = hci_get_drvdata(hdev);
> +
> + BT_DBG("hdev %p serdev %p", hdev, hu->serdev);
> +
> + if (hu->tx_skb) {
> + kfree_skb(hu->tx_skb); hu->tx_skb = NULL;
> + }
> +
> + /* Flush any pending characters in the driver and discipline. */
> + serdev_device_write_flush(hu->serdev);
> +
> + if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> + hu->proto->flush(hu);
> +
> + return 0;
> +}
> +
> +/* Close device */
> +static int hci_uart_close(struct hci_dev *hdev)
> +{
> + struct hci_uart *hu  = hci_get_drvdata(hdev);
> +
> + BT_DBG("hdev %p", hdev);
> +
> + hci_uart_flush(hdev);
> + hdev->flush = NULL;
> +
> + serdev_device_close(hu->serdev);
> +
> + return 0;
> +}
> +
> +/* Send frames from HCI layer */
> +static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> +
> + BT_DBG("%s: type %d len %d", hdev->name, hci_skb_pkt_type(skb),
> +       skb->len);
> +
> + hu->proto->enqueue(hu, skb);
> +
> + hci_uart_tx_wakeup(hu);
> +
> + return 0;
> +}
> +
> +static int hci_uart_setup(struct hci_dev *hdev)
> +{
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> + struct hci_rp_read_local_version *ver;
> + struct sk_buff *skb;
> + unsigned int speed;
> + int err;
> +
> + /* Init speed if any */
> + if (hu->init_speed)
> + speed = hu->init_speed;
> + else if (hu->proto->init_speed)
> + speed = hu->proto->init_speed;
> + else
> + speed = 0;
> +
> + if (speed)
> + serdev_device_set_baudrate(hu->serdev, speed);
> +
> + /* Operational speed if any */
> + if (hu->oper_speed)
> + speed = hu->oper_speed;
> + else if (hu->proto->oper_speed)
> + speed = hu->proto->oper_speed;
> + else
> + speed = 0;
> +
> + if (hu->proto->set_baudrate && speed) {
> + err = hu->proto->set_baudrate(hu, speed);
> + if (err)
> + BT_ERR("%s: failed to set baudrate", hdev->name);
> + else
> + serdev_device_set_baudrate(hu->serdev, speed);
> + }
> +
> + if (hu->proto->setup)
> + return hu->proto->setup(hu);
> +
> + if (!test_bit(HCI_UART_VND_DETECT, &hu->hdev_flags))
> + return 0;
> +
> + skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
> +     HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + BT_ERR("%s: Reading local version information failed (%ld)",
> +       hdev->name, PTR_ERR(skb));
> + return 0;
> + }
> +
> + if (skb->len != sizeof(*ver)) {
> + BT_ERR("%s: Event length mismatch for version information",
> +       hdev->name);
> + }
> +
> + kfree_skb(skb);
> + return 0;
> +}
> +
> +/** hci_uart_write_wakeup - transmit buffer wakeup
> + * @serdev: serial device
> + *
> + * This function is called by the serdev framework when it accepts
> + * more data being sent.
> + */
> +static void hci_uart_write_wakeup(struct serdev_device *serdev)
> +{
> + struct hci_uart *hu = serdev_device_get_drvdata(serdev);
> +
> + BT_DBG("");
> +
> + if (!hu || serdev != hu->serdev) {
> + WARN_ON(1);
> + return;
> + }
> +
> + if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
> + hci_uart_tx_wakeup(hu);
> +}
> +
> +/** hci_uart_receive_buf - receive buffer wakeup
> + * @serdev: serial device
> + * @data:   pointer to received data
> + * @count:  count of received data in bytes
> + *
> + * This function is called by the serdev framework when it received data
> + * in the RX buffer.
> + *
> + * Return: number of processed bytes
> + */
> +static int hci_uart_receive_buf(struct serdev_device *serdev, const u8 *data,
> +   size_t count)
> +{
> + struct hci_uart *hu = serdev_device_get_drvdata(serdev);
> +
> + if (!hu || serdev != hu->serdev) {
> + WARN_ON(1);
> + return 0;
> + }
> +
> + if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
> + return 0;
> +
> + /* It does not need a lock here as it is already protected by a mutex in
> + * tty caller
> + */
> + hu->proto->recv(hu, data, count);
> +
> + if (hu->hdev)
> + hu->hdev->stat.byte_rx += count;
> +
> + return count;
> +}
> +
> +struct serdev_device_ops hci_serdev_client_ops = {
> + .receive_buf = hci_uart_receive_buf,
> + .write_wakeup = hci_uart_write_wakeup,
> +};
> +
> +int hci_uart_register_device(struct hci_uart *hu,
> +     const struct hci_uart_proto *p)
> +{
> + int err;
> + struct hci_dev *hdev;
> +
> + BT_DBG("");
> +
> + err = p->open(hu);
> + if (err)
> + return err;
> +
> + hu->proto = p;
> + set_bit(HCI_UART_PROTO_READY, &hu->flags);
> +
> + /* Initialize and register HCI device */
> + hdev = hci_alloc_dev();
> + if (!hdev) {
> + BT_ERR("Can't allocate HCI device");
> + err = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + hu->hdev = hdev;
> +
> + hdev->bus = HCI_UART;
> + hci_set_drvdata(hdev, hu);
> +
> + INIT_WORK(&hu->write_work, hci_uart_write_work);
> +
> + /* Only when vendor specific setup callback is provided, consider
> + * the manufacturer information valid. This avoids filling in the
> + * value for Ericsson when nothing is specified.
> + */
> + if (hu->proto->setup)
> + hdev->manufacturer = hu->proto->manufacturer;
> +
> + hdev->open  = hci_uart_open;
> + hdev->close = hci_uart_close;
> + hdev->flush = hci_uart_flush;
> + hdev->send  = hci_uart_send_frame;
> + hdev->setup = hci_uart_setup;
> + SET_HCIDEV_DEV(hdev, &hu->serdev->dev);
> +
> + if (test_bit(HCI_UART_RAW_DEVICE, &hu->hdev_flags))
> + set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
> +
> + if (test_bit(HCI_UART_EXT_CONFIG, &hu->hdev_flags))
> + set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks);
> +
> + if (!test_bit(HCI_UART_RESET_ON_INIT, &hu->hdev_flags))
> + set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
> +
> + if (test_bit(HCI_UART_CREATE_AMP, &hu->hdev_flags))
> + hdev->dev_type = HCI_AMP;
> + else
> + hdev->dev_type = HCI_PRIMARY;
> +
> + if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
> + return 0;
> +
> + if (hci_register_dev(hdev) < 0) {
> + BT_ERR("Can't register HCI device");
> + err = -ENODEV;
> + goto err_register;
> + }
> +
> + set_bit(HCI_UART_REGISTERED, &hu->flags);
> +
> + return 0;
> +
> +err_register:
> + hci_free_dev(hdev);
> +err_alloc:
> + clear_bit(HCI_UART_PROTO_READY, &hu->flags);
> + p->close(hu);
> + return err;
> +}
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 82c92f1b65b4..168aaaa2e58c 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -55,6 +55,7 @@
>  #define HCI_UART_VND_DETECT 5
>  
>  struct hci_uart;
> +struct serdev_device;
>  
>  struct hci_uart_proto {
>   unsigned int id;
> @@ -74,6 +75,7 @@ struct hci_uart_proto {
>  
>  struct hci_uart {
>   struct tty_struct *tty;
> + struct serdev_device *serdev;

IIUC, you're wanting to bring this patch back to Xenial so that the
hci_uart_has_flow_control() function, which is added by the next patch,
works as expected. Here's the hunk of interest from the next patch:

=======================================================================
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 96bcec5598c2..8e4362361769 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -257,6 +257,19 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
  return 0;
 }
 
+/* Check the underlying device or tty has flow control support */
+bool hci_uart_has_flow_control(struct hci_uart *hu)
+{
+ /* serdev nodes check if the needed operations are present */
+ if (hu->serdev)
+ return true;
+
+ if (hu->tty->driver->ops->tiocmget && hu->tty->driver->ops->tiocmset)
+ return true;
+
+ return false;
+}
+
 /* Flow control or un-flow control the device */
 void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
 {
=======================================================================

The only dependence hci_uart_has_flow_control() has on this patch is
that it does a check on the serdev member of the hci_uart struct.
However, the serdev member wouldn't exist if we simply didn't apply this
patch. I think the better Xenial backport is to *not* bring back this
patch and to modify hci_uart_has_flow_control(), added by the next
patch, by dropping the check on hu->serdev.

Tyler

>   struct hci_dev *hdev;
>   unsigned long flags;
>   unsigned long hdev_flags;
> @@ -101,6 +103,8 @@ struct hci_uart {
>  
>  int hci_uart_register_proto(const struct hci_uart_proto *p);
>  int hci_uart_unregister_proto(const struct hci_uart_proto *p);
> +int hci_uart_register_device(struct hci_uart *hu, const struct hci_uart_proto *p);
> +
>  int hci_uart_tx_wakeup(struct hci_uart *hu);
>  int hci_uart_init_ready(struct hci_uart *hu);
>  void hci_uart_init_tty(struct hci_uart *hu);
> --
> 2.20.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK for Xenial: [X/D][SRU][CVE-2019-10207] check for missing tty operations

Connor Kuehl
In reply to this post by Connor Kuehl
On 8/12/19 4:08 PM, Connor Kuehl wrote:

> https://people.canonical.com/~ubuntu-security/cve/2019/CVE-2019-10207.html
>
>  From the fix description:
>
> "Certain ttys operations (pty_unix98_ops) lack tiocmget() and tiocmset()
> functions which are called by the certain HCI UART protocols (hci_ath,
> hci_bcm, hci_intel, hci_mrvl, hci_qca) via hci_uart_set_flow_control()
> or directly. This leads to an execution at NULL and can be triggered by
> an unprivileged user. Fix this by adding a check for the missing tty
> operations to the protocols which use them."
>
> Since the fix required the serdev patch for it to make its checks, I have
> included that commit for Xenial as well.
>
> Disco was a clean cherry pick.
>
> Rob Herring (1):
>    Bluetooth: hci_uart: add serdev driver support library
>
> Vladis Dronov (1):
>    Bluetooth: hci_uart: check for missing tty operations
>
>   drivers/bluetooth/Makefile     |   1 +
>   drivers/bluetooth/hci_ath.c    |   3 +
>   drivers/bluetooth/hci_bcm.c    |   3 +
>   drivers/bluetooth/hci_intel.c  |   3 +
>   drivers/bluetooth/hci_ldisc.c  |  13 ++
>   drivers/bluetooth/hci_qca.c    |   3 +
>   drivers/bluetooth/hci_serdev.c | 361 +++++++++++++++++++++++++++++++++
>   drivers/bluetooth/hci_uart.h   |   5 +
>   8 files changed, 392 insertions(+)
>   create mode 100644 drivers/bluetooth/hci_serdev.c
>

I will send a V2.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team