Commit a2ac591c authored by Ruihan Li's avatar Ruihan Li Committed by Luiz Augusto von Dentz
Browse files

Bluetooth: Fix UAF in hci_conn_hash_flush again 



Commit 06149746 ("Bluetooth: hci_conn: Add support for linking
multiple hcon") reintroduced a previously fixed bug [1] ("KASAN:
slab-use-after-free Read in hci_conn_hash_flush"). This bug was
originally fixed by commit 5dc7d23e ("Bluetooth: hci_conn: Fix
possible UAF").

The hci_conn_unlink function was added to avoid invalidating the link
traversal caused by successive hci_conn_del operations releasing extra
connections. However, currently hci_conn_unlink itself also releases
extra connections, resulted in the reintroduced bug.

This patch follows a more robust solution for cleaning up all
connections, by repeatedly removing the first connection until there are
none left. This approach does not rely on the inner workings of
hci_conn_del and ensures proper cleanup of all connections.

Meanwhile, we need to make sure that hci_conn_del never fails. Indeed it
doesn't, as it now always returns zero. To make this a bit clearer, this
patch also changes its return type to void.

Reported-by: default avatar <syzbot+8bb72f86fc823817bc5d@syzkaller.appspotmail.com>
Closes: https://lore.kernel.org/linux-bluetooth/000000000000aa920505f60d25ad@google.com/


Fixes: 06149746 ("Bluetooth: hci_conn: Add support for linking multiple hcon")
Signed-off-by: default avatarRuihan Li <lrh2000@pku.edu.cn>
Co-developed-by: default avatarLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
Signed-off-by: default avatarLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
parent 2910431a

include/net/bluetooth/hci_core.h

+1 −1
Original line number Diff line number Diff line
@@ -1327,7 +1327,7 @@ int hci_le_create_cis(struct hci_conn *conn); 


struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,

			      u8 role);

int hci_conn_del(struct hci_conn *conn);

void hci_conn_del(struct hci_conn *conn);

void hci_conn_hash_flush(struct hci_dev *hdev);

void hci_conn_check_pending(struct hci_dev *hdev);

net/bluetooth/hci_conn.c

+22 −11
Original line number Diff line number Diff line
@@ -1088,6 +1088,14 @@ static void hci_conn_unlink(struct hci_conn *conn) 


			hci_conn_unlink(child);



			/* If hdev is down it means

			 * hci_dev_close_sync/hci_conn_hash_flush is in progress

			 * and links don't need to be cleanup as all connections

			 * would be cleanup.

			 */

			if (!test_bit(HCI_UP, &hdev->flags))

				continue;



			/* Due to race, SCO connection might be not established

			 * yet at this point. Delete it now, otherwise it is

			 * possible for it to be stuck and can't be deleted.
@@ -1112,7 +1120,7 @@ static void hci_conn_unlink(struct hci_conn *conn) 
	conn->link = NULL;

}



int hci_conn_del(struct hci_conn *conn)

void hci_conn_del(struct hci_conn *conn)

{

	struct hci_dev *hdev = conn->hdev;
@@ -1163,8 +1171,6 @@ int hci_conn_del(struct hci_conn *conn) 
	 * rest of hci_conn_del.

	 */

	hci_conn_cleanup(conn);



	return 0;

}



struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
@@ -2465,22 +2471,27 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active) 
/* Drop all connection on the device */

void hci_conn_hash_flush(struct hci_dev *hdev)

{

	struct hci_conn_hash *h = &hdev->conn_hash;

	struct hci_conn *c, *n;

	struct list_head *head = &hdev->conn_hash.list;

	struct hci_conn *conn;



	BT_DBG("hdev %s", hdev->name);



	list_for_each_entry_safe(c, n, &h->list, list) {

		c->state = BT_CLOSED;



		hci_disconn_cfm(c, HCI_ERROR_LOCAL_HOST_TERM);

	/* We should not traverse the list here, because hci_conn_del

	 * can remove extra links, which may cause the list traversal

	 * to hit items that have already been released.

	 */

	while ((conn = list_first_entry_or_null(head,

						struct hci_conn,

						list)) != NULL) {

		conn->state = BT_CLOSED;

		hci_disconn_cfm(conn, HCI_ERROR_LOCAL_HOST_TERM);



		/* Unlink before deleting otherwise it is possible that

		 * hci_conn_del removes the link which may cause the list to

		 * contain items already freed.

		 */

		hci_conn_unlink(c);

		hci_conn_del(c);

		hci_conn_unlink(conn);

		hci_conn_del(conn);

	}

}