Re: [PATCH] Bluetooth: SCO: fix refcount over-put in sco_conn_del()

Hi,

to, 2026-07-23 kello 19:10 -0300, Your Name kirjoitti:
> From: Aldo Ariel Panzardo <[email protected]>
> 
> 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: [email protected]
> Signed-off-by: Aldo Ariel Panzardo <[email protected]>
> ---
>  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