diff mbox series

Linux v5.4.199: Bluetooth: hci_event: Ignore multiple conn complete events

Message ID SJ0PR13MB575532A88E75738299CB1636FEB49@SJ0PR13MB5755.namprd13.prod.outlook.com
State New
Headers show
Series Linux v5.4.199: Bluetooth: hci_event: Ignore multiple conn complete events | expand

Commit Message

John Klug June 24, 2022, 10:46 p.m. UTC
This patch updated for the 5.4.199 kernel on Friday, 24 June 2022 by
John Klug <john.klug@multitech.com>

From: Soenke Huster <soenke.huster@eknoes.de>
Date: Sun, 23 Jan 2022 15:06:24 +0100
Subject: Bluetooth: hci_event: Ignore multiple conn complete events

When one of the three connection complete events is received multiple
times for the same handle, the device is registered multiple times which
leads to memory corruptions. Therefore, consequent events for a single
connection are ignored.

The conn->state can hold different values, therefore HCI_CONN_HANDLE_UNSET
is introduced to identify new connections. To make sure the events do not
contain this or another invalid handle HCI_CONN_HANDLE_MAX and checks
are introduced.

---
 include/net/bluetooth/hci_core.h |  3 ++
 net/bluetooth/hci_conn.c         |  1 +
 net/bluetooth/hci_event.c        | 63 ++++++++++++++++++++++++++++++----------
 3 files changed, 52 insertions(+), 15 deletions(-)

Comments

bluez.test.bot@gmail.com June 24, 2022, 11:45 p.m. UTC | #1
This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----
error: corrupt patch at line 11
hint: Use 'git am --show-current-patch' to see the failed patch


Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth
Soenke Huster June 29, 2022, 6:33 a.m. UTC | #2
Hi John,

On 25.06.22 00:46, John Klug wrote:
> This patch updated for the 5.4.199 kernel on Friday, 24 June 2022 by
> John Klug <john.klug@multitech.com>

Thanks for backporting! I do not know how the process is for backporting patches to stable releases - do you need my Tested-by or Reviewed-by or similar? I'll test it in the afternoon.

Furthermore, I think it must go to stable, and there seem to be some guidance how that works over here:
https://www.kernel.org/doc/html/v4.17/process/stable-kernel-rules.html#option-3

Best
Sönke
Luiz Augusto von Dentz July 7, 2022, 8:26 p.m. UTC | #3
Hi John,

On Tue, Jun 28, 2022 at 11:35 PM Sönke Huster <soenke.huster@eknoes.de> wrote:
>
> Hi John,
>
> On 25.06.22 00:46, John Klug wrote:
> > This patch updated for the 5.4.199 kernel on Friday, 24 June 2022 by
> > John Klug <john.klug@multitech.com>
>
> Thanks for backporting! I do not know how the process is for backporting patches to stable releases - do you need my Tested-by or Reviewed-by or similar? I'll test it in the afternoon.
>
> Furthermore, I think it must go to stable, and there seem to be some guidance how that works over here:
> https://www.kernel.org/doc/html/v4.17/process/stable-kernel-rules.html#option-3

This probably should be sent to stable tree directly, at least our CI
won't be able to test it as it expects to be applied to
bluetooth-next.
diff mbox series

Patch

diff -Naru a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
--- a/include/net/bluetooth/hci_core.h	2022-06-24 09:07:33.521766338 -0500
+++ b/include/net/bluetooth/hci_core.h	2022-06-24 09:16:20.317754010 -0500
@@ -193,6 +193,9 @@ 
 
 #define HCI_MAX_SHORT_NAME_LENGTH	10
 
+#define HCI_CONN_HANDLE_UNSET		0xffff
+#define HCI_CONN_HANDLE_MAX		0x0eff
+
 /* Min encryption key size to match with SMP */
 #define HCI_MIN_ENC_KEY_SIZE		7
 
diff -Naru a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
--- a/net/bluetooth/hci_conn.c	2022-06-24 09:08:47.105764616 -0500
+++ b/net/bluetooth/hci_conn.c	2022-06-24 09:16:20.317754010 -0500
@@ -504,6 +504,7 @@ 
 
 	bacpy(&conn->dst, dst);
 	bacpy(&conn->src, &hdev->bdaddr);
