[Xenial,SRU,CVE-2019-10207,v2] Bluetooth: hci_uart: check for missing tty operations
diff mbox series

Message ID 20190813172639.23747-1-connor.kuehl@canonical.com
State New
Headers show
Series
  • [Xenial,SRU,CVE-2019-10207,v2] Bluetooth: hci_uart: check for missing tty operations
Related show

Commit Message

Connor Kuehl Aug. 13, 2019, 5:26 p.m. UTC
From: Vladis Dronov <vdronov@redhat.com>

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: syzbot+79337b501d6aa974d0f6@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org # 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 <vdronov@redhat.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: Yu-Chen, Cho <acho@suse.com>
Tested-by: Yu-Chen, Cho <acho@suse.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(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. The check on the
  'serdev' data member was also dropped from hci_uart_has_flow_control()
  because it doesn't exist in Xenial and there is no reason to backport
  the serdev commit(s) since this fix has no functional dependency on
  serdev (none of its functions are called). ]
Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>
---
v1 -> v2:
 * Dropped the commit that introduced serdev which let me:
 * Drop the if statement checking for the serdev data member on
   the hci_uart structure

 drivers/bluetooth/hci_ath.c   | 3 +++
 drivers/bluetooth/hci_bcm.c   | 3 +++
 drivers/bluetooth/hci_intel.c | 3 +++
 drivers/bluetooth/hci_ldisc.c | 9 +++++++++
 drivers/bluetooth/hci_qca.c   | 3 +++
 drivers/bluetooth/hci_uart.h  | 1 +
 6 files changed, 22 insertions(+)

Comments

Tyler Hicks Aug. 14, 2019, 11:05 p.m. UTC | #1
On 2019-08-13 10:26:39, Connor Kuehl wrote:
> From: Vladis Dronov <vdronov@redhat.com>
> 
> 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: syzbot+79337b501d6aa974d0f6@syzkaller.appspotmail.com
> Cc: stable@vger.kernel.org # 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 <vdronov@redhat.com>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> Reviewed-by: Yu-Chen, Cho <acho@suse.com>
> Tested-by: Yu-Chen, Cho <acho@suse.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (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. The check on the
>   'serdev' data member was also dropped from hci_uart_has_flow_control()
>   because it doesn't exist in Xenial and there is no reason to backport
>   the serdev commit(s) since this fix has no functional dependency on
>   serdev (none of its functions are called). ]
> Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com>

Thanks for v2!

 Acked-by: Tyler Hicks <tyhicks@canonical.com>

Tyler

> ---
> v1 -> v2:
>  * Dropped the commit that introduced serdev which let me:
>  * Drop the if statement checking for the serdev data member on
>    the hci_uart structure
> 
>  drivers/bluetooth/hci_ath.c   | 3 +++
>  drivers/bluetooth/hci_bcm.c   | 3 +++
>  drivers/bluetooth/hci_intel.c | 3 +++
>  drivers/bluetooth/hci_ldisc.c | 9 +++++++++
>  drivers/bluetooth/hci_qca.c   | 3 +++
>  drivers/bluetooth/hci_uart.h  | 1 +
>  6 files changed, 22 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..d5db2332eb6f 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -257,6 +257,15 @@ 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)
> +{
> +	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 82c92f1b65b4..ce00c02eb63f 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -105,6 +105,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
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch
diff mbox series

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..d5db2332eb6f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -257,6 +257,15 @@  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)
+{
+	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 82c92f1b65b4..ce00c02eb63f 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -105,6 +105,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);