* Re: [PATCH] Bluetooth: SCO: fix refcount over-put in sco_conn_del()
2026-07-23 22:10 [PATCH] Bluetooth: SCO: fix refcount over-put in sco_conn_del() Your Name
@ 2026-07-23 22:50 ` Pauli Virtanen
0 siblings, 0 replies; 2+ messages in thread
From: Pauli Virtanen @ 2026-07-23 22:50 UTC (permalink / raw)
To: Your Name, linux-bluetooth; +Cc: marcel, luiz.dentz, linux-kernel, stable
Hi,
to, 2026-07-23 kello 19:10 -0300, Your Name kirjoitti:
> From: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
>
> sco_conn_del() takes exactly one transient reference to the sco_conn via
> sco_conn_hold_unless_zero() and drops it with the sco_conn_put() that
> follows sco_sock_hold(). The additional sco_conn_put() in the !sk branch
> drops a reference the function never acquired:
>
> conn = sco_conn_hold_unless_zero(conn);
> if (!conn)
> return;
> ...
> sco_conn_lock(conn);
> sk = sco_sock_hold(conn);
> sco_conn_unlock(conn);
> sco_conn_put(conn);
>
> if (!sk) {
> sco_conn_put(conn); /* drops a reference we do not own */
> return;
> }
>
> sco_sock_timeout() has the same entry semantics -- one
> sco_conn_hold_unless_zero(), no incoming reference owned -- and simply
> returns from its !sk branch with no second put.
>
> In steady state the only counted reference to a struct sco_conn is the
> one held by the socket. When close() races the controller's Disconnection
> Complete, sco_chan_del() clears conn->sk and drops the socket reference
> while sco_conn_del() is running. sco_conn_del() then observes sk == NULL,
> its own put drops the count to zero and frees the conn, and the second put
> writes to the freed kref:
>
> BUG: KASAN: slab-use-after-free in sco_conn_put.part.0+0x1a/0x190
> Write of size 4 at addr ffff8881099dec74 by task kworker/u17:3/413
> Workqueue: hci1 hci_rx_work
> Call Trace:
> sco_conn_put.part.0+0x1a/0x190
> hci_disconn_complete_evt+0x1ee/0x3e0
> hci_event_packet+0x54a/0x650
> hci_rx_work+0x321/0x3d0
> Allocated by task 413:
> sco_conn_add+0x72/0x1a0
> sco_connect_cfm+0x88/0x670
> hci_conn_complete_evt+0x4c5/0x890
> Freed by task 413:
> sco_conn_del.isra.0+0x3f/0xf0
> hci_disconn_complete_evt+0x1ee/0x3e0
> refcount_t: underflow; use-after-free.
>
> The freed object is a kmalloc-128 allocation, so this is an out-of-bounds
> write into a reclaimed slab object rather than a plain crash. Opening an
> SCO socket requires no capability, and the race is reached whenever the
> controller delivers a Disconnection Complete concurrently with the socket
> being closed.
>
> A related refcount imbalance introduced by the same commit was already
> fixed in commit ed9588554943 ("Bluetooth: SCO: remove the redundant
> sco_conn_put").
>
> Drop the extra put so the !sk branch simply returns.
>
> Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
> net/bluetooth/sco.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index fcc597be5bbd..6502fcdfc0f0 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -265,10 +265,8 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
> sco_conn_unlock(conn);
> sco_conn_put(conn);
>
> - if (!sk) {
> - sco_conn_put(conn);
> + if (!sk)
> return;
This will leak the sco_conn under some conditions, you probably need
something like the below and check all code paths touching the sco_conn
are consistent who owns the reference.
Previously the idea appears to have been it is sometimes owned by
hci_conn::sco_data but it's probably simpler vs race conditions to have
it owned only by sco_pi(sk)->conn.
With this change, sco_conn_free() probably should not touch
sco_pi(conn->sk)->conn
Please also check the hci_conn_drop() refcounting vs. hci_connect_sco()
is OK.
Please also describe to what extent this was tested.
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index c05f79b7aa31..3000b723f081 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -283,7 +283,7 @@ static void __sco_chan_add(struct sco_conn *conn,
struct sock *sk,
{
BT_DBG("conn %p", conn);
- sco_pi(sk)->conn = conn;
+ sco_pi(sk)->conn = sco_conn_hold(conn);
conn->sk = sk;
if (parent)
@@ -366,12 +366,14 @@ static int sco_connect(struct sock *sk)
*/
if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
release_sock(sk);
+ sco_conn_put(conn);
hci_conn_drop(hcon);
err = -EBADFD;
goto unlock;
}
err = sco_chan_add(conn, sk, NULL);
+ sco_conn_put(conn);
if (err) {
release_sock(sk);
hci_conn_drop(hcon);
@@ -1452,7 +1454,6 @@ static void sco_conn_ready(struct sco_conn *conn)
bacpy(&sco_pi(sk)->src, &conn->hcon->src);
bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
- sco_conn_hold(conn);
hci_conn_hold(conn->hcon);
__sco_chan_add(conn, sk, parent);
> - }
>
> /* Kill socket */
> lock_sock(sk);
--
Pauli Virtanen
^ permalink raw reply related [flat|nested] 2+ messages in thread