+	conn->handle = HCI_CONN_HANDLE_UNSET;
 	conn->hdev  = hdev;
 	conn->type  = type;
 	conn->role  = role;
diff -Naru a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
--- a/net/bluetooth/hci_event.c	2022-06-24 09:09:10.825764061 -0500
+++ b/net/bluetooth/hci_event.c	2022-06-24 09:19:52.017749056 -0500
@@ -2494,6 +2494,11 @@ 
 	struct hci_ev_conn_complete *ev = (void *) skb->data;
 	struct hci_conn *conn;
 
+	if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
+		BT_DBG("Ignoring HCI_Connection_Complete for invalid handle");
+		return;
+	}
+        
 	BT_DBG("%s", hdev->name);
 
 	hci_dev_lock(hdev);
@@ -2510,6 +2515,17 @@ 
 		conn->type = SCO_LINK;
 	}
 
+	/* The HCI_Connection_Complete event is only sent once per connection.
+	 * Processing it more than once per connection can corrupt kernel memory.
+	 *
+	 * As the connection handle is set here for the first time, it indicates
+	 * whether the connection is already set up.
+	 */
+	if (conn->handle != HCI_CONN_HANDLE_UNSET) {
+		BT_DBG("Ignoring HCI_Connection_Complete for existing connection");
+		goto unlock;
+	}
+
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
 
@@ -4177,6 +4193,11 @@ 
 	struct hci_ev_sync_conn_complete *ev = (void *) skb->data;
 	struct hci_conn *conn;
 
+	if (__le16_to_cpu(ev->handle) > HCI_CONN_HANDLE_MAX) {
+		BT_DBG("Ignoring HCI_Sync_Conn_Complete event for invalid handle");
+		return;
+	}
+
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
 	hci_dev_lock(hdev);
@@ -4200,23 +4221,19 @@ 
 			goto unlock;
 	}
 
+	/* The HCI_Synchronous_Connection_Complete event is only sent once per connection.
+	 * Processing it more than once per connection can corrupt kernel memory.
+	 *
+	 * As the connection handle is set here for the first time, it indicates
+	 * whether the connection is already set up.
+	 */
+	if (conn->handle != HCI_CONN_HANDLE_UNSET) {
+	    BT_DBG("Ignoring HCI_Sync_Conn_Complete event for existing connection");
+		goto unlock;
+	}
+
 	switch (ev->status) {
 	case 0x00:
-		/* The synchronous connection complete event should only be
-		 * sent once per new connection. Receiving a successful
-		 * complete event when the connection status is already
-		 * BT_CONNECTED means that the device is misbehaving and sent
-		 * multiple complete event packets for the same new connection.
-		 *
-		 * Registering the device more than once can corrupt kernel
-		 * memory, hence upon detecting this invalid event, we report
-		 * an error and ignore the packet.
-		 */
-		if (conn->state == BT_CONNECTED) {
-			bt_dev_err(hdev, "Ignoring connect complete event for existing connection");
-			goto unlock;
-		}
-
 		conn->handle = __le16_to_cpu(ev->handle);
 		conn->state  = BT_CONNECTED;
 		conn->type   = ev->link_type;
@@ -4985,6 +5002,11 @@ 
 	struct smp_irk *irk;
 	u8 addr_type;
 
+	if (handle > HCI_CONN_HANDLE_MAX) {
+		BT_DBG("Ignoring HCI_LE_Connection_Complete for invalid handle");
+		return;
+	}
+
 	hci_dev_lock(hdev);
 
 	/* All controllers implicitly stop advertising in the event of a
@@ -5026,6 +5048,17 @@ 
 		cancel_delayed_work(&conn->le_conn_timeout);
 	}
 
+	/* The HCI_LE_Connection_Complete event is only sent once per connection.
+	 * Processing it more than once per connection can corrupt kernel memory.
+	 *
+	 * As the connection handle is set here for the first time, it indicates
+	 * whether the connection is already set up.
+	 */
+	if (conn->handle != HCI_CONN_HANDLE_UNSET) {
+		BT_DBG("Ignoring HCI_Connection_Complete for existing connection");
+		goto unlock;
+	}
+
 	le_conn_update_addr(conn, bdaddr, bdaddr_type, local_rpa);
 
 	/* Lookup the identity address from the stored connection