* [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync
@ 2024-02-13 21:31 Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 2/3] Bluetooth: hci_sync: Add helper functions to manipulate cmd_sync queue Luiz Augusto von Dentz
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2024-02-13 21:31 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This fixes the UAF on __hci_acl_create_connection_sync caused by
connection abortion, it uses the same logic as to LE_LINK which uses
hci_cmd_sync_cancel to prevent the callback to run if the connection is
abort prematurely.
Reported-by: syzbot+3f0a39be7a2035700868@syzkaller.appspotmail.com
Fixes: 456561ba8e49 ("Bluetooth: hci_conn: Only do ACL connections sequentially")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/hci_sync.h | 3 +--
net/bluetooth/hci_conn.c | 3 ++-
net/bluetooth/hci_sync.c | 16 ++++++++++------
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 824660f8f30d..ed334c253ebc 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -139,5 +139,4 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
-int hci_acl_create_connection_sync(struct hci_dev *hdev,
- struct hci_conn *conn);
+int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 8164502234c5..587eb27f374c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1645,7 +1645,7 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
acl->auth_type = auth_type;
acl->conn_timeout = timeout;
- err = hci_acl_create_connection_sync(hdev, acl);
+ err = hci_connect_acl_sync(hdev, acl);
if (err) {
hci_conn_del(acl);
return ERR_PTR(err);
@@ -2942,6 +2942,7 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
*/
if (conn->state == BT_CONNECT && hdev->req_status == HCI_REQ_PEND) {
switch (hci_skb_event(hdev->sent_cmd)) {
+ case HCI_EV_CONN_COMPLETE:
case HCI_EV_LE_CONN_COMPLETE:
case HCI_EV_LE_ENHANCED_CONN_COMPLETE:
case HCI_EVT_LE_CIS_ESTABLISHED:
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 788a889210d8..e1fdcb3c2706 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6493,13 +6493,18 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
UINT_PTR(instance), NULL);
}
-static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
+static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn = data;
+ struct hci_conn *conn;
+ u16 handle = PTR_UINT(data);
struct inquiry_entry *ie;
struct hci_cp_create_conn cp;
int err;
+ conn = hci_conn_hash_lookup_handle(hdev, handle);
+ if (!conn)
+ return 0;
+
/* Many controllers disallow HCI Create Connection while it is doing
* HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
* Connection. This may cause the MGMT discovering state to become false
@@ -6556,9 +6561,8 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
return err;
}
-int hci_acl_create_connection_sync(struct hci_dev *hdev,
- struct hci_conn *conn)
+int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
{
- return hci_cmd_sync_queue(hdev, __hci_acl_create_connection_sync,
- conn, NULL);
+ return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync,
+ UINT_PTR(conn->handle), NULL);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] Bluetooth: hci_sync: Add helper functions to manipulate cmd_sync queue
2024-02-13 21:31 [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync Luiz Augusto von Dentz
@ 2024-02-13 21:31 ` Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt Luiz Augusto von Dentz
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2024-02-13 21:31 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds functions to queue, dequeue and lookup into the cmd_sync
list.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/hci_sync.h | 12 +++
net/bluetooth/hci_sync.c | 132 +++++++++++++++++++++++++++++--
2 files changed, 136 insertions(+), 8 deletions(-)
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index ed334c253ebc..4ff4aa68ee19 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -48,6 +48,18 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
+struct hci_cmd_sync_work_entry *
+hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy);
+int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy);
+void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
+ struct hci_cmd_sync_work_entry *entry);
+bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy);
+bool hci_cmd_sync_dequeue_once(struct hci_dev *hdev,
+ hci_cmd_sync_work_func_t func, void *data,
+ hci_cmd_sync_work_destroy_t destroy);
int hci_update_eir_sync(struct hci_dev *hdev);
int hci_update_class_sync(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index e1fdcb3c2706..5b314bf844f8 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -566,6 +566,17 @@ void hci_cmd_sync_init(struct hci_dev *hdev)
INIT_DELAYED_WORK(&hdev->adv_instance_expire, adv_timeout_expire);
}
+static void _hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
+ struct hci_cmd_sync_work_entry *entry,
+ int err)
+{
+ if (entry->destroy)
+ entry->destroy(hdev, entry->data, err);
+
+ list_del(&entry->list);
+ kfree(entry);
+}
+
void hci_cmd_sync_clear(struct hci_dev *hdev)
{
struct hci_cmd_sync_work_entry *entry, *tmp;
@@ -574,13 +585,8 @@ void hci_cmd_sync_clear(struct hci_dev *hdev)
cancel_work_sync(&hdev->reenable_adv_work);
mutex_lock(&hdev->cmd_sync_work_lock);
- list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) {
- if (entry->destroy)
- entry->destroy(hdev, entry->data, -ECANCELED);
-
- list_del(&entry->list);
- kfree(entry);
- }
+ list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list)
+ _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
mutex_unlock(&hdev->cmd_sync_work_lock);
}
@@ -669,6 +675,115 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
}
EXPORT_SYMBOL(hci_cmd_sync_queue);
+static struct hci_cmd_sync_work_entry *
+_hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+ struct hci_cmd_sync_work_entry *entry, *tmp;
+
+ list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) {
+ if (func && entry->func != func)
+ continue;
+
+ if (data && entry->data != data)
+ continue;
+
+ if (destroy && entry->destroy != destroy)
+ continue;
+
+ return entry;
+ }
+
+ return NULL;
+}
+
+/* Queue HCI command entry once:
+ *
+ * - Lookup if an entry already exist and only if it doesn't creates a new entry
+ * and queue it.
+ */
+int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+ if (hci_cmd_sync_lookup_entry(hdev, func, data, destroy))
+ return 0;
+
+ return hci_cmd_sync_queue(hdev, func, data, destroy);
+}
+EXPORT_SYMBOL(hci_cmd_sync_queue_once);
+
+/* Lookup HCI command entry:
+ *
+ * - Return first entry that matches by function callback or data or
+ * destroy callback.
+ */
+struct hci_cmd_sync_work_entry *
+hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+ struct hci_cmd_sync_work_entry *entry;
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ entry = _hci_cmd_sync_lookup_entry(hdev, func, data, destroy);
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ return entry;
+}
+EXPORT_SYMBOL(hci_cmd_sync_lookup_entry);
+
+/* Cancel HCI command entry */
+void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
+ struct hci_cmd_sync_work_entry *entry)
+{
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+}
+EXPORT_SYMBOL(hci_cmd_sync_cancel_entry);
+
+/* Dequeue one HCI command entry:
+ *
+ * - Lookup and cancel first entry that matches.
+ */
+bool hci_cmd_sync_dequeue_once(struct hci_dev *hdev,
+ hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+ struct hci_cmd_sync_work_entry *entry;
+
+ entry = hci_cmd_sync_lookup_entry(hdev, func, data, destroy);
+ if (!entry)
+ return false;
+
+ hci_cmd_sync_cancel_entry(hdev, entry);
+
+ return true;
+}
+EXPORT_SYMBOL(hci_cmd_sync_dequeue_once);
+
+/* Dequeue HCI command entry:
+ *
+ * - Lookup and cancel any entry that matches by function callback or data or
+ * destroy callback.
+ */
+bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+ struct hci_cmd_sync_work_entry *entry;
+ bool ret = false;
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ while ((entry = _hci_cmd_sync_lookup_entry(hdev, func, data,
+ destroy))) {
+ _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
+ ret = true;
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(hci_cmd_sync_dequeue);
+
int hci_update_eir_sync(struct hci_dev *hdev)
{
struct hci_cp_write_eir cp;
@@ -2881,7 +2996,8 @@ int hci_update_passive_scan(struct hci_dev *hdev)
hci_dev_test_flag(hdev, HCI_UNREGISTER))
return 0;
- return hci_cmd_sync_queue(hdev, update_passive_scan_sync, NULL, NULL);
+ return hci_cmd_sync_queue_once(hdev, update_passive_scan_sync, NULL,
+ NULL);
}
int hci_write_sc_support_sync(struct hci_dev *hdev, u8 val)
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt
2024-02-13 21:31 [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 2/3] Bluetooth: hci_sync: Add helper functions to manipulate cmd_sync queue Luiz Augusto von Dentz
@ 2024-02-13 21:31 ` Luiz Augusto von Dentz
2024-02-13 23:47 ` Jonas Dreßler
2024-02-13 21:59 ` [v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync bluez.test.bot
2024-02-15 21:20 ` [PATCH v2 1/3] " patchwork-bot+bluetooth
3 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2024-02-13 21:31 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
If connection is still queued/pending in the cmd_sync queue it means no
command has been generated and it should be safe to just dequeue the
callback when it is being aborted.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/hci_core.h | 19 ++++++++
include/net/bluetooth/hci_sync.h | 10 +++--
net/bluetooth/hci_conn.c | 70 ++++++------------------------
net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++----
4 files changed, 102 insertions(+), 71 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2bdea85b7c44..317d495cfcf5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev)
return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num;
}
+static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn)
+{
+ struct hci_conn_hash *h = &hdev->conn_hash;
+ struct hci_conn *c;
+
+ rcu_read_lock();
+
+ list_for_each_entry_rcu(c, &h->list, list) {
+ if (c == conn) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle)
{
struct hci_conn_hash *h = &hdev->conn_hash;
@@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, bool dst_resolved, u8 sec_level,
u16 conn_timeout, u8 role);
+void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
u8 sec_level, u8 auth_type,
enum conn_reasons conn_reason, u16 timeout);
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 4ff4aa68ee19..6a9d063e9f47 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
+int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy);
struct hci_cmd_sync_work_entry *
hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
-int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
- void *data, hci_cmd_sync_work_destroy_t destroy);
void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
struct hci_cmd_sync_work_entry *entry);
bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
@@ -139,8 +139,6 @@ struct hci_conn;
int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason);
-int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn);
-
int hci_le_create_cis_sync(struct hci_dev *hdev);
int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle);
@@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn);
+
+int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn);
+
+int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 587eb27f374c..21e0b4064d05 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = {
};
/* This function requires the caller holds hdev->lock */
-static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
+void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
{
struct hci_conn_params *params;
struct hci_dev *hdev = conn->hdev;
@@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn)
* rest of hci_conn_del.
*/
hci_conn_cleanup(conn);
+
+ /* Dequeue callbacks using connection pointer as data */
+ hci_cmd_sync_dequeue(hdev, NULL, conn, NULL);
}
struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
@@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
return 0;
}
-static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
-{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return;
-
- bt_dev_dbg(hdev, "err %d", err);
-
- hci_dev_lock(hdev);
-
- if (!err) {
- hci_connect_le_scan_cleanup(conn, 0x00);
- goto done;
- }
-
- /* Check if connection is still pending */
- if (conn != hci_lookup_le_connect(hdev))
- goto done;
-
- /* Flush to make sure we send create conn cancel command if needed */
- flush_delayed_work(&conn->le_conn_timeout);
- hci_conn_failed(conn, bt_status(err));
-
-done:
- hci_dev_unlock(hdev);
-}
-
-static int hci_connect_le_sync(struct hci_dev *hdev, void *data)
-{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
-
- bt_dev_dbg(hdev, "conn %p", conn);
-
- clear_bit(HCI_CONN_SCANNING, &conn->flags);
- conn->state = BT_CONNECT;
-
- return hci_le_create_conn_sync(hdev, conn);
-}
-
struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, bool dst_resolved, u8 sec_level,
u16 conn_timeout, u8 role)
@@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
conn->sec_level = BT_SECURITY_LOW;
conn->conn_timeout = conn_timeout;
- err = hci_cmd_sync_queue(hdev, hci_connect_le_sync,
- UINT_PTR(conn->handle),
- create_le_conn_complete);
+ err = hci_connect_le_sync(hdev, conn);
if (err) {
hci_conn_del(conn);
return ERR_PTR(err);
@@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
static int abort_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
+ struct hci_conn *conn = data;
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
+ if (!hci_conn_valid(hdev, conn))
+ return -ECANCELED;
return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
}
@@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
hci_cmd_sync_cancel(hdev, -ECANCELED);
break;
}
+ /* Cancel connect attempt if still queued/pending */
+ } else if (!hci_cancel_connect_sync(hdev, conn)) {
+ return 0;
}
- return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle),
- NULL);
+ return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
}
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 5b314bf844f8..b7d8e99e2a30 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
conn->conn_timeout, NULL);
}
-int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
+static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
{
struct hci_cp_le_create_conn cp;
struct hci_conn_params *params;
u8 own_addr_type;
int err;
+ struct hci_conn *conn = data;
+
+ if (!hci_conn_valid(hdev, conn))
+ return -ECANCELED;
+
+ bt_dev_dbg(hdev, "conn %p", conn);
+
+ clear_bit(HCI_CONN_SCANNING, &conn->flags);
+ conn->state = BT_CONNECT;
/* If requested to connect as peripheral use directed advertising */
if (conn->role == HCI_ROLE_SLAVE) {
@@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
+ struct hci_conn *conn = data;
struct inquiry_entry *ie;
struct hci_cp_create_conn cp;
int err;
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
-
/* Many controllers disallow HCI Create Connection while it is doing
* HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
* Connection. This may cause the MGMT discovering state to become false
@@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
{
- return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync,
- UINT_PTR(conn->handle), NULL);
+ return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
+ NULL);
+}
+
+static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
+{
+ struct hci_conn *conn = data;
+
+ bt_dev_dbg(hdev, "err %d", err);
+
+ if (err == -ECANCELED)
+ return;
+
+ hci_dev_lock(hdev);
+
+ if (!err) {
+ hci_connect_le_scan_cleanup(conn, 0x00);
+ goto done;
+ }
+
+ /* Check if connection is still pending */
+ if (conn != hci_lookup_le_connect(hdev))
+ goto done;
+
+ /* Flush to make sure we send create conn cancel command if needed */
+ flush_delayed_work(&conn->le_conn_timeout);
+ hci_conn_failed(conn, bt_status(err));
+
+done:
+ hci_dev_unlock(hdev);
+}
+
+int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
+{
+ return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
+ create_le_conn_complete);
+}
+
+int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
+{
+ if (conn->state != BT_OPEN)
+ return -EINVAL;
+
+ switch (conn->type) {
+ case ACL_LINK:
+ return !hci_cmd_sync_dequeue_once(hdev,
+ hci_acl_create_conn_sync,
+ conn, NULL);
+ case LE_LINK:
+ return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync,
+ conn, create_le_conn_complete);
+ }
+
+ return -ENOENT;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync
2024-02-13 21:31 [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 2/3] Bluetooth: hci_sync: Add helper functions to manipulate cmd_sync queue Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt Luiz Augusto von Dentz
@ 2024-02-13 21:59 ` bluez.test.bot
2024-02-15 21:20 ` [PATCH v2 1/3] " patchwork-bot+bluetooth
3 siblings, 0 replies; 8+ messages in thread
From: bluez.test.bot @ 2024-02-13 21:59 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=825785
---Test result---
Test Summary:
CheckPatch FAIL 3.38 seconds
GitLint PASS 0.94 seconds
SubjectPrefix PASS 0.36 seconds
BuildKernel PASS 27.69 seconds
CheckAllWarning PASS 30.21 seconds
CheckSparse PASS 35.97 seconds
CheckSmatch PASS 98.24 seconds
BuildKernel32 PASS 26.93 seconds
TestRunnerSetup PASS 501.35 seconds
TestRunner_l2cap-tester FAIL 12.30 seconds
TestRunner_iso-tester PASS 31.44 seconds
TestRunner_bnep-tester PASS 4.92 seconds
TestRunner_mgmt-tester FAIL 171.41 seconds
TestRunner_rfcomm-tester PASS 7.45 seconds
TestRunner_sco-tester PASS 15.09 seconds
TestRunner_ioctl-tester PASS 7.92 seconds
TestRunner_mesh-tester PASS 5.93 seconds
TestRunner_smp-tester PASS 6.97 seconds
TestRunner_userchan-tester PASS 5.02 seconds
IncrementalBuild PASS 74.76 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#97:
Reported-by: syzbot+3f0a39be7a2035700868@syzkaller.appspotmail.com
Fixes: 456561ba8e49 ("Bluetooth: hci_conn: Only do ACL connections sequentially")
total: 0 errors, 1 warnings, 0 checks, 53 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13555669.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
No test result found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 486 (98.8%), Failed: 5, Not Run: 1
Failed Test Cases
LL Privacy - Add Device 5 (2 Devices to RL) Failed 0.119 seconds
LL Privacy - Add Device 6 (RL is full) Failed 0.143 seconds
LL Privacy - Remove Device 1 (Remove from AL) Timed out 2.199 seconds
LL Privacy - Remove Device 2 (Remove from RL) Timed out 1.999 seconds
LL Privacy - Remove Device 4 (Disable Adv) Timed out 1.839 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt
2024-02-13 21:31 ` [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt Luiz Augusto von Dentz
@ 2024-02-13 23:47 ` Jonas Dreßler
2024-02-14 18:44 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 8+ messages in thread
From: Jonas Dreßler @ 2024-02-13 23:47 UTC (permalink / raw)
To: luiz.dentz; +Cc: linux-bluetooth
Hi Luiz,
> If connection is still queued/pending in the cmd_sync queue it means no
> command has been generated and it should be safe to just dequeue the
> callback when it is being aborted.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 19 ++++++++
> include/net/bluetooth/hci_sync.h | 10 +++--
> net/bluetooth/hci_conn.c | 70 ++++++------------------------
> net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++----
> 4 files changed, 102 insertions(+), 71 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2bdea85b7c44..317d495cfcf5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev)
> return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num;
> }
>
> +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn)
> +{
> + struct hci_conn_hash *h = &hdev->conn_hash;
> + struct hci_conn *c;
> +
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(c, &h->list, list) {
> + if (c == conn) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle)
> {
> struct hci_conn_hash *h = &hdev->conn_hash;
> @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> u8 dst_type, bool dst_resolved, u8 sec_level,
> u16 conn_timeout, u8 role);
> +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
> struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> u8 sec_level, u8 auth_type,
> enum conn_reasons conn_reason, u16 timeout);
> diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
> index 4ff4aa68ee19..6a9d063e9f47 100644
> --- a/include/net/bluetooth/hci_sync.h
> +++ b/include/net/bluetooth/hci_sync.h
> @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> void *data, hci_cmd_sync_work_destroy_t destroy);
> int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> void *data, hci_cmd_sync_work_destroy_t destroy);
> +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> + void *data, hci_cmd_sync_work_destroy_t destroy);
> struct hci_cmd_sync_work_entry *
> hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> void *data, hci_cmd_sync_work_destroy_t destroy);
> -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> - void *data, hci_cmd_sync_work_destroy_t destroy);
> void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
> struct hci_cmd_sync_work_entry *entry);
> bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> @@ -139,8 +139,6 @@ struct hci_conn;
>
> int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason);
>
> -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn);
> -
> int hci_le_create_cis_sync(struct hci_dev *hdev);
>
> int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle);
> @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
> int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
>
> int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn);
> +
> +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn);
> +
> +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 587eb27f374c..21e0b4064d05 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = {
> };
>
> /* This function requires the caller holds hdev->lock */
> -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
> +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
> {
> struct hci_conn_params *params;
> struct hci_dev *hdev = conn->hdev;
> @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn)
> * rest of hci_conn_del.
> */
> hci_conn_cleanup(conn);
> +
> + /* Dequeue callbacks using connection pointer as data */
> + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL);
> }
>
> struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
> @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
> return 0;
> }
>
> -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
> -{
> - struct hci_conn *conn;
> - u16 handle = PTR_UINT(data);
> -
> - conn = hci_conn_hash_lookup_handle(hdev, handle);
> - if (!conn)
> - return;
> -
> - bt_dev_dbg(hdev, "err %d", err);
> -
> - hci_dev_lock(hdev);
> -
> - if (!err) {
> - hci_connect_le_scan_cleanup(conn, 0x00);
> - goto done;
> - }
> -
> - /* Check if connection is still pending */
> - if (conn != hci_lookup_le_connect(hdev))
> - goto done;
> -
> - /* Flush to make sure we send create conn cancel command if needed */
> - flush_delayed_work(&conn->le_conn_timeout);
> - hci_conn_failed(conn, bt_status(err));
> -
> -done:
> - hci_dev_unlock(hdev);
> -}
> -
> -static int hci_connect_le_sync(struct hci_dev *hdev, void *data)
> -{
> - struct hci_conn *conn;
> - u16 handle = PTR_UINT(data);
> -
> - conn = hci_conn_hash_lookup_handle(hdev, handle);
> - if (!conn)
> - return 0;
> -
> - bt_dev_dbg(hdev, "conn %p", conn);
> -
> - clear_bit(HCI_CONN_SCANNING, &conn->flags);
> - conn->state = BT_CONNECT;
> -
> - return hci_le_create_conn_sync(hdev, conn);
> -}
> -
> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> u8 dst_type, bool dst_resolved, u8 sec_level,
> u16 conn_timeout, u8 role)
> @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> conn->sec_level = BT_SECURITY_LOW;
> conn->conn_timeout = conn_timeout;
>
Might want to add a
+ if (conn->state != BT_OPEN && conn->state != BT_CLOSED)
+ return conn;
before setting the conn->dst_type while at it, similar to how it's
in hci_connect_acl().
> - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync,
> - UINT_PTR(conn->handle),
> - create_le_conn_complete);
> + err = hci_connect_le_sync(hdev, conn);
> if (err) {
> hci_conn_del(conn);
> return ERR_PTR(err);
> @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
>
> static int abort_conn_sync(struct hci_dev *hdev, void *data)
> {
> - struct hci_conn *conn;
> - u16 handle = PTR_UINT(data);
> + struct hci_conn *conn = data;
>
> - conn = hci_conn_hash_lookup_handle(hdev, handle);
> - if (!conn)
> - return 0;
> + if (!hci_conn_valid(hdev, conn))
> + return -ECANCELED;
>
> return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> }
> @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
> hci_cmd_sync_cancel(hdev, -ECANCELED);
> break;
> }
> + /* Cancel connect attempt if still queued/pending */
> + } else if (!hci_cancel_connect_sync(hdev, conn)) {
> + return 0;
> }
>
> - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle),
> - NULL);
> + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
> }
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 5b314bf844f8..b7d8e99e2a30 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
> conn->conn_timeout, NULL);
> }
>
> -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
> {
> struct hci_cp_le_create_conn cp;
> struct hci_conn_params *params;
> u8 own_addr_type;
> int err;
> + struct hci_conn *conn = data;
> +
> + if (!hci_conn_valid(hdev, conn))
> + return -ECANCELED;
> +
> + bt_dev_dbg(hdev, "conn %p", conn);
> +
> + clear_bit(HCI_CONN_SCANNING, &conn->flags);
> + conn->state = BT_CONNECT;
>
> /* If requested to connect as peripheral use directed advertising */
> if (conn->role == HCI_ROLE_SLAVE) {
> @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
>
> static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
> {
> - struct hci_conn *conn;
> - u16 handle = PTR_UINT(data);
> + struct hci_conn *conn = data;
> struct inquiry_entry *ie;
> struct hci_cp_create_conn cp;
> int err;
>
> - conn = hci_conn_hash_lookup_handle(hdev, handle);
> - if (!conn)
> - return 0;
> -
> /* Many controllers disallow HCI Create Connection while it is doing
> * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
> * Connection. This may cause the MGMT discovering state to become false
> @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
>
> int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
> {
> - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync,
> - UINT_PTR(conn->handle), NULL);
> + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
> + NULL);
> +}
> +
> +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
> +{
> + struct hci_conn *conn = data;
> +
> + bt_dev_dbg(hdev, "err %d", err);
> +
> + if (err == -ECANCELED)
> + return;
> +
> + hci_dev_lock(hdev);
> +
> + if (!err) {
> + hci_connect_le_scan_cleanup(conn, 0x00);
> + goto done;
> + }
> +
> + /* Check if connection is still pending */
> + if (conn != hci_lookup_le_connect(hdev))
> + goto done;
> +
> + /* Flush to make sure we send create conn cancel command if needed */
> + flush_delayed_work(&conn->le_conn_timeout);
> + hci_conn_failed(conn, bt_status(err));
> +
> +done:
> + hci_dev_unlock(hdev);
> +}
> +
> +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
> +{
> + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
> + create_le_conn_complete);
> +}
> +
> +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
> +{
> + if (conn->state != BT_OPEN)
> + return -EINVAL;
> +
> + switch (conn->type) {
> + case ACL_LINK:
> + return !hci_cmd_sync_dequeue_once(hdev,
> + hci_acl_create_conn_sync,
> + conn, NULL);
> + case LE_LINK:
> + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync,
> + conn, create_le_conn_complete);
> + }
> +
> + return -ENOENT;
> }
> --
> 2.43.0
Thanks for sending those patches, this is pretty much exactly what I had in mind!
I came up with a slightly different cancel function for the queued work, one that
also cancels the ongoing work. I'm not sure if it makes too much sense, because it
means adding careful -ECANCELED handling to those sync_work callbacks.
The nice thing about it is that it should also allow getting rid of the
hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn().
Adding the patch below, feel free to reuse whatever you like!
Cheers,
Jonas
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 067d445701..a2b14f6be1 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
bt_dev_dbg(hdev, "Opcode 0x%4x", opcode);
+ if (hdev->cur_hci_sync_work_cancelled) {
+ hdev->cur_hci_sync_work_cancelled = false;
+
+ return ERR_PTR(-ECANCELED);
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
hci_req_init(&req, hdev);
hci_cmd_sync_add(&req, opcode, plen, param, event, sk);
@@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work)
int err;
hci_req_sync_lock(hdev);
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ hdev->cur_hci_sync_work_func = entry->func;
+ hdev->cur_hci_sync_work_data = entry->data;
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
err = entry->func(hdev, entry->data);
if (entry->destroy)
entry->destroy(hdev, entry->data, err);
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ hdev->cur_hci_sync_work_func = NULL;
+ hdev->cur_hci_sync_work_data = NULL;
+
+ if (hdev->cur_hci_sync_work_cancelled) {
+ /* If cur_hci_sync_work_cancelled is still set at this point,
+ * no more request was sent and the work func has never been
+ * notified of our cancellation.
+ */
+ hdev->cur_hci_sync_work_cancelled = false;
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
hci_req_sync_unlock(hdev);
}
@@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
}
EXPORT_SYMBOL(hci_cmd_sync_queue);
+bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data)
+{
+ struct hci_cmd_sync_work_entry *entry;
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ if (hdev->cur_hci_sync_work_func == func &&
+ hdev->cur_hci_sync_work_data == data) {
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+ return true;
+ }
+
+ list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
+ if (entry->func == func && entry->data == data) {
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+ return true;
+ }
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ return false;
+}
+
+void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data)
+{
+ struct hci_cmd_sync_work_entry *entry;
+ bool work_already_ongoing;
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ if (hdev->cur_hci_sync_work_func == func &&
+ hdev->cur_hci_sync_work_data == data) {
+ /* If the command is already ongoing, we check if we're currently
+ * sending a async HCI request. If we are, we can cancel that
+ * and hope that the hci_cmd_sync_work_func is handling -ECANCELED.
+ */
+
+ if (hdev->req_status == HCI_REQ_PEND) {
+ /* If we're already executing a request, cancel that request.
+ * This will signal cancellation to the work func which sent
+ * the request in the first place.
+ */
+ __hci_cmd_sync_cancel(hdev, -ECANCELED);
+ } else {
+ /* Otherwise, just set a flag which will cancel the next
+ * request. Just as above, this will then signal cancellation
+ * to the work func.
+ */
+ hdev->cur_hci_sync_work_cancelled = true;
+ }
+
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ return;
+ } else {
+ /* Or is it still queued? Then we remove it from the queue and
+ * execute the destroy callback.
+ */
+ list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
+ if (entry->func == func && entry->data == data) {
+ if (entry->destroy)
+ entry->destroy(hdev, entry->data, -ECANCELED);
+ list_del(&entry->list);
+ kfree(entry);
+
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ if (list_empty(&hdev->cmd_sync_work_list)) {
+ cancel_work_sync(&hdev->cmd_sync_work);
+ cancel_work_sync(&hdev->reenable_adv_work);
+ }
+
+ return;
+ }
+ }
+
+ }
+
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+}
+
int hci_update_eir_sync(struct hci_dev *hdev)
{
struct hci_cp_write_eir cp;
@@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
}
/* Pause advertising while doing directed advertising. */
- hci_pause_advertising_sync(hdev);
+ err = hci_pause_advertising_sync(hdev);
+ if (err == -ECANCELED)
+ goto done;
err = hci_le_directed_advertising_sync(hdev, conn);
goto done;
}
/* Disable advertising if simultaneous roles is not in use. */
- if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES))
- hci_pause_advertising_sync(hdev);
+ if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
+ err = hci_pause_advertising_sync(hdev);
+ if (err == -ECANCELED)
+ goto done;
+ }
params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
if (params) {
@@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
* state.
*/
if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
- hci_scan_disable_sync(hdev);
+ err = hci_scan_disable_sync(hdev);
+ if (err == -ECANCELED)
+ goto done;
+
hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED);
}
@@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
HCI_EV_LE_ENHANCED_CONN_COMPLETE :
HCI_EV_LE_CONN_COMPLETE,
conn->conn_timeout, NULL);
+ if (err == -ECANCELED || err == -ETIMEDOUT)
+ hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00);
done:
- if (err == -ETIMEDOUT)
- hci_le_connect_cancel_sync(hdev, conn, 0x00);
-
/* Re-enable advertising after the connection attempt is finished. */
hci_resume_advertising_sync(hdev);
return err;
@@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
if (test_bit(HCI_INQUIRY, &hdev->flags)) {
err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
NULL, HCI_CMD_TIMEOUT);
- if (err)
+ if (err == -ECANCELED)
+ return -ECANCELED;
+ else if (err)
bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
}
@@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
HCI_EV_CONN_COMPLETE,
HCI_ACL_CONN_TIMEOUT, NULL);
- if (err == -ETIMEDOUT)
- hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM);
+ if (err == -ECANCELED || err == -ETIMEDOUT) {
+ hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM);
+ return err;
+ }
return err;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt
2024-02-13 23:47 ` Jonas Dreßler
@ 2024-02-14 18:44 ` Luiz Augusto von Dentz
2024-02-21 22:25 ` Jonas Dreßler
0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2024-02-14 18:44 UTC (permalink / raw)
To: Jonas Dreßler; +Cc: linux-bluetooth
Hi Jonas,
On Tue, Feb 13, 2024 at 6:47 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Hi Luiz,
>
> > If connection is still queued/pending in the cmd_sync queue it means no
> > command has been generated and it should be safe to just dequeue the
> > callback when it is being aborted.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci_core.h | 19 ++++++++
> > include/net/bluetooth/hci_sync.h | 10 +++--
> > net/bluetooth/hci_conn.c | 70 ++++++------------------------
> > net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++----
> > 4 files changed, 102 insertions(+), 71 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 2bdea85b7c44..317d495cfcf5 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev)
> > return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num;
> > }
> >
> > +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn)
> > +{
> > + struct hci_conn_hash *h = &hdev->conn_hash;
> > + struct hci_conn *c;
> > +
> > + rcu_read_lock();
> > +
> > + list_for_each_entry_rcu(c, &h->list, list) {
> > + if (c == conn) {
> > + rcu_read_unlock();
> > + return true;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > + return false;
> > +}
> > +
> > static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle)
> > {
> > struct hci_conn_hash *h = &hdev->conn_hash;
> > @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
> > struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> > u8 dst_type, bool dst_resolved, u8 sec_level,
> > u16 conn_timeout, u8 role);
> > +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
> > struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> > u8 sec_level, u8 auth_type,
> > enum conn_reasons conn_reason, u16 timeout);
> > diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
> > index 4ff4aa68ee19..6a9d063e9f47 100644
> > --- a/include/net/bluetooth/hci_sync.h
> > +++ b/include/net/bluetooth/hci_sync.h
> > @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> > void *data, hci_cmd_sync_work_destroy_t destroy);
> > int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> > void *data, hci_cmd_sync_work_destroy_t destroy);
> > +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> > + void *data, hci_cmd_sync_work_destroy_t destroy);
> > struct hci_cmd_sync_work_entry *
> > hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> > void *data, hci_cmd_sync_work_destroy_t destroy);
> > -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> > - void *data, hci_cmd_sync_work_destroy_t destroy);
> > void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
> > struct hci_cmd_sync_work_entry *entry);
> > bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> > @@ -139,8 +139,6 @@ struct hci_conn;
> >
> > int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason);
> >
> > -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn);
> > -
> > int hci_le_create_cis_sync(struct hci_dev *hdev);
> >
> > int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle);
> > @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
> > int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
> >
> > int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn);
> > +
> > +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn);
> > +
> > +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn);
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 587eb27f374c..21e0b4064d05 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = {
> > };
> >
> > /* This function requires the caller holds hdev->lock */
> > -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
> > +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
> > {
> > struct hci_conn_params *params;
> > struct hci_dev *hdev = conn->hdev;
> > @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn)
> > * rest of hci_conn_del.
> > */
> > hci_conn_cleanup(conn);
> > +
> > + /* Dequeue callbacks using connection pointer as data */
> > + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL);
> > }
> >
> > struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
> > @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
> > return 0;
> > }
> >
> > -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
> > -{
> > - struct hci_conn *conn;
> > - u16 handle = PTR_UINT(data);
> > -
> > - conn = hci_conn_hash_lookup_handle(hdev, handle);
> > - if (!conn)
> > - return;
> > -
> > - bt_dev_dbg(hdev, "err %d", err);
> > -
> > - hci_dev_lock(hdev);
> > -
> > - if (!err) {
> > - hci_connect_le_scan_cleanup(conn, 0x00);
> > - goto done;
> > - }
> > -
> > - /* Check if connection is still pending */
> > - if (conn != hci_lookup_le_connect(hdev))
> > - goto done;
> > -
> > - /* Flush to make sure we send create conn cancel command if needed */
> > - flush_delayed_work(&conn->le_conn_timeout);
> > - hci_conn_failed(conn, bt_status(err));
> > -
> > -done:
> > - hci_dev_unlock(hdev);
> > -}
> > -
> > -static int hci_connect_le_sync(struct hci_dev *hdev, void *data)
> > -{
> > - struct hci_conn *conn;
> > - u16 handle = PTR_UINT(data);
> > -
> > - conn = hci_conn_hash_lookup_handle(hdev, handle);
> > - if (!conn)
> > - return 0;
> > -
> > - bt_dev_dbg(hdev, "conn %p", conn);
> > -
> > - clear_bit(HCI_CONN_SCANNING, &conn->flags);
> > - conn->state = BT_CONNECT;
> > -
> > - return hci_le_create_conn_sync(hdev, conn);
> > -}
> > -
> > struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> > u8 dst_type, bool dst_resolved, u8 sec_level,
> > u16 conn_timeout, u8 role)
> > @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> > conn->sec_level = BT_SECURITY_LOW;
> > conn->conn_timeout = conn_timeout;
> >
>
> Might want to add a
>
> + if (conn->state != BT_OPEN && conn->state != BT_CLOSED)
> + return conn;
>
> before setting the conn->dst_type while at it, similar to how it's
> in hci_connect_acl().
Hmm but shall never be the case since we have the following check before it:
/* Since the controller supports only one LE connection attempt at a
* time, we return -EBUSY if there is any connection attempt running.
*/
if (hci_lookup_le_connect(hdev))
return ERR_PTR(-EBUSY);
>
> > - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync,
> > - UINT_PTR(conn->handle),
> > - create_le_conn_complete);
> > + err = hci_connect_le_sync(hdev, conn);
> > if (err) {
> > hci_conn_del(conn);
> > return ERR_PTR(err);
> > @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
> >
> > static int abort_conn_sync(struct hci_dev *hdev, void *data)
> > {
> > - struct hci_conn *conn;
> > - u16 handle = PTR_UINT(data);
> > + struct hci_conn *conn = data;
> >
> > - conn = hci_conn_hash_lookup_handle(hdev, handle);
> > - if (!conn)
> > - return 0;
> > + if (!hci_conn_valid(hdev, conn))
> > + return -ECANCELED;
> >
> > return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> > }
> > @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
> > hci_cmd_sync_cancel(hdev, -ECANCELED);
> > break;
> > }
> > + /* Cancel connect attempt if still queued/pending */
> > + } else if (!hci_cancel_connect_sync(hdev, conn)) {
> > + return 0;
> > }
> >
> > - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle),
> > - NULL);
> > + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
> > }
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 5b314bf844f8..b7d8e99e2a30 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
> > conn->conn_timeout, NULL);
> > }
> >
> > -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> > +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
> > {
> > struct hci_cp_le_create_conn cp;
> > struct hci_conn_params *params;
> > u8 own_addr_type;
> > int err;
> > + struct hci_conn *conn = data;
> > +
> > + if (!hci_conn_valid(hdev, conn))
> > + return -ECANCELED;
> > +
> > + bt_dev_dbg(hdev, "conn %p", conn);
> > +
> > + clear_bit(HCI_CONN_SCANNING, &conn->flags);
> > + conn->state = BT_CONNECT;
> >
> > /* If requested to connect as peripheral use directed advertising */
> > if (conn->role == HCI_ROLE_SLAVE) {
> > @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
> >
> > static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
> > {
> > - struct hci_conn *conn;
> > - u16 handle = PTR_UINT(data);
> > + struct hci_conn *conn = data;
> > struct inquiry_entry *ie;
> > struct hci_cp_create_conn cp;
> > int err;
> >
> > - conn = hci_conn_hash_lookup_handle(hdev, handle);
> > - if (!conn)
> > - return 0;
> > -
> > /* Many controllers disallow HCI Create Connection while it is doing
> > * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
> > * Connection. This may cause the MGMT discovering state to become false
> > @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
> >
> > int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
> > {
> > - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync,
> > - UINT_PTR(conn->handle), NULL);
> > + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
> > + NULL);
> > +}
> > +
> > +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
> > +{
> > + struct hci_conn *conn = data;
> > +
> > + bt_dev_dbg(hdev, "err %d", err);
> > +
> > + if (err == -ECANCELED)
> > + return;
> > +
> > + hci_dev_lock(hdev);
> > +
> > + if (!err) {
> > + hci_connect_le_scan_cleanup(conn, 0x00);
> > + goto done;
> > + }
> > +
> > + /* Check if connection is still pending */
> > + if (conn != hci_lookup_le_connect(hdev))
> > + goto done;
> > +
> > + /* Flush to make sure we send create conn cancel command if needed */
> > + flush_delayed_work(&conn->le_conn_timeout);
> > + hci_conn_failed(conn, bt_status(err));
> > +
> > +done:
> > + hci_dev_unlock(hdev);
> > +}
> > +
> > +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
> > +{
> > + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
> > + create_le_conn_complete);
> > +}
> > +
> > +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
> > +{
> > + if (conn->state != BT_OPEN)
> > + return -EINVAL;
> > +
> > + switch (conn->type) {
> > + case ACL_LINK:
> > + return !hci_cmd_sync_dequeue_once(hdev,
> > + hci_acl_create_conn_sync,
> > + conn, NULL);
> > + case LE_LINK:
> > + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync,
> > + conn, create_le_conn_complete);
> > + }
> > +
> > + return -ENOENT;
> > }
> > --
> > 2.43.0
>
> Thanks for sending those patches, this is pretty much exactly what I had in mind!
>
> I came up with a slightly different cancel function for the queued work, one that
> also cancels the ongoing work. I'm not sure if it makes too much sense, because it
> means adding careful -ECANCELED handling to those sync_work callbacks.
>
> The nice thing about it is that it should also allow getting rid of the
> hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn().
>
> Adding the patch below, feel free to reuse whatever you like!
>
> Cheers,
> Jonas
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 067d445701..a2b14f6be1 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
>
> bt_dev_dbg(hdev, "Opcode 0x%4x", opcode);
>
> + if (hdev->cur_hci_sync_work_cancelled) {
> + hdev->cur_hci_sync_work_cancelled = false;
> +
> + return ERR_PTR(-ECANCELED);
> + }
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> hci_req_init(&req, hdev);
>
> hci_cmd_sync_add(&req, opcode, plen, param, event, sk);
> @@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work)
> int err;
>
> hci_req_sync_lock(hdev);
> +
> + mutex_lock(&hdev->cmd_sync_work_lock);
> + hdev->cur_hci_sync_work_func = entry->func;
> + hdev->cur_hci_sync_work_data = entry->data;
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> err = entry->func(hdev, entry->data);
> if (entry->destroy)
> entry->destroy(hdev, entry->data, err);
> +
> + mutex_lock(&hdev->cmd_sync_work_lock);
> + hdev->cur_hci_sync_work_func = NULL;
> + hdev->cur_hci_sync_work_data = NULL;
> +
> + if (hdev->cur_hci_sync_work_cancelled) {
> + /* If cur_hci_sync_work_cancelled is still set at this point,
> + * no more request was sent and the work func has never been
> + * notified of our cancellation.
> + */
> + hdev->cur_hci_sync_work_cancelled = false;
> + }
> + mutex_unlock(&hdev->cmd_sync_work_lock);
Not really following this code, do you want to be able to cancel
callbacks that are actually executing, rather than queued?
> hci_req_sync_unlock(hdev);
> }
>
> @@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> }
> EXPORT_SYMBOL(hci_cmd_sync_queue);
>
> +bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> + void *data)
> +{
> + struct hci_cmd_sync_work_entry *entry;
> +
> + mutex_lock(&hdev->cmd_sync_work_lock);
> + if (hdev->cur_hci_sync_work_func == func &&
> + hdev->cur_hci_sync_work_data == data) {
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> + return true;
> + }
> +
> + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
> + if (entry->func == func && entry->data == data) {
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> + return true;
> + }
> + }
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> + return false;
> +}
> +
> +void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
> + void *data)
> +{
> + struct hci_cmd_sync_work_entry *entry;
> + bool work_already_ongoing;
> +
> + mutex_lock(&hdev->cmd_sync_work_lock);
> + if (hdev->cur_hci_sync_work_func == func &&
> + hdev->cur_hci_sync_work_data == data) {
> + /* If the command is already ongoing, we check if we're currently
> + * sending a async HCI request. If we are, we can cancel that
> + * and hope that the hci_cmd_sync_work_func is handling -ECANCELED.
> + */
> +
> + if (hdev->req_status == HCI_REQ_PEND) {
> + /* If we're already executing a request, cancel that request.
> + * This will signal cancellation to the work func which sent
> + * the request in the first place.
> + */
> + __hci_cmd_sync_cancel(hdev, -ECANCELED);
> + } else {
> + /* Otherwise, just set a flag which will cancel the next
> + * request. Just as above, this will then signal cancellation
> + * to the work func.
> + */
> + hdev->cur_hci_sync_work_cancelled = true;
> + }
It might be better to save the executing entry at hdev and then make
the lookup_entry return it if the parameters match so it can be
cancelled with cancel_entry, that said perhaps it would be better to
just cancel the work if -ECANCELED is received so we don't have to
keep checking if it is returned on every command the callback
generates.
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> + return;
> + } else {
> + /* Or is it still queued? Then we remove it from the queue and
> + * execute the destroy callback.
> + */
> + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
> + if (entry->func == func && entry->data == data) {
> + if (entry->destroy)
> + entry->destroy(hdev, entry->data, -ECANCELED);
> + list_del(&entry->list);
> + kfree(entry);
> +
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> + if (list_empty(&hdev->cmd_sync_work_list)) {
> + cancel_work_sync(&hdev->cmd_sync_work);
> + cancel_work_sync(&hdev->reenable_adv_work);
> + }
> +
> + return;
> + }
> + }
> +
> + }
> +
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> +}
> +
> int hci_update_eir_sync(struct hci_dev *hdev)
> {
> struct hci_cp_write_eir cp;
> @@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> }
>
> /* Pause advertising while doing directed advertising. */
> - hci_pause_advertising_sync(hdev);
> + err = hci_pause_advertising_sync(hdev);
> + if (err == -ECANCELED)
> + goto done;
>
> err = hci_le_directed_advertising_sync(hdev, conn);
> goto done;
> }
>
> /* Disable advertising if simultaneous roles is not in use. */
> - if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES))
> - hci_pause_advertising_sync(hdev);
> + if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
> + err = hci_pause_advertising_sync(hdev);
> + if (err == -ECANCELED)
> + goto done;
> + }
>
> params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
> if (params) {
> @@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> * state.
> */
> if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
> - hci_scan_disable_sync(hdev);
> + err = hci_scan_disable_sync(hdev);
> + if (err == -ECANCELED)
> + goto done;
> +
> hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED);
> }
>
> @@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> HCI_EV_LE_ENHANCED_CONN_COMPLETE :
> HCI_EV_LE_CONN_COMPLETE,
> conn->conn_timeout, NULL);
> + if (err == -ECANCELED || err == -ETIMEDOUT)
> + hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00);
>
> done:
> - if (err == -ETIMEDOUT)
> - hci_le_connect_cancel_sync(hdev, conn, 0x00);
> -
> /* Re-enable advertising after the connection attempt is finished. */
> hci_resume_advertising_sync(hdev);
> return err;
> @@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
> NULL, HCI_CMD_TIMEOUT);
> - if (err)
> + if (err == -ECANCELED)
> + return -ECANCELED;
> + else if (err)
> bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
> }
>
> @@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
> HCI_EV_CONN_COMPLETE,
> HCI_ACL_CONN_TIMEOUT, NULL);
>
> - if (err == -ETIMEDOUT)
> - hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM);
> + if (err == -ECANCELED || err == -ETIMEDOUT) {
> + hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM);
> + return err;
> + }
>
> return err;
> }
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync
2024-02-13 21:31 [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync Luiz Augusto von Dentz
` (2 preceding siblings ...)
2024-02-13 21:59 ` [v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync bluez.test.bot
@ 2024-02-15 21:20 ` patchwork-bot+bluetooth
3 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+bluetooth @ 2024-02-15 21:20 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hello:
This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
On Tue, 13 Feb 2024 16:31:21 -0500 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This fixes the UAF on __hci_acl_create_connection_sync caused by
> connection abortion, it uses the same logic as to LE_LINK which uses
> hci_cmd_sync_cancel to prevent the callback to run if the connection is
> abort prematurely.
>
> [...]
Here is the summary with links:
- [v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync
https://git.kernel.org/bluetooth/bluetooth-next/c/f36e87456bf6
- [v2,2/3] Bluetooth: hci_sync: Add helper functions to manipulate cmd_sync queue
https://git.kernel.org/bluetooth/bluetooth-next/c/4a8a8f446ef2
- [v2,3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt
https://git.kernel.org/bluetooth/bluetooth-next/c/96fb2aab16bf
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt
2024-02-14 18:44 ` Luiz Augusto von Dentz
@ 2024-02-21 22:25 ` Jonas Dreßler
0 siblings, 0 replies; 8+ messages in thread
From: Jonas Dreßler @ 2024-02-21 22:25 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
On 14.02.24 19:44, Luiz Augusto von Dentz wrote:
> Hi Jonas,
>
> On Tue, Feb 13, 2024 at 6:47 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> Hi Luiz,
>>
>>> If connection is still queued/pending in the cmd_sync queue it means no
>>> command has been generated and it should be safe to just dequeue the
>>> callback when it is being aborted.
>>>
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> include/net/bluetooth/hci_core.h | 19 ++++++++
>>> include/net/bluetooth/hci_sync.h | 10 +++--
>>> net/bluetooth/hci_conn.c | 70 ++++++------------------------
>>> net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++----
>>> 4 files changed, 102 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 2bdea85b7c44..317d495cfcf5 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev)
>>> return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num;
>>> }
>>>
>>> +static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn)
>>> +{
>>> + struct hci_conn_hash *h = &hdev->conn_hash;
>>> + struct hci_conn *c;
>>> +
>>> + rcu_read_lock();
>>> +
>>> + list_for_each_entry_rcu(c, &h->list, list) {
>>> + if (c == conn) {
>>> + rcu_read_unlock();
>>> + return true;
>>> + }
>>> + }
>>> + rcu_read_unlock();
>>> +
>>> + return false;
>>> +}
>>> +
>>> static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle)
>>> {
>>> struct hci_conn_hash *h = &hdev->conn_hash;
>>> @@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
>>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>>> u8 dst_type, bool dst_resolved, u8 sec_level,
>>> u16 conn_timeout, u8 role);
>>> +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
>>> struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>>> u8 sec_level, u8 auth_type,
>>> enum conn_reasons conn_reason, u16 timeout);
>>> diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
>>> index 4ff4aa68ee19..6a9d063e9f47 100644
>>> --- a/include/net/bluetooth/hci_sync.h
>>> +++ b/include/net/bluetooth/hci_sync.h
>>> @@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>>> void *data, hci_cmd_sync_work_destroy_t destroy);
>>> int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>>> void *data, hci_cmd_sync_work_destroy_t destroy);
>>> +int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>>> + void *data, hci_cmd_sync_work_destroy_t destroy);
>>> struct hci_cmd_sync_work_entry *
>>> hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>>> void *data, hci_cmd_sync_work_destroy_t destroy);
>>> -int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>>> - void *data, hci_cmd_sync_work_destroy_t destroy);
>>> void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
>>> struct hci_cmd_sync_work_entry *entry);
>>> bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>>> @@ -139,8 +139,6 @@ struct hci_conn;
>>>
>>> int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason);
>>>
>>> -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn);
>>> -
>>> int hci_le_create_cis_sync(struct hci_dev *hdev);
>>>
>>> int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle);
>>> @@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
>>> int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
>>>
>>> int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn);
>>> +
>>> +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn);
>>> +
>>> +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn);
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index 587eb27f374c..21e0b4064d05 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = {
>>> };
>>>
>>> /* This function requires the caller holds hdev->lock */
>>> -static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
>>> +void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
>>> {
>>> struct hci_conn_params *params;
>>> struct hci_dev *hdev = conn->hdev;
>>> @@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn)
>>> * rest of hci_conn_del.
>>> */
>>> hci_conn_cleanup(conn);
>>> +
>>> + /* Dequeue callbacks using connection pointer as data */
>>> + hci_cmd_sync_dequeue(hdev, NULL, conn, NULL);
>>> }
>>>
>>> struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
>>> @@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
>>> return 0;
>>> }
>>>
>>> -static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
>>> -{
>>> - struct hci_conn *conn;
>>> - u16 handle = PTR_UINT(data);
>>> -
>>> - conn = hci_conn_hash_lookup_handle(hdev, handle);
>>> - if (!conn)
>>> - return;
>>> -
>>> - bt_dev_dbg(hdev, "err %d", err);
>>> -
>>> - hci_dev_lock(hdev);
>>> -
>>> - if (!err) {
>>> - hci_connect_le_scan_cleanup(conn, 0x00);
>>> - goto done;
>>> - }
>>> -
>>> - /* Check if connection is still pending */
>>> - if (conn != hci_lookup_le_connect(hdev))
>>> - goto done;
>>> -
>>> - /* Flush to make sure we send create conn cancel command if needed */
>>> - flush_delayed_work(&conn->le_conn_timeout);
>>> - hci_conn_failed(conn, bt_status(err));
>>> -
>>> -done:
>>> - hci_dev_unlock(hdev);
>>> -}
>>> -
>>> -static int hci_connect_le_sync(struct hci_dev *hdev, void *data)
>>> -{
>>> - struct hci_conn *conn;
>>> - u16 handle = PTR_UINT(data);
>>> -
>>> - conn = hci_conn_hash_lookup_handle(hdev, handle);
>>> - if (!conn)
>>> - return 0;
>>> -
>>> - bt_dev_dbg(hdev, "conn %p", conn);
>>> -
>>> - clear_bit(HCI_CONN_SCANNING, &conn->flags);
>>> - conn->state = BT_CONNECT;
>>> -
>>> - return hci_le_create_conn_sync(hdev, conn);
>>> -}
>>> -
>>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>>> u8 dst_type, bool dst_resolved, u8 sec_level,
>>> u16 conn_timeout, u8 role)
>>> @@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>>> conn->sec_level = BT_SECURITY_LOW;
>>> conn->conn_timeout = conn_timeout;
>>>
>>
>> Might want to add a
>>
>> + if (conn->state != BT_OPEN && conn->state != BT_CLOSED)
>> + return conn;
>>
>> before setting the conn->dst_type while at it, similar to how it's
>> in hci_connect_acl().
>
> Hmm but shall never be the case since we have the following check before it:
>
> /* Since the controller supports only one LE connection attempt at a
> * time, we return -EBUSY if there is any connection attempt running.
> */
> if (hci_lookup_le_connect(hdev))
> return ERR_PTR(-EBUSY);
Ahh okay, fair, didn't notice that!
>
>>
>>> - err = hci_cmd_sync_queue(hdev, hci_connect_le_sync,
>>> - UINT_PTR(conn->handle),
>>> - create_le_conn_complete);
>>> + err = hci_connect_le_sync(hdev, conn);
>>> if (err) {
>>> hci_conn_del(conn);
>>> return ERR_PTR(err);
>>> @@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
>>>
>>> static int abort_conn_sync(struct hci_dev *hdev, void *data)
>>> {
>>> - struct hci_conn *conn;
>>> - u16 handle = PTR_UINT(data);
>>> + struct hci_conn *conn = data;
>>>
>>> - conn = hci_conn_hash_lookup_handle(hdev, handle);
>>> - if (!conn)
>>> - return 0;
>>> + if (!hci_conn_valid(hdev, conn))
>>> + return -ECANCELED;
>>>
>>> return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
>>> }
>>> @@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
>>> hci_cmd_sync_cancel(hdev, -ECANCELED);
>>> break;
>>> }
>>> + /* Cancel connect attempt if still queued/pending */
>>> + } else if (!hci_cancel_connect_sync(hdev, conn)) {
>>> + return 0;
>>> }
>>>
>>> - return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle),
>>> - NULL);
>>> + return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
>>> }
>>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>>> index 5b314bf844f8..b7d8e99e2a30 100644
>>> --- a/net/bluetooth/hci_sync.c
>>> +++ b/net/bluetooth/hci_sync.c
>>> @@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
>>> conn->conn_timeout, NULL);
>>> }
>>>
>>> -int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
>>> +static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>>> {
>>> struct hci_cp_le_create_conn cp;
>>> struct hci_conn_params *params;
>>> u8 own_addr_type;
>>> int err;
>>> + struct hci_conn *conn = data;
>>> +
>>> + if (!hci_conn_valid(hdev, conn))
>>> + return -ECANCELED;
>>> +
>>> + bt_dev_dbg(hdev, "conn %p", conn);
>>> +
>>> + clear_bit(HCI_CONN_SCANNING, &conn->flags);
>>> + conn->state = BT_CONNECT;
>>>
>>> /* If requested to connect as peripheral use directed advertising */
>>> if (conn->role == HCI_ROLE_SLAVE) {
>>> @@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
>>>
>>> static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
>>> {
>>> - struct hci_conn *conn;
>>> - u16 handle = PTR_UINT(data);
>>> + struct hci_conn *conn = data;
>>> struct inquiry_entry *ie;
>>> struct hci_cp_create_conn cp;
>>> int err;
>>>
>>> - conn = hci_conn_hash_lookup_handle(hdev, handle);
>>> - if (!conn)
>>> - return 0;
>>> -
>>> /* Many controllers disallow HCI Create Connection while it is doing
>>> * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
>>> * Connection. This may cause the MGMT discovering state to become false
>>> @@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
>>>
>>> int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
>>> {
>>> - return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync,
>>> - UINT_PTR(conn->handle), NULL);
>>> + return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
>>> + NULL);
>>> +}
>>> +
>>> +static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
>>> +{
>>> + struct hci_conn *conn = data;
>>> +
>>> + bt_dev_dbg(hdev, "err %d", err);
>>> +
>>> + if (err == -ECANCELED)
>>> + return;
>>> +
>>> + hci_dev_lock(hdev);
>>> +
>>> + if (!err) {
>>> + hci_connect_le_scan_cleanup(conn, 0x00);
>>> + goto done;
>>> + }
>>> +
>>> + /* Check if connection is still pending */
>>> + if (conn != hci_lookup_le_connect(hdev))
>>> + goto done;
>>> +
>>> + /* Flush to make sure we send create conn cancel command if needed */
>>> + flush_delayed_work(&conn->le_conn_timeout);
>>> + hci_conn_failed(conn, bt_status(err));
>>> +
>>> +done:
>>> + hci_dev_unlock(hdev);
>>> +}
>>> +
>>> +int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
>>> +{
>>> + return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
>>> + create_le_conn_complete);
>>> +}
>>> +
>>> +int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
>>> +{
>>> + if (conn->state != BT_OPEN)
>>> + return -EINVAL;
>>> +
>>> + switch (conn->type) {
>>> + case ACL_LINK:
>>> + return !hci_cmd_sync_dequeue_once(hdev,
>>> + hci_acl_create_conn_sync,
>>> + conn, NULL);
>>> + case LE_LINK:
>>> + return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync,
>>> + conn, create_le_conn_complete);
>>> + }
>>> +
>>> + return -ENOENT;
>>> }
>>> --
>>> 2.43.0
>>
>> Thanks for sending those patches, this is pretty much exactly what I had in mind!
>>
>> I came up with a slightly different cancel function for the queued work, one that
>> also cancels the ongoing work. I'm not sure if it makes too much sense, because it
>> means adding careful -ECANCELED handling to those sync_work callbacks.
>>
>> The nice thing about it is that it should also allow getting rid of the
>> hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn().
>>
>> Adding the patch below, feel free to reuse whatever you like!
>>
>> Cheers,
>> Jonas
>>
>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>> index 067d445701..a2b14f6be1 100644
>> --- a/net/bluetooth/hci_sync.c
>> +++ b/net/bluetooth/hci_sync.c
>> @@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
>>
>> bt_dev_dbg(hdev, "Opcode 0x%4x", opcode);
>>
>> + if (hdev->cur_hci_sync_work_cancelled) {
>> + hdev->cur_hci_sync_work_cancelled = false;
>> +
>> + return ERR_PTR(-ECANCELED);
>> + }
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> +
>> hci_req_init(&req, hdev);
>>
>> hci_cmd_sync_add(&req, opcode, plen, param, event, sk);
>> @@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work)
>> int err;
>>
>> hci_req_sync_lock(hdev);
>> +
>> + mutex_lock(&hdev->cmd_sync_work_lock);
>> + hdev->cur_hci_sync_work_func = entry->func;
>> + hdev->cur_hci_sync_work_data = entry->data;
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> +
>> err = entry->func(hdev, entry->data);
>> if (entry->destroy)
>> entry->destroy(hdev, entry->data, err);
>> +
>> + mutex_lock(&hdev->cmd_sync_work_lock);
>> + hdev->cur_hci_sync_work_func = NULL;
>> + hdev->cur_hci_sync_work_data = NULL;
>> +
>> + if (hdev->cur_hci_sync_work_cancelled) {
>> + /* If cur_hci_sync_work_cancelled is still set at this point,
>> + * no more request was sent and the work func has never been
>> + * notified of our cancellation.
>> + */
>> + hdev->cur_hci_sync_work_cancelled = false;
>> + }
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>
> Not really following this code, do you want to be able to cancel
> callbacks that are actually executing, rather than queued?
Yup exactly, I'm using __hci_cmd_sync_cancel(-ECANCELED) for that, and
in the case where there's not currently a hci req ongoing, I'm setting
a flag so that the next hci req will return -ECANCELED immediately.
It's not too necessary to cancel the ongoing callback too I think, but
since we have __hci_cmd_sync_cancel() already it seemed to make sense.
And IMO it's a lot more elegant than the check for hci_skb_event() that
hci_abort_conn() currently does.
>
>> hci_req_sync_unlock(hdev);
>> }
>>
>> @@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>> }
>> EXPORT_SYMBOL(hci_cmd_sync_queue);
>>
>> +bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>> + void *data)
>> +{
>> + struct hci_cmd_sync_work_entry *entry;
>> +
>> + mutex_lock(&hdev->cmd_sync_work_lock);
>> + if (hdev->cur_hci_sync_work_func == func &&
>> + hdev->cur_hci_sync_work_data == data) {
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> + return true;
>> + }
>> +
>> + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
>> + if (entry->func == func && entry->data == data) {
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> + return true;
>> + }
>> + }
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> +
>> + return false;
>> +}
>> +
>> +void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
>> + void *data)
>> +{
>> + struct hci_cmd_sync_work_entry *entry;
>> + bool work_already_ongoing;
>> +
>> + mutex_lock(&hdev->cmd_sync_work_lock);
>> + if (hdev->cur_hci_sync_work_func == func &&
>> + hdev->cur_hci_sync_work_data == data) {
>> + /* If the command is already ongoing, we check if we're currently
>> + * sending a async HCI request. If we are, we can cancel that
>> + * and hope that the hci_cmd_sync_work_func is handling -ECANCELED.
>> + */
>> +
>> + if (hdev->req_status == HCI_REQ_PEND) {
>> + /* If we're already executing a request, cancel that request.
>> + * This will signal cancellation to the work func which sent
>> + * the request in the first place.
>> + */
>> + __hci_cmd_sync_cancel(hdev, -ECANCELED);
>> + } else {
>> + /* Otherwise, just set a flag which will cancel the next
>> + * request. Just as above, this will then signal cancellation
>> + * to the work func.
>> + */
>> + hdev->cur_hci_sync_work_cancelled = true;
>> + }
>
> It might be better to save the executing entry at hdev and then make
> the lookup_entry return it if the parameters match so it can be
> cancelled with cancel_entry,
Ahh yeah, now that lookup_entry() is a thing, that seems nicer indeed.
> that said perhaps it would be better to
> just cancel the work if -ECANCELED is received so we don't have to
> keep checking if it is returned on every command the callback
> generates
Hmm, I don't see how it makes sense to cancel the cmd_sync_work, and
we can't cancel the callback execution from outside, can we? I don't
think there's a way around checking -ECANCELED all the time as long
as __hci_cmd_sync_cancel()/hci_cmd_sync_cancel() are a thing, we
should probably ensure that every hci_sync callback handles this
properly anyway.
Cheers
Jonas
>
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> +
>> + return;
>> + } else {
>> + /* Or is it still queued? Then we remove it from the queue and
>> + * execute the destroy callback.
>> + */
>> + list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
>> + if (entry->func == func && entry->data == data) {
>> + if (entry->destroy)
>> + entry->destroy(hdev, entry->data, -ECANCELED);
>> + list_del(&entry->list);
>> + kfree(entry);
>> +
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> +
>> + if (list_empty(&hdev->cmd_sync_work_list)) {
>> + cancel_work_sync(&hdev->cmd_sync_work);
>> + cancel_work_sync(&hdev->reenable_adv_work);
>> + }
>> +
>> + return;
>> + }
>> + }
>> +
>> + }
>> +
>> + mutex_unlock(&hdev->cmd_sync_work_lock);
>> +}
>> +
>> int hci_update_eir_sync(struct hci_dev *hdev)
>> {
>> struct hci_cp_write_eir cp;
>> @@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> }
>>
>> /* Pause advertising while doing directed advertising. */
>> - hci_pause_advertising_sync(hdev);
>> + err = hci_pause_advertising_sync(hdev);
>> + if (err == -ECANCELED)
>> + goto done;
>>
>> err = hci_le_directed_advertising_sync(hdev, conn);
>> goto done;
>> }
>>
>> /* Disable advertising if simultaneous roles is not in use. */
>> - if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES))
>> - hci_pause_advertising_sync(hdev);
>> + if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
>> + err = hci_pause_advertising_sync(hdev);
>> + if (err == -ECANCELED)
>> + goto done;
>> + }
>>
>> params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
>> if (params) {
>> @@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> * state.
>> */
>> if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
>> - hci_scan_disable_sync(hdev);
>> + err = hci_scan_disable_sync(hdev);
>> + if (err == -ECANCELED)
>> + goto done;
>> +
>> hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED);
>> }
>>
>> @@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> HCI_EV_LE_ENHANCED_CONN_COMPLETE :
>> HCI_EV_LE_CONN_COMPLETE,
>> conn->conn_timeout, NULL);
>> + if (err == -ECANCELED || err == -ETIMEDOUT)
>> + hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00);
>>
>> done:
>> - if (err == -ETIMEDOUT)
>> - hci_le_connect_cancel_sync(hdev, conn, 0x00);
>> -
>> /* Re-enable advertising after the connection attempt is finished. */
>> hci_resume_advertising_sync(hdev);
>> return err;
>> @@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
>> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
>> err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
>> NULL, HCI_CMD_TIMEOUT);
>> - if (err)
>> + if (err == -ECANCELED)
>> + return -ECANCELED;
>> + else if (err)
>> bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
>> }
>>
>> @@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
>> HCI_EV_CONN_COMPLETE,
>> HCI_ACL_CONN_TIMEOUT, NULL);
>>
>> - if (err == -ETIMEDOUT)
>> - hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM);
>> + if (err == -ECANCELED || err == -ETIMEDOUT) {
>> + hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM);
>> + return err;
>> + }
>>
>> return err;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-02-21 22:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13 21:31 [PATCH v2 1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 2/3] Bluetooth: hci_sync: Add helper functions to manipulate cmd_sync queue Luiz Augusto von Dentz
2024-02-13 21:31 ` [PATCH v2 3/3] Bluetooth: hci_sync: Attempt to dequeue connection attempt Luiz Augusto von Dentz
2024-02-13 23:47 ` Jonas Dreßler
2024-02-14 18:44 ` Luiz Augusto von Dentz
2024-02-21 22:25 ` Jonas Dreßler
2024-02-13 21:59 ` [v2,1/3] Bluetooth: hci_conn: Fix UAF Write in __hci_acl_create_connection_sync bluez.test.bot
2024-02-15 21:20 ` [PATCH v2 1/3] " patchwork-bot+bluetooth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).