* [PATCH] Bluetooth: hci_sync: always check if connection is alive before deleting
@ 2023-08-25 21:59 Pauli Virtanen
2023-08-25 22:12 ` bluez.test.bot
0 siblings, 1 reply; 2+ messages in thread
From: Pauli Virtanen @ 2023-08-25 21:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
In hci_abort_conn_sync it is possible that conn is deleted concurrently
by something else, also e.g. when waiting for hdev->lock. This causes
double deletion of the conn, so UAF or conn_hash.list corruption.
Fix by checking for all deletions that the connection is still in
conn_hash, while holding hdev->lock which prevents any races.
Also remove bis/pa sync special handling, as it's not really necessary.
Also fixes a busy loop in disconnect_all_sync for non-BIG conns in state
BT_OPEN.
Log (when powering off while BAP streaming, occurs rarely):
=======================================================================
kernel BUG at lib/list_debug.c:56!
...
? __list_del_entry_valid (lib/list_debug.c:56)
hci_conn_del (net/bluetooth/hci_conn.c:154) bluetooth
hci_abort_conn_sync (net/bluetooth/hci_sync.c:5415) bluetooth
? __pfx_hci_abort_conn_sync+0x10/0x10 [bluetooth]
? lock_release+0x1d5/0x3c0
? hci_disconnect_all_sync.constprop.0+0xb2/0x230 [bluetooth]
? __pfx_lock_release+0x10/0x10
? __kmem_cache_free+0x14d/0x2e0
hci_disconnect_all_sync.constprop.0+0xda/0x230 [bluetooth]
? __pfx_hci_disconnect_all_sync.constprop.0+0x10/0x10 [bluetooth]
? hci_clear_adv_sync+0x14f/0x170 [bluetooth]
? __pfx_set_powered_sync+0x10/0x10 [bluetooth]
hci_set_powered_sync+0x293/0x450 [bluetooth]
=======================================================================
Fixes: 94d9ba9f9888 ("Bluetooth: hci_sync: Fix UAF in hci_disconnect_all_sync")
Fixes: fbdc4bc47268 ("Bluetooth: ISO: Use defer setup to separate PA sync and BIG sync")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
Notes:
Deleting conns in hci_sync just keeps on giving, this hopefully should
be the last one on hci_sync side.
I tested 94d9ba9f9888 earlier, but apparently didn't have enough
good/bad luck to hit this one then.
There might be more in hdev->workqueue side, given that some code paths
there don't lock properly and assume deletions are serialized. Maybe
these should be fixed to lock, or move the lookup-handle-then-delete
part to hdev->workqueue.
net/bluetooth/hci_sync.c | 41 ++++++++++++----------------------------
1 file changed, 12 insertions(+), 29 deletions(-)
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a93096c5cbfd..f834c9ff0dfe 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5374,6 +5374,7 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
int err = 0;
u16 handle = conn->handle;
struct hci_conn *c;
+ bool disconnect = false;
WARN_ON(!reason);
@@ -5389,40 +5390,16 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
err = hci_reject_conn_sync(hdev, conn, reason);
break;
case BT_OPEN:
- hci_dev_lock(hdev);
-
- /* Cleanup bis or pa sync connections */
- if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags) ||
- test_and_clear_bit(HCI_CONN_PA_SYNC_FAILED, &conn->flags)) {
- hci_conn_failed(conn, reason);
- } else if (test_bit(HCI_CONN_PA_SYNC, &conn->flags) ||
- test_bit(HCI_CONN_BIG_SYNC, &conn->flags)) {
- conn->state = BT_CLOSED;
- hci_disconn_cfm(conn, reason);
- hci_conn_del(conn);
- }
-
- hci_dev_unlock(hdev);
- return 0;
case BT_BOUND:
- hci_dev_lock(hdev);
- hci_conn_failed(conn, reason);
- hci_dev_unlock(hdev);
- return 0;
+ break;
default:
- hci_dev_lock(hdev);
- conn->state = BT_CLOSED;
- hci_disconn_cfm(conn, reason);
- hci_conn_del(conn);
- hci_dev_unlock(hdev);
- return 0;
+ disconnect = true;
+ break;
}
hci_dev_lock(hdev);
- /* Check if the connection hasn't been cleanup while waiting
- * commands to complete.
- */
+ /* Check if the connection has been cleaned up concurrently */
c = hci_conn_hash_lookup_handle(hdev, handle);
if (!c || c != conn) {
err = 0;
@@ -5434,7 +5411,13 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
* or in case of LE it was still scanning so it can be cleanup
* safely.
*/
- hci_conn_failed(conn, reason);
+ if (disconnect) {
+ conn->state = BT_CLOSED;
+ hci_disconn_cfm(conn, reason);
+ hci_conn_del(conn);
+ } else {
+ hci_conn_failed(conn, reason);
+ }
unlock:
hci_dev_unlock(hdev);
--
2.41.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* RE: Bluetooth: hci_sync: always check if connection is alive before deleting
2023-08-25 21:59 [PATCH] Bluetooth: hci_sync: always check if connection is alive before deleting Pauli Virtanen
@ 2023-08-25 22:12 ` bluez.test.bot
0 siblings, 0 replies; 2+ messages in thread
From: bluez.test.bot @ 2023-08-25 22:12 UTC (permalink / raw)
To: linux-bluetooth, pav
[-- Attachment #1: Type: text/plain, Size: 551 bytes --]
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: patch failed: net/bluetooth/hci_sync.c:5374
error: net/bluetooth/hci_sync.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Please resolve the issue and submit the patches again.
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-08-25 22:13 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-25 21:59 [PATCH] Bluetooth: hci_sync: always check if connection is alive before deleting Pauli Virtanen
2023-08-25 22:12 ` bluez.test.bot
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).