Bluetooth: ISO: fix HUP on socket release/shutdown + UAF/locking fixes
Subsystems (2)
cover Bluetooth: ISO: fix HUP on socket release/shutdown + UAF/locking fixes
The retracted patch in commit e824c0bbe0ec9 ("Bluetooth: ISO: clear iso_data always when detaching conn from hcon") merged in bluetooth-next/master broke ISO socket transition to BT_CLOSED so they don't send POLLHUP properly any more, as seen in "ISO Disconnect - Success" test.
This is rebased version of and supercedes
https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/
The first commit in this series fixes that, and subsequent mostly independent commits other minor issues found looking at the ISO socket code.
The last three fix UAF issues. The locking/lifetime of iso_conn complicates the code here and the last three patches can be replaced by somewhat simpler solution with bigger diff that removes iso_conn
https://github.com/pv/linux/commit/e4b460b5f7aaba060b958229aaec6b2217642178
Fixed issues aside BT_CONNECT, these appear to be pre-existing in several previous kernel releases:
- sk is always leaked on socket release
- timeout_work may deadlock under certain conditions
- correctness of iso_conn_del() vs UAF requires somewhat too complex reasoning about race conditions, and it's not quite correct
[Task 1] [Task hdev->workqueue]
iso_sock_timeout iso_conn_del
iso_conn_hold_unless_zero iso_chan_del
`------------> iso_conn_put
caller frees hcon
iso_conn_put
iso_conn_free
conn->hcon->iso_data = NULL; /* UAF */- attempt to free iso_conn in iso_conn_del() races with iso_conn_del with potential UAF
- iso_sock_ready() gets conn->sk without requisite locks, this should be done like in sco.c
- iso_sock_getname, iso_connect_ind are missing lock_sock and NULL checks
- kref_get_unless_zero(&((struct iso_conn *)hcon->iso_data)->ref) requires synchronization primitive.
Tested vs iso-tester, Pipewire Qemu ucast/bcast audio tests, and real ucast audio streaming, which pass without KASAN/locking splats. Also checked iso-tester produces balanced iso_sock_init / iso_sock_destruct kprints.
Pauli Virtanen (11):
Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
Bluetooth: ISO: lock sk in iso_sock_getname
Bluetooth: ISO: lock sk in iso_connect_ind
Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos
Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis()
Bluetooth: ISO: hold sk properly in iso_conn_ready
Bluetooth: ISO: fix leaking sk after socket release
Bluetooth: ISO: avoid deadlocks in iso_sock_timeout
Bluetooth: ISO: ensure no dangling hcon references in iso_conn
Bluetooth: ISO: fix refcounting of iso_conn
Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero
include/net/bluetooth/hci_core.h | 4 +- net/bluetooth/hci_conn.c | 2 + net/bluetooth/iso.c | 274 ++++++++++++++++++++++--------- 3 files changed, 197 insertions(+), 83 deletions(-)
Signature · 1 line
2.55.0
PATCH 1/11 Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
Commit e824c0bbe0ec ("Bluetooth: ISO: clear iso_data always when detaching conn from hcon") merged a version of the UAF fix that breaks releasing connected
ISO sockets. Since hci_conn::iso_data is set to NULL, iso_chan_del() won't be called when the hci_conn disconnects, and the ISO socket does not emit POLLHUP correctly.
Fix by retaining full hci_conn <-> iso_conn association while in BT_DISCONNECT state, so that local disconnect via shutdown() follows similar ISO socket code path as remote disconnect. Use a separate flag to track whether hci_conn_drop() is needed, instead of setting iso_conn::hcon = NULL
In iso_sock_ready(), disallow disconnecting socket going BT_CONNECTED, in case hcon connects while its drop is pending.
---
Notes:
This is rebased version of and supercedes
net/bluetooth/iso.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
net/bluetooth/iso.c +20 -6
@@ -24,8 +24,14 @@ static struct bt_sock_list iso_sk_list = { };/* ---- ISO connections ---- */ +enum { +ISO_CONN_DROPPED , + __ISO_CONN_NUM_FLAGS +}; +struct iso_conn {struct hci_conn *hcon; + DECLARE_BITMAP(flags, __ISO_CONN_NUM_FLAGS);/* @lock: spinlock protecting changes to iso_conn fields */ spinlock_t lock;
@@ -107,7 +113,8 @@ static void iso_conn_free(struct kref *ref)if (conn->hcon) { conn->hcon->iso_data =NULL ; -hci_conn_drop (conn->hcon); +if (!test_and_set_bit (ISO_CONN_DROPPED , conn->flags)) +hci_conn_drop (conn->hcon); }/* Ensure no more work items will run since hci_conn has been dropped */
@@ -306,6 +313,7 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,iso_pi (sk)->conn = conn; conn->sk = sk; +clear_bit (ISO_CONN_DROPPED , conn->flags);if (parent)bt_accept_enqueue (parent, sk, true);
@@ -835,11 +843,8 @@ static void iso_sock_disconn(struct sock *sk) } sk->sk_state =BT_DISCONN ; -iso_conn_lock (iso_pi (sk)->conn); -hci_conn_drop (iso_pi (sk)->conn->hcon); -iso_pi (sk)->conn->hcon->iso_data =NULL ; -iso_pi (sk)->conn->hcon =NULL ; -iso_conn_unlock (iso_pi (sk)->conn); +if (!test_and_set_bit (ISO_CONN_DROPPED ,iso_pi (sk)->conn->flags)) +hci_conn_drop (iso_pi (sk)->conn->hcon); }static void __iso_sock_close (struct sock *sk)
@@ -2042,9 +2047,18 @@ static void iso_sock_ready(struct sock *sk)return ;lock_sock (sk); + +switch (sk->sk_state) { +case BT_DISCONN : +case BT_CLOSED : +release_sock (sk); +return ; + } +iso_sock_clear_timer (sk); sk->sk_state =BT_CONNECTED ; sk->sk_state_change (sk); +release_sock (sk); }
Signature · 1 line
2.55.0
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=1134197
---Test result---
Test Summary:
CheckPatch FAIL 6.73 seconds VerifyFixes PASS 0.08 seconds VerifySignedoff PASS 0.07 seconds GitLint FAIL 4.45 seconds SubjectPrefix PASS 1.68 seconds BuildKernel PASS 27.02 seconds CheckAllWarning PASS 29.21 seconds CheckSparse PASS 28.50 seconds BuildKernel32 PASS 25.88 seconds CheckKernelLLVM SKIP 0.00 seconds TestRunnerSetup PASS 494.62 seconds TestRunner_l2cap-tester PASS 60.21 seconds TestRunner_iso-tester PASS 75.92 seconds TestRunner_bnep-tester PASS 19.09 seconds TestRunner_mgmt-tester FAIL 215.54 seconds TestRunner_rfcomm-tester PASS 25.49 seconds TestRunner_sco-tester PASS 31.21 seconds TestRunner_ioctl-tester PASS 26.29 seconds TestRunner_mesh-tester FAIL 25.85 seconds TestRunner_smp-tester PASS 23.26 seconds TestRunner_userchan-tester PASS 20.08 seconds TestRunner_6lowpan-tester PASS 23.29 seconds IncrementalBuild PASS 63.55 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[01/11] Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #90:
Commit e824c0bbe0ec ("Bluetooth: ISO: clear iso_data always when detaching conn from hcon")
total: 0 errors, 1 warnings, 0 checks, 61 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/patch/14709653.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: GitLint - FAIL
Desc: Run gitlint
Output:
[01/11] Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
3: B1 Line exceeds max length (91>80): "Commit e824c0bbe0ec ("Bluetooth: ISO: clear iso_data always when detaching conn from hcon")"
23: B2 Line has trailing whitespace: " "
[11/11] Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero
6: B3 Line contains hard tab characters (\t): " [Task hdev->workqueue] [Task 2]"
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4
Failed Test Cases
Read Exp Feature - Success Failed 0.242 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
Failed Test Cases
Mesh - Send cancel - 1 Timed out 2.461 seconds Mesh - Send cancel - 2 Timed out 1.986 seconds
https://github.com/bluez/bluetooth-next/pull/491
---
Regards,
Linux Bluetooth
PATCH 2/11 Bluetooth: ISO: lock sk in iso_sock_getname
In reply to Pauli VirtanenAccessing iso_pi(sk)->conn requires lock_sock, which is not held here.
Fix by adding the lock/release.
--- net/bluetooth/iso.c | 4 ++++ 1 file changed, 4 insertions(+)
net/bluetooth/iso.c +4 -0
@@ -1472,6 +1472,8 @@ static int iso_sock_getname(struct socket *sock, struct sockaddr *addr,BT_DBG ("sock %p, sk %p" , sock, sk); +lock_sock (sk); + addr->sa_family =AF_BLUETOOTH ;if (peer) {
@@ -1493,6 +1495,8 @@ static int iso_sock_getname(struct socket *sock, struct sockaddr *addr, sa->iso_bdaddr_type =iso_pi (sk)->src_type; } +release_sock (sk); +return len; }
Signature · 1 line
2.55.0
PATCH 3/11 Bluetooth: ISO: lock sk in iso_connect_ind
In reply to Pauli VirtanenAccessing iso_pi(sk)->conn requires lock_sock, which is not taken in the "ev3" part of iso_connect_ind. It may also be NULL if socket has transitioned away from the LISTEN/CONNECT states before locking.
Fix by adding lock/release. Recheck hcon is valid after lock acquire where needed.
--- net/bluetooth/iso.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
net/bluetooth/iso.c +11 -8
@@ -2369,7 +2369,7 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)lock_sock (sk); - hcon =iso_pi (sk)->conn->hcon; + hcon =iso_pi (sk)->conn ?iso_pi (sk)->conn->hcon :NULL ;iso_pi (sk)->qos.bcast.encryption = ev2->encryption;if (ev2->num_bis <iso_pi (sk)->bc_num_bis)
@@ -2409,9 +2409,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)if (!sk) goto done; - hcon =iso_pi (sk)->conn->hcon; +lock_sock (sk); + + hcon =iso_pi (sk)->conn ?iso_pi (sk)->conn->hcon :NULL ;if (!hcon) - goto done; + goto release3;if (ev3->data_status ==LE_PA_DATA_TRUNCATED ) {/* The controller was unable to retrieve PA data. */
@@ -2419,12 +2421,12 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) HCI_MAX_PER_AD_TOT_LEN); hcon->le_per_adv_data_len =0 ; hcon->le_per_adv_data_offset =0 ; - goto done; + goto release3; }if (hcon->le_per_adv_data_offset + ev3->length >HCI_MAX_PER_AD_TOT_LEN ) - goto done; + goto release3;memcpy (hcon->le_per_adv_data + hcon->le_per_adv_data_offset, ev3->data, ev3->length);
@@ -2443,18 +2445,19 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) &base_len);if (!base || base_len >BASE_MAX_LENGTH ) - goto done; + goto release3; -lock_sock (sk);memcpy (iso_pi (sk)->base, base, base_len);iso_pi (sk)->base_len = base_len; -release_sock (sk); }else {/* This is a PA data fragment. Keep pa_data_len set to 0 * until all data has been reassembled. */ hcon->le_per_adv_data_len =0 ; } + +release3: +release_sock (sk); }else { sk =iso_get_sock (hdev, &hdev->bdaddr,BDADDR_ANY ,BT_LISTEN , iso_match_dst,BDADDR_ANY );
Signature · 1 line
2.55.0
PATCH 4/11 Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos
In reply to Pauli VirtanenIn iso.c check_bcast_qos(), missing bcast.timeout is not set to its default value, and appears typoed as bcast.sync_timeout.
Fix the typo.
--- net/bluetooth/iso.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/iso.c +1 -1
@@ -1796,7 +1796,7 @@ static bool check_bcast_qos(struct bt_iso_qos *qos)return false;if (!qos->bcast.timeout) - qos->bcast.sync_timeout =BT_ISO_SYNC_TIMEOUT ; + qos->bcast.timeout =BT_ISO_SYNC_TIMEOUT ;if (qos->bcast.timeout <0x000a || qos->bcast.timeout >0x4000 )return false;
Signature · 1 line
2.55.0
PATCH 5/11 Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis()
In reply to Pauli Virtaneniso_sock_rebind_bis() updates socket iso_pi(sk)->bc_num_bis before validating the BIS values, so it's possible to end up with bc_num_bis inconsistent.
Assign to iso_pi(sk)->bc_num_bis only after validation.
--- net/bluetooth/iso.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
net/bluetooth/iso.c +3 -3
@@ -1039,15 +1039,15 @@ static int iso_sock_rebind_bis(struct sock *sk, struct sockaddr_iso *sa, goto done; } -iso_pi (sk)->bc_num_bis = sa->iso_bc->bc_num_bis; - -for (int i =0 ; i <iso_pi (sk)->bc_num_bis; i++) +for (int i =0 ; i < sa->iso_bc->bc_num_bis; i++)if (sa->iso_bc->bc_bis[i] <0x01 || sa->iso_bc->bc_bis[i] >0x1f ) { err = -EINVAL ; goto done; } +iso_pi (sk)->bc_num_bis = sa->iso_bc->bc_num_bis; +memcpy (iso_pi (sk)->bc_bis, sa->iso_bc->bc_bis,iso_pi (sk)->bc_num_bis);
Signature · 1 line
2.55.0
PATCH 6/11 Bluetooth: ISO: hold sk properly in iso_conn_ready
In reply to Pauli Virtanensk deref in iso_conn_ready must be done either under conn->lock, or holding a refcount, to avoid concurrent close. conn->sk is currently accessed without either:
[Task 1] [Task 2]
iso_sock_release
iso_conn_ready
sk = conn->sk
lock_sock(sk)
conn->sk = NULL
lock_sock(sk)
release_sock(sk)
iso_sock_kill(sk)
UAF on sk deref
Fix possible UAF by holding sk refcount in iso_conn_ready(). Also recheck after lock_sock that the socket is still valid. Adjust locking so conn->sk is cleared only under lock_sock.
--- net/bluetooth/iso.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)
net/bluetooth/iso.c +23 -11
@@ -805,11 +805,13 @@ static void iso_sock_kill(struct sock *sk)BT_DBG ("sk %p state %d" , sk, sk->sk_state);/* Sock is dead, so set conn->sk to NULL to avoid possible UAF */ +lock_sock (sk);if (iso_pi (sk)->conn) {iso_conn_lock (iso_pi (sk)->conn);iso_pi (sk)->conn->sk =NULL ;iso_conn_unlock (iso_pi (sk)->conn); } +release_sock (sk);/* Kill poor orphan */ bt_sock_unlink (&iso_sk_list, sk);
@@ -2047,23 +2049,17 @@ static void iso_sock_ready(struct sock *sk) {BT_DBG ("sk %p" , sk); -if (!sk) -return ; - -lock_sock (sk); +lockdep_assert (lockdep_sock_is_held (sk));switch (sk->sk_state) {case BT_DISCONN :case BT_CLOSED : -release_sock (sk);return ; }iso_sock_clear_timer (sk); sk->sk_state =BT_CONNECTED ; sk->sk_state_change (sk); - -release_sock (sk); }static bool iso_match_big (struct sock *sk,void *data)
@@ -2093,7 +2089,7 @@ static bool iso_match_dst(struct sock *sk, void *data)static void iso_conn_ready (struct iso_conn *conn) {struct sock *parent =NULL ; -struct sock *sk = conn->sk; +struct sock *sk;struct hci_ev_le_big_sync_established *ev =NULL ;struct hci_ev_le_pa_sync_established *ev2 =NULL ;struct hci_ev_le_per_adv_report *ev3 =NULL ;
@@ -2102,7 +2098,22 @@ static void iso_conn_ready(struct iso_conn *conn)BT_DBG ("conn %p" , conn); +iso_conn_lock (conn); + sk =iso_sock_hold (conn); +iso_conn_unlock (conn); +if (sk) { +lock_sock (sk); + +/* conn->sk may have become NULL if racing with sk close, but +* due to held hdev->lock, it can't become different sk. +*/ +if (!conn->sk) { +release_sock (sk); +sock_put (sk); +return ; + } +/* Attempt to update source address in case of BIS Sender if * the advertisement is using a random address. */
@@ -2115,14 +2126,15 @@ static void iso_conn_ready(struct iso_conn *conn) adv =hci_find_adv_instance (bis->hdev, bis->iso_qos.bcast.bis);if (adv &&bacmp (&adv->random_addr,BDADDR_ANY )) { -lock_sock (sk);iso_pi (sk)->src_type =BDADDR_LE_RANDOM ;bacpy (&iso_pi (sk)->src, &adv->random_addr); -release_sock (sk); } } -iso_sock_ready (conn->sk); +iso_sock_ready (sk); + +release_sock (sk); +sock_put (sk); }else { hcon = conn->hcon;if (!hcon)
Signature · 1 line
2.55.0
PATCH 7/11 Bluetooth: ISO: fix leaking sk after socket release
In reply to Pauli Virtaneniso_sock_kill() tests !sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket || sock_flag(sk, SOCK_DEAD) for early return, but this is always true since sock_orphan(sk) sets SOCK_DEAD, so the sk reference released by socket always leaks, iso_sock_destruct is never called.
The socket reference also leaks when __iso_sock_close() does not set SOCK_ZAPPED, since iso_conn_del() does not call iso_sock_kill() after zapping.
Fix by replacing SOCK_DEAD by BT_SK_KILLED flag that is not used for something else, and lock_sock to ensure iso_sock_kill() puts sk only after socket release only once. Release and iso_conn_del may run concurrently. Call iso_sock_kill() from iso_conn_del() to clean sk up after zapping.
Remove call to iso_sock_kill() from iso_sock_close(), as it's generally no-op there.
--- net/bluetooth/iso.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
net/bluetooth/iso.c +18 -4
@@ -62,6 +62,7 @@ static void iso_sock_kill(struct sock *sk);enum {BT_SK_BIG_SYNC ,BT_SK_PA_SYNC , +BT_SK_KILLED , };struct iso_pinfo {
@@ -295,6 +296,7 @@ static void iso_conn_del(struct hci_conn *hcon, int err)iso_sock_clear_timer (sk);iso_chan_del (sk, err);release_sock (sk); +iso_sock_kill (sk);sock_put (sk); }
@@ -798,24 +800,29 @@ static void iso_sock_cleanup_listen(struct sock *parent)*/ static void iso_sock_kill (struct sock *sk) { +lock_sock (sk); +if (!sock_flag (sk,SOCK_ZAPPED ) || sk->sk_socket || -sock_flag (sk,SOCK_DEAD )) +test_bit (BT_SK_KILLED , &iso_pi (sk)->flags)) { +release_sock (sk);return ; + }BT_DBG ("sk %p state %d" , sk, sk->sk_state);/* Sock is dead, so set conn->sk to NULL to avoid possible UAF */ -lock_sock (sk);if (iso_pi (sk)->conn) {iso_conn_lock (iso_pi (sk)->conn);iso_pi (sk)->conn->sk =NULL ;iso_conn_unlock (iso_pi (sk)->conn); } -release_sock (sk);/* Kill poor orphan */ bt_sock_unlink (&iso_sk_list, sk);sock_set_flag (sk,SOCK_DEAD ); +set_bit (BT_SK_KILLED , &iso_pi (sk)->flags); + +release_sock (sk);sock_put (sk); }
@@ -892,7 +899,6 @@ static void iso_sock_close(struct sock *sk)iso_sock_clear_timer (sk);__iso_sock_close (sk);release_sock (sk); -iso_sock_kill (sk); }static void iso_sock_init (struct sock *sk,struct sock *parent)
@@ -2040,8 +2046,16 @@ static int iso_sock_release(struct socket *sock)release_sock (sk); } +/* Make sure sk is valid even if iso_conn_del() is concurrent */ +sock_hold (sk); + +lock_sock (sk);sock_orphan (sk); +release_sock (sk); +iso_sock_kill (sk); + +sock_put (sk);return err; }
Signature · 1 line
2.55.0
PATCH 8/11 Bluetooth: ISO: avoid deadlocks in iso_sock_timeout
In reply to Pauli Virtaneniso_sock_timeout() takes lock_sock, so sync disabling the timer while holding that lock may deadlock.
iso_sock_timeout() may also run concurrently with iso_conn_del(), which leads to UAF
[Task 1] [Task hdev->workqueue]
iso_sock_timeout iso_conn_del
iso_conn_hold_unless_zero iso_chan_del
`------------> iso_conn_put
caller frees hcon
iso_conn_put
iso_conn_free
conn->hcon->iso_data = NULL; /* UAF */Fix the deadlock by removing the disable from the lock_sock sections. Move the timer from iso_conn to iso_pinfo to decouple it from iso_conn which may need to be freed in lock_sock section. Convert some of the clear_timer to disable_timer.
--- net/bluetooth/iso.c | 60 ++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 31 deletions(-)
net/bluetooth/iso.c +29 -31
@@ -37,8 +37,6 @@ struct iso_conn {spinlock_t lock;struct sock *sk; -struct delayed_work timeout_work; -struct sk_buff *rx_skb;__u32 rx_len;__u16 tx_sn;
@@ -81,6 +79,7 @@ struct iso_pinfo {__u8 base_len;__u8 base[BASE_MAX_LENGTH ];struct iso_conn *conn; +struct delayed_work timeout_work; };static struct bt_iso_qos default_qos;
@@ -118,9 +117,6 @@ static void iso_conn_free(struct kref *ref)hci_conn_drop (conn->hcon); } -/* Ensure no more work items will run since hci_conn has been dropped */ -disable_delayed_work_sync (&conn->timeout_work); -kfree_skb (conn->rx_skb);kfree (conn);
@@ -161,48 +157,45 @@ static struct sock *iso_sock_hold(struct iso_conn *conn)static void iso_sock_timeout (struct work_struct *work) { - struct iso_conn *conn = container_of(work, struct iso_conn, - timeout_work.work); -struct sock *sk; - - conn =iso_conn_hold_unless_zero (conn); -if (!conn) -return ; - -iso_conn_lock (conn); - sk =iso_sock_hold (conn); -iso_conn_unlock (conn); -iso_conn_put (conn); - -if (!sk) -return ; + struct iso_pinfo *pi = container_of(work, struct iso_pinfo, + timeout_work.work); +struct sock *sk = &pi->bt.sk;BT_DBG ("sock %p state %d" , sk, sk->sk_state);lock_sock (sk); - sk->sk_err =ETIMEDOUT ; - sk->sk_state_change (sk); +if (!sock_flag (sk,SOCK_ZAPPED )) { + sk->sk_err =ETIMEDOUT ; + sk->sk_state_change (sk); + }release_sock (sk); -sock_put (sk); }static void iso_sock_set_timer (struct sock *sk,long timeout) { +lockdep_assert (lockdep_sock_is_held (sk)); + +cancel_delayed_work (&iso_pi (sk)->timeout_work); +if (!iso_pi (sk)->conn)return ;BT_DBG ("sock %p state %d timeout %ld" , sk, sk->sk_state, timeout); -cancel_delayed_work (&iso_pi (sk)->conn->timeout_work); -schedule_delayed_work (&iso_pi (sk)->conn->timeout_work, timeout); +schedule_delayed_work (&iso_pi (sk)->timeout_work, timeout); }static void iso_sock_clear_timer (struct sock *sk) { -if (!iso_pi (sk)->conn) -return ; +BT_DBG ("sock %p state %d" , sk, sk->sk_state); +cancel_delayed_work (&iso_pi (sk)->timeout_work); +} + +static void iso_sock_disable_timer (struct sock *sk) +{ +lockdep_assert (!lockdep_sock_is_held (sk));BT_DBG ("sock %p state %d" , sk, sk->sk_state); -cancel_delayed_work (&iso_pi (sk)->conn->timeout_work); +disable_delayed_work_sync (&iso_pi (sk)->timeout_work); }/* ---- ISO connections ---- */
@@ -227,7 +220,6 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)kref_init (&conn->ref);spin_lock_init (&conn->lock); -INIT_DELAYED_WORK (&conn->timeout_work, iso_sock_timeout); hcon->iso_data = conn; conn->hcon = hcon;
@@ -292,8 +284,9 @@ static void iso_conn_del(struct hci_conn *hcon, int err)return ; } +iso_sock_disable_timer (sk); +lock_sock (sk); -iso_sock_clear_timer (sk);iso_chan_del (sk, err);release_sock (sk);iso_sock_kill (sk);
@@ -800,6 +793,8 @@ static void iso_sock_cleanup_listen(struct sock *parent)*/ static void iso_sock_kill (struct sock *sk) { +iso_sock_disable_timer (sk); +lock_sock (sk); if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
@@ -895,8 +890,9 @@ static void __iso_sock_close(struct sock *sk)/* Must be called on unlocked socket. */ static void iso_sock_close (struct sock *sk) { +iso_sock_disable_timer (sk); +lock_sock (sk); -iso_sock_clear_timer (sk);__iso_sock_close (sk);release_sock (sk); }
@@ -965,6 +961,8 @@ static struct sock *iso_sock_alloc(struct net *net, struct socket *sock,iso_pi (sk)->qos = default_qos;iso_pi (sk)->sync_handle =-1 ; +INIT_DELAYED_WORK (&iso_pi (sk)->timeout_work, iso_sock_timeout); +bt_sock_link (&iso_sk_list, sk);return sk; }
Signature · 1 line
2.55.0
PATCH 9/11 Bluetooth: ISO: ensure no dangling hcon references in iso_conn
In reply to Pauli VirtanenAfter iso_conn_del(), ISO sockets should not dereference the hcon any more. Currently, clearing iso_conn::hcon relies on iso_conn_del() releasing the last reference to the iso_conn.
Simplify this by explicitly clearing conn->hcon in iso_conn_del(), to avoid more complex reasoning on races about who holds the last reference.
--- net/bluetooth/iso.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)
net/bluetooth/iso.c +21 -3
@@ -263,6 +263,7 @@ static void iso_chan_del(struct sock *sk, int err) }static void iso_conn_del (struct hci_conn *hcon,int err) + __must_hold(&hcon->hdev->lock) {struct iso_conn *conn = hcon->iso_data;struct sock *sk;
@@ -277,11 +278,10 @@ static void iso_conn_del(struct hci_conn *hcon, int err)iso_conn_lock (conn); sk =iso_sock_hold (conn);iso_conn_unlock (conn); -iso_conn_put (conn);if (!sk) {iso_conn_put (conn); -return ; + goto done; }iso_sock_disable_timer (sk);
@@ -291,6 +291,15 @@ static void iso_conn_del(struct hci_conn *hcon, int err)release_sock (sk);iso_sock_kill (sk);sock_put (sk); + +done: +/* No sk access to conn->hcon any more (lock_sock + hdev->lock) */ +iso_conn_lock (conn); + conn->hcon =NULL ; + hcon->iso_data =NULL ; +iso_conn_unlock (conn); + +iso_conn_put (conn); } static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
@@ -306,6 +315,11 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,return -EBUSY ; } +if (!conn->hcon) { +BT_ERR ("conn->hcon missing" ); +return -EIO ; + } +iso_pi (sk)->conn = conn; conn->sk = sk;clear_bit (ISO_CONN_DROPPED , conn->flags);
@@ -2500,6 +2514,7 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) }static void iso_connect_cfm (struct hci_conn *hcon,__u8 status) + __must_hold(&hcon->hdev->lock) {if (hcon->type !=CIS_LINK && hcon->type !=BIS_LINK && hcon->type !=PA_LINK ) {
@@ -2511,8 +2526,10 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)struct hci_link *link, *t;list_for_each_entry_safe (link, t, &hcon->link_list, - list) + list) { +lockdep_assert_held (&link->conn->hdev->lock);iso_conn_del (link->conn,bt_to_errno (status)); + }return ; }
@@ -2542,6 +2559,7 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status) }static void iso_disconn_cfm (struct hci_conn *hcon,__u8 reason) + __must_hold(&hcon->hdev->lock) {if (hcon->type !=CIS_LINK && hcon->type !=BIS_LINK && hcon->type !=PA_LINK )
Signature · 1 line
2.55.0
PATCH 10/11 Bluetooth: ISO: fix refcounting of iso_conn
In reply to Pauli Virtaneniso_conn_del() and iso_chan_del() have a race that results to double-put of iso_conn:
[Task hdev->workqueue] [Task 2]
iso_conn_del iso_chan_del
iso_conn_hold_unless_zero iso_conn_lock
iso_conn_lock conn->sk = NULL
iso_conn_unlock
sk = iso_sock_hold(conn) <---------´
if (!sk) iso_conn_put iso_conn_put
iso_conn_put /* UAF */The extra put for !sk in iso_conn_del() is currently required since failing iso_chan_add() may leave iso_conn not associated with any sk.
Fix by having iso_pi(sk)->conn own refcount when non-NULL, so iso_conn_del does not need to put it. Adjust the iso_conn_add() refcounting so that conn is put if it does not get associated with an sk.
--- net/bluetooth/iso.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
net/bluetooth/iso.c +17 -12
@@ -108,9 +108,6 @@ static void iso_conn_free(struct kref *ref)BT_DBG ("conn %p" , conn); -if (conn->sk) -iso_pi (conn->sk)->conn =NULL ; -if (conn->hcon) { conn->hcon->iso_data =NULL ;if (!test_and_set_bit (ISO_CONN_DROPPED , conn->flags))
@@ -145,6 +142,14 @@ static struct iso_conn *iso_conn_hold_unless_zero(struct iso_conn *conn)return conn; } +static struct iso_conn *iso_conn_hold (struct iso_conn *conn) +{ +BT_DBG ("conn %p refcnt %u" , conn,kref_read (&conn->ref)); + +kref_get (&conn->ref); +return conn; +} +static struct sock *iso_sock_hold (struct iso_conn *conn) {if (!conn || !bt_sock_linked (&iso_sk_list, conn->sk))
@@ -210,7 +215,6 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon) conn->hcon = hcon;iso_conn_unlock (conn); } -iso_conn_put (conn);return conn; }
@@ -279,10 +283,8 @@ static void iso_conn_del(struct hci_conn *hcon, int err) sk =iso_sock_hold (conn);iso_conn_unlock (conn); -if (!sk) { -iso_conn_put (conn); +if (!sk) goto done; - }iso_sock_disable_timer (sk);
@@ -320,7 +322,7 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,return -EIO ; } -iso_pi (sk)->conn = conn; +iso_pi (sk)->conn =iso_conn_hold (conn); conn->sk = sk;clear_bit (ISO_CONN_DROPPED , conn->flags);
@@ -427,6 +429,7 @@ static int iso_connect_bis(struct sock *sk) } err =iso_chan_add (conn, sk,NULL ); +iso_conn_put (conn);if (err) goto unlock;
@@ -529,6 +532,7 @@ static int iso_connect_cis(struct sock *sk) } err =iso_chan_add (conn, sk,NULL ); +iso_conn_put (conn);if (err) goto unlock;
@@ -1310,10 +1314,9 @@ static int iso_listen_bis(struct sock *sk) } err =iso_chan_add (conn, sk,NULL ); -if (err) { -hci_conn_drop (hcon); +iso_conn_put (conn); +if (err) goto unlock; - } unlock:release_sock (sk);
@@ -2551,8 +2554,10 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)struct iso_conn *conn; conn =iso_conn_add (hcon); -if (conn) +if (conn) {iso_conn_ready (conn); +iso_conn_put (conn); + } }else {iso_conn_del (hcon,bt_to_errno (status)); }
Signature · 1 line
2.55.0
PATCH 11/11 Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero
In reply to Pauli Virtanenhci_conn::iso_data is accessed and modified without lock or RCU. This leads to a race
[Task hdev->workqueue] [Task 2]
iso_recv iso_conn_put(conn)
conn = LOAD hcon->iso_data iso_conn_free(conn)
iso_conn_hold_unless_zero(conn) hcon->iso_data = NULL
kfree(conn)
kref_get_unless_zero(&conn->ref) /* UAF */and also to races in iso_conn_add() vs. iso_conn_free().
Fix by adding spinlock hci_conn::proto_lock and using it to guard hci_conn::iso_data.
--- include/net/bluetooth/hci_core.h | 4 +- net/bluetooth/hci_conn.c | 2 + net/bluetooth/iso.c | 64 ++++++++++++++++++++++++++------ 3 files changed, 58 insertions(+), 12 deletions(-)
include/net/bluetooth/hci_core.h +3 -1
@@ -767,9 +767,11 @@ struct hci_conn {struct dentry *debugfs;struct hci_dev *hdev; + +spinlock_t proto_lock;/* lock guarding protocol data */ void *l2cap_data;void *sco_data; -void *iso_data; + void *iso_data __guarded_by(&proto_lock);struct list_head link_list;struct hci_conn *parent;
net/bluetooth/hci_conn.c +2 -0
@@ -1123,6 +1123,8 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type,INIT_DELAYED_WORK (&conn->idle_work, hci_conn_idle);INIT_DELAYED_WORK (&conn->le_conn_timeout, le_conn_timeout); +spin_lock_init (&conn->proto_lock); +atomic_set (&conn->refcnt,0 );hci_dev_hold (hdev);
net/bluetooth/iso.c +53 -11
150 lines not on this page · raw message
Signature · 1 line
2.55.0
pe, 2026-07-24 kello 23:20 +0300, Pauli Virtanen kirjoitti:
Quoted 5 lines
The retracted patch in commit
e824c0bbe0ec9("Bluetooth: ISO: cleariso_dataalways when detaching conn from hcon") merged in bluetooth-next/master broke ISO socket transition toBT_CLOSEDso they don't send POLLHUP properly any more, as seen in "ISO Disconnect - Success" test.
On Sashiko comments:
https://sashiko.dev/#/patchset/cover.1784923689.git.pav%40iki.fi
Patch 3: pre-existing issue, not UAF/locking issue so maybe should be in separate series.
Patch 9: the comment appears to be the UAF which is fixed by the next Patch 10 as described in its commit message. This was also pre-existing (SCO has the same race).
Patch 10: these comments seem confused (no rcu_read_lock in iso_conn_put in this patch, doesn't seem to notice synchronize_rcu in hci_conn_hash_del?), but remaining UAF/locking issues should be fixed by Patch 11.
Patch 11: the comment is about hci_conn_hold/drop refcount when iso_conn_free and iso_conn_add happen to execute exactly at the same time, with the same pre-existing hci_conn.
hci_connect_cis/bis don't give a new refcount when the hci_conn is pre- existing and usable, which should mitigate this, the refcount stays at 1 and becomes owned by the new iso_conn. IIUC it should be OK as it is.
Quoted 65 lines
This is rebased version of and supercedes
https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/The first commit in this series fixes that, and subsequent mostly independent commits other minor issues found looking at the ISO socket code.
The last three fix UAF issues. The locking/lifetime of
iso_conncomplicates the code here and the last three patches can be replaced by somewhat simpler solution with bigger diff that removesiso_conn
https://github.com/pv/linux/commit/e4b460b5f7aaba060b958229aaec6b2217642178Fixed issues aside
BT_CONNECT, these appear to be pre-existing in several previous kernel releases:- sk is always leaked on socket release
-
timeout_workmay deadlock under certain conditions- correctness of
iso_conn_del()vs UAF requires somewhat too complex reasoning about race conditions, and it's not quite correct[Task 1] [Task hdev->workqueue] iso_sock_timeout iso_conn_del iso_conn_hold_unless_zero iso_chan_del `------------> iso_conn_put caller frees hcon iso_conn_put iso_conn_free conn->hcon->iso_data = NULL; /* UAF */- attempt to free
iso_conniniso_conn_del()races withiso_conn_delwith potential UAF-
iso_sock_ready()getsconn->skwithout requisite locks, this should be done like insco.c-
iso_sock_getname,iso_connect_indare missinglock_sockand NULL checks-
kref_get_unless_zero(&((struct iso_conn *)hcon->iso_data)->ref)requires synchronization primitive.Tested vs iso-tester, Pipewire Qemu ucast/bcast audio tests, and real ucast audio streaming, which pass without KASAN/locking splats. Also checked iso-tester produces balanced
iso_sock_init/iso_sock_destructkprints.Pauli Virtanen (11):
Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
Bluetooth: ISO: lock sk iniso_sock_getname
Bluetooth: ISO: lock sk iniso_connect_ind
Bluetooth: ISO: fix timeout vssync_timeouttypo incheck_bcast_qos
Bluetooth: ISO: validatesockaddr_isofirst iniso_sock_rebind_bis()
Bluetooth: ISO: hold sk properly iniso_conn_ready
Bluetooth: ISO: fix leaking sk after socket release
Bluetooth: ISO: avoid deadlocks iniso_sock_timeout
Bluetooth: ISO: ensure no dangling hcon references iniso_conn
Bluetooth: ISO: fix refcounting ofiso_conn
Bluetooth: ISO: fix race of kfree vskref_get_unless_zeroinclude/net/bluetooth/hci_core.h | 4 +- net/bluetooth/hci_conn.c | 2 + net/bluetooth/iso.c | 274 ++++++++++++++++++++++--------- 3 files changed, 197 insertions(+), 83 deletions(-)
Signature · 1 line
Pauli Virtanen
This series was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:
Hello:
This series was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <[email protected]>:
Quoted 10 lines · On Fri, 24 Jul 2026 23:20:23 +0300 you wrote:
The retracted patch in commit
e824c0bbe0ec9("Bluetooth: ISO: cleariso_dataalways when detaching conn from hcon") merged in bluetooth-next/master broke ISO socket transition toBT_CLOSEDso they don't send POLLHUP properly any more, as seen in "ISO Disconnect - Success" test.This is rebased version of and supercedes
https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/[...]
Here is the summary with links:
- [01/11] Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
https://git.kernel.org/bluetooth/bluetooth-next/c/278aeca60782 - [02/11] Bluetooth: ISO: lock sk in iso_sock_getname https://git.kernel.org/bluetooth/bluetooth-next/c/6d16b799d7f4 - [03/11] Bluetooth: ISO: lock sk in iso_connect_ind https://git.kernel.org/bluetooth/bluetooth-next/c/df810355bbe7 - [04/11] Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos https://git.kernel.org/bluetooth/bluetooth-next/c/c08593655456 - [05/11] Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis() https://git.kernel.org/bluetooth/bluetooth-next/c/126e8f096c10 - [06/11] Bluetooth: ISO: hold sk properly in iso_conn_ready https://git.kernel.org/bluetooth/bluetooth-next/c/deb1d93a07f0 - [07/11] Bluetooth: ISO: fix leaking sk after socket release https://git.kernel.org/bluetooth/bluetooth-next/c/2a93966653a9 - [08/11] Bluetooth: ISO: avoid deadlocks in iso_sock_timeout https://git.kernel.org/bluetooth/bluetooth-next/c/f8cb802462f5 - [09/11] Bluetooth: ISO: ensure no dangling hcon references in iso_conn https://git.kernel.org/bluetooth/bluetooth-next/c/f80c3b2232a2 - [10/11] Bluetooth: ISO: fix refcounting of iso_conn https://git.kernel.org/bluetooth/bluetooth-next/c/e4d507fb7f6e - [11/11] Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero https://git.kernel.org/bluetooth/bluetooth-next/c/4b56754e9da3
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html