Re: [PATCH v2] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[Pauli Virtanen]

Hi,

ti, 2026-07-28 kello 01:26 -0300, Aldo Ariel Panzardo kirjoitti:
> sco_recv_scodata() upgrades the weak hcon->sco_data back-pointer to a
> strong reference under hdev->lock:

iso.c in current bluetooth-next/master kept the weak-reference iso_data
and addressed races in a different way (I had alternative unsent patch
also for the strong ref approach).

Either one should work, strong ref may be a bit simpler.

>     hci_dev_lock(hdev);
>     hcon = hci_conn_hash_lookup_handle(hdev, handle);
>     ...
>     conn = sco_conn_hold_unless_zero(hcon->sco_data);
>     hci_dev_unlock(hdev);
> 
> but the pointer is cleared from the other side without that lock. When
> the last sco_conn reference is dropped, sco_conn_free() ran
> 
>     conn->hcon->sco_data = NULL;
> 
> with no hdev->lock held, so the RX path could read hcon->sco_data and
> call kref_get_unless_zero() on an sco_conn that was concurrently freed:
> 
>     BUG: KASAN: slab-use-after-free in sco_conn_hold_unless_zero+0xbe/0x160
>     Write of size 4 by task kworker/u17:0
>     Workqueue: hci0 hci_rx_work
>     Call Trace:
>      sco_conn_hold_unless_zero+0xbe/0x160
>      sco_recv_scodata+0x13f/0x490
>      hci_rx_work+0x3af/0x730
> 
> kref_get_unless_zero() only guards against a zero refcount, not against
> the backing memory already being freed.
> 
> Give hcon->sco_data an actual reference on the sco_conn it points to, so
> the object cannot be freed while the pointer is still installed, and only
> clear and drop it from sco_conn_del(), which runs under hdev->lock (its
> callers, sco_connect_cfm() and sco_disconn_cfm(), hold it). The reader in
> sco_recv_scodata() also takes hdev->lock, so the store and the read are
> now serialised: the RX path either observes NULL or a reference that is
> guaranteed to stay valid until it drops its own.
> 
> sco_conn_add() no longer hands its kref_init() reference to the caller as
> the connection's only reference; that initial reference is the one owned
> by hcon->sco_data, and callers get their own via sco_conn_hold().
> 
> Because the sco_conn can now outlive the socket (the association keeps it
> alive until the link goes down), the hci_conn can no longer be dropped
> from sco_conn_free() without regressing socket close: closing a connected
> SCO socket must still tear the link down. Move hci_conn ownership to the
> socket instead: __sco_chan_add() takes an hci_conn reference and it is
> released from sco_chan_del() and sco_sock_destruct(), exactly once. The
> sco_conn no longer owns an hci_conn reference, so sco_conn_add() stops
> consuming one and sco_connect() drops the hci_connect_sco() reference on
> every path; sco_connect_cfm() no longer needs its hci_conn_hold() dance.
> 
> Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Pauli Virtanen <pav@xxxxxx>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>
> ---
> v2: quote the inlined code and KASAN splat in the commit message with
>     spaces instead of hard tabs, to satisfy the bluez CI gitlint check;
>     no functional change (CI sco-tester and sparse pass).
> 
>  net/bluetooth/sco.c | 84 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 15 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 3d4362a09df4..8df0fa48fc60 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -84,12 +84,14 @@ static void sco_conn_free(struct kref *ref)
>  	if (conn->sk)
>  		sco_pi(conn->sk)->conn = NULL;
>  
> -	if (conn->hcon) {
> -		conn->hcon->sco_data = NULL;
> -		hci_conn_drop(conn->hcon);
> -	}
> +	/* hcon->sco_data is cleared and the association's reference on the
> +	 * sco_conn is dropped in sco_conn_del() under hdev->lock, and the
> +	 * hci_conn is now owned by the socket (held in __sco_chan_add() and
> +	 * dropped in sco_chan_del()/sco_sock_destruct()), so there is nothing
> +	 * left to release towards hcon here.
> +	 */
>  
> -	/* Ensure no more work items will run since hci_conn has been dropped */
> +	/* Ensure no more work items will run before the connection is freed */
>  	disable_delayed_work_sync(&conn->timeout_work);
>  
>  	kfree(conn);
> @@ -188,8 +190,11 @@ static void sco_sock_clear_timer(struct sock *sk)
>  }
>  
>  /* ---- SCO connections ---- */
> -/* Consumes a reference on @hcon, which the returned sco_conn owns until it is
> - * freed. On failure (NULL return) the reference is left for the caller to drop.
> +/* Returns a new reference the caller must drop with sco_conn_put(). The
> + * hcon->sco_data association holds its own reference on the sco_conn for the
> + * connection's lifetime; it is dropped in sco_conn_del() under hdev->lock.
> + * @hcon is not consumed: the hci_conn reference is taken and owned by the
> + * socket in __sco_chan_add().
>   */
>  static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>  {
> @@ -201,9 +206,6 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>  			sco_conn_lock(conn);
>  			conn->hcon = hcon;
>  			sco_conn_unlock(conn);

The above is probably dead code now, is conn->hcon == NULL && hcon-
>sco_data != NULL possible?

> -		} else {
> -			/* conn already owns a reference on hcon */
> -			hci_conn_drop(hcon);
>  		}
>  		return conn;
>  	}
> @@ -227,7 +229,10 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>  
>  	BT_DBG("hcon %p conn %p", hcon, conn);
>  
> -	return conn;
> +	/* kref_init() above set the association reference owned by
> +	 * hcon->sco_data; hand the caller its own reference.
> +	 */
> +	return sco_conn_hold(conn);
>  }
>  
>  /* Delete channel.
> @@ -242,9 +247,20 @@ static void sco_chan_del(struct sock *sk, int err)
>  	BT_DBG("sk %p, conn %p, err %d", sk, conn, err);
>  
>  	if (conn) {
> +		struct hci_conn *hcon;
> +
>  		sco_conn_lock(conn);
>  		conn->sk = NULL;
> +		hcon = conn->hcon;
>  		sco_conn_unlock(conn);
> +
> +		/* Release the socket's own reference on the hci_conn taken in
> +		 * __sco_chan_add() so that closing the socket still tears the
> +		 * link down even while the sco_conn is kept alive by the
> +		 * hcon->sco_data association reference.
> +		 */
> +		if (hcon)
> +			hci_conn_drop(hcon);

UAF race here, similarly in sco_sock_destruct()

    [Task 1]                          [Task hdev->workqueue]
    sco_chan_del()                    sco_conn_del()
       sco_conn_lock                    conn = sco_conn_hold_unless_zero(conn);
       conn->sk = NULL                  hcon->sco_data = NULL;
       sco_conn_unlock -------------->  sco_conn_lock
                                        sk = sco_sock_hold(conn); /* -> NULL */
                                        sco_conn_unlock
					if (!sk) return;
                                      /* caller frees hci_conn */
       hci_conn_drop(hcon) /* UAF */

Moving the drop before conn->sk = NULL probably fixes this.

Probably a good idea to have GPT or other recent AI model review your
patch (give sco.c and the patch as context) with prompt that explains
the hci_conn lifetime (iso_conn_del called before free, hci_conn_hold
does not prevent free).

Theres AI review at sashiko.dev which often useful, but it has trouble
understanding hci_conn lifetime, and it didn't get this one.

>  		sco_conn_put(conn);
>  	}
>  
> @@ -266,6 +282,15 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>  
>  	BT_DBG("hcon %p conn %p, err %d", hcon, conn, err);
>  
> +	/* Detach the connection from the hci_conn and drop the reference held
> +	 * by the hcon->sco_data association.  The caller holds hdev->lock,
> +	 * which serialises this NULL store and put against the read of
> +	 * hcon->sco_data in sco_recv_scodata(), closing the use-after-free
> +	 * where the RX path could upgrade an already-freed sco_conn.
> +	 */
> +	hcon->sco_data = NULL;
> +	sco_conn_put(conn);

In this design you can add `void *sco_data __guarded_by(&hdev->lock)`
context analysis annotation, so there's no need for the temporary hold
of sco_data in sco_conn_del() as access is serialized by hdev lock.

> +
>  	sco_conn_lock(conn);
>  	sk = sco_sock_hold(conn);
>  	sco_conn_unlock(conn);
> @@ -290,6 +315,13 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  	sco_pi(sk)->conn = sco_conn_hold(conn);
>  	conn->sk = sk;
>  
> +	/* The socket owns a reference on the hci_conn for as long as it stays
> +	 * attached; it is dropped in sco_chan_del()/sco_sock_destruct().  This
> +	 * keeps close() tearing the link down now that the sco_conn can outlive
> +	 * the socket through the hcon->sco_data association reference.
> +	 */
> +	hci_conn_hold(conn->hcon);
> +
>  	if (parent)
>  		bt_accept_enqueue(parent, sk, true);
>  }
> @@ -371,6 +403,7 @@ 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;
>  	}
> @@ -379,6 +412,7 @@ static int sco_connect(struct sock *sk)
>  	sco_conn_put(conn);
>  	if (err) {
>  		release_sock(sk);
> +		hci_conn_drop(hcon);
>  		goto unlock;
>  	}
>  
> @@ -395,6 +429,11 @@ static int sco_connect(struct sock *sk)
>  
>  	release_sock(sk);
>  
> +	/* The socket took its own hci_conn reference in __sco_chan_add(); drop
> +	 * the one returned by hci_connect_sco().
> +	 */
> +	hci_conn_drop(hcon);

You can move this after __sco_chan_add, probably doesn't need comment.

> +
>  unlock:
>  	hci_dev_unlock(hdev);
>  	hci_dev_put(hdev);
> @@ -495,9 +534,26 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src)
>  
>  static void sco_sock_destruct(struct sock *sk)
>  {
> +	struct sco_conn *conn = sco_pi(sk)->conn;
> +
>  	BT_DBG("sk %p", sk);
>  
> -	sco_conn_put(sco_pi(sk)->conn);
> +	/* If the channel was not already torn down via sco_chan_del(), drop the
> +	 * socket's own references here.  sco_chan_del() clears sco_pi(sk)->conn,
> +	 * so the hci_conn and sco_conn references are released exactly once.
> +	 */
> +	if (conn) {
> +		struct hci_conn *hcon;
> +
> +		sco_conn_lock(conn);
> +		hcon = conn->hcon;
> +		sco_conn_unlock(conn);
> +
> +		if (hcon)
> +			hci_conn_drop(hcon);

UAF since conn->hcon is not valid after conn->sk = NULL in
sco_sock_kill.

> +		sco_pi(sk)->conn = NULL;
> +		sco_conn_put(conn);
> +	}
>  
>  	skb_queue_purge(&sk->sk_receive_queue);
>  	skb_queue_purge(&sk->sk_write_queue);
> @@ -1511,12 +1567,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status)
>  	if (!status) {
>  		struct sco_conn *conn;
>  
> -		conn = sco_conn_add(hci_conn_hold(hcon));
> +		conn = sco_conn_add(hcon);
>  		if (conn) {
>  			sco_conn_ready(conn);
>  			sco_conn_put(conn);
> -		} else {
> -			hci_conn_drop(hcon);
>  		}
>  	} else
>  		sco_conn_del(hcon, bt_to_errno(status));

-- 
Pauli Virtanen