Re: [PATCH] Bluetooth: SCO: fix sco_conn double free on outgoing connect

Hi,

su, 2026-07-26 kello 14:54 +0900, Baul Lee kirjoitti:
> sco_conn is refcounted with a kref that sco_conn_add() initialises to 1.
> That single reference is the connection's association reference, owned
> by the hcon and released when the link goes down.
> 
> The incoming attach path takes an extra reference for the socket before
> __sco_chan_add(), but the outgoing path, sco_connect() -> sco_chan_add(),
> does not.  An outgoing SCO socket is therefore attached while conn->ref
> is still 1, and two unrelated teardown paths both release that one
> reference: sco_chan_del(), reached from close(), and the !sk branch of
> sco_conn_del(), reached from the sco_connect_cfm() / sco_disconn_cfm()
> callbacks.

This appears to break closing SCO connections, indeed "SCO Disconnect -
Success" fails now, due to hci_conn_drop() not called on socket close
since one sco_conn refcount is now owned by hci_conn::sco_data.

The socket should own a hci_conn_hold/drop refcount that it drops when
closing.

Another option is to have only socket own sco_conn, like here
https://lore.kernel.org/linux-bluetooth/[email protected]/

However, in that solution one would still need additional
synchronization to deal with the race between sco_conn_del(),
sco_conn_free(), sco_conn_add() vs. hci_conn::sco_data access
https://lore.kernel.org/linux-bluetooth/b84fd01c20f0d2975c7817da345a6937d67cf314.1784923689.git.pav@iki.fi/


In the patch here there seems to be some remaining UAF, could make
sense to try to address them while at it

sco_chan_del()				sco_conn_del()
  sco_conn_lock
  conn->sk = NULL
  sco_conn_unlock
					sk = ... /* = NULL */
  					if (!sk)
					  { sco_conn_put; return; }
					/* free hci_conn */
  sco_conn_put
    conn->hcon->sco_data = NULL /* UAF */

and

sco_sock_timeout()			sco_conn_del()
  conn = ...				conn = ...
  ...	  				...
					sco_conn_put()
					free hci_conn
  sco_conn_put()
    conn->hcon->sco_data = NULL /* UAF */

since access to sco_data is not serialized by any lock. Probably it can
be cleared in sco_conn_del() so it can get
__guarded_by(&hdev->lock) context analysis annotation
https://docs.kernel.org/dev-tools/context-analysis.html

> When an outgoing SCO setup fails while the socket is being closed, the
> two race, starting from conn->ref == 1:
> 
>   1. sco_chan_del() clears conn->sk.
>   2. sco_conn_del() takes a temporary reference (ref 2) and, seeing
>      conn->sk already cleared, gets a NULL sk.
>   3. sco_chan_del() puts the reference it believes it owns (ref 1).
>   4. sco_conn_del() drops its temporary reference (ref 0) and the
>      sco_conn is freed.
>   5. sco_conn_del() takes the !sk branch and puts the freed object.
> 
> KASAN reports a slab-use-after-free of the kmalloc-128 sco_conn in
> sco_chan_del(), followed by a refcount_t underflow.  Both operations are
> reachable from an unprivileged AF_BLUETOOTH / BTPROTO_SCO socket doing
> connect() and close().
> 
> Give the socket its own reference on the outgoing attach so that the two
> teardown owners no longer contend for a single reference, and drop the
> association reference in sco_conn_del()'s socket-kill path so that it is
> released exactly once there as well, symmetrically with the existing !sk
> branch.  sco_conn_ready() consequently has to take both references
> itself, because sco_connect_cfm() puts the one from sco_conn_add() as
> soon as sco_conn_ready() returns.
> 
> Discovered by XBOW, triaged by Baul Lee <[email protected]>
> Reported privately to the maintainers on 2026-07-10 with root-cause
> analysis, a PoC, a KASAN log and this fix; posting to the list was
> requested as the follow-up.
> 
> Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
> Reported-by: Federico Kirschbaum <[email protected]>
> Reported-by: Baul Lee <[email protected]>
> Cc: [email protected]
> Signed-off-by: Baul Lee <[email protected]>
> ---
>  net/bluetooth/sco.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index c05f79b7aa31..3db6552de06c 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -276,6 +276,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>  	sco_chan_del(sk, err);
>  	release_sock(sk);
>  	sock_put(sk);
> +
> +	/* Drop the association reference, as the !sk branch above does */
> +	sco_conn_put(conn);
>  }
>  
>  static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
> @@ -296,10 +299,16 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  	int err = 0;
>  
>  	sco_conn_lock(conn);
> -	if (conn->sk || sco_pi(sk)->conn)
> +	if (conn->sk || sco_pi(sk)->conn) {
>  		err = -EBUSY;
> -	else
> +	} else {
> +		/* Take the socket reference, which sco_chan_del() drops when
> +		 * the socket detaches.  Without it the socket and the hcon
> +		 * would share the single reference from sco_conn_add().
> +		 */
> +		sco_conn_hold(conn);
>  		__sco_chan_add(conn, sk, parent);
> +	}
>  
>  	sco_conn_unlock(conn);
>  	return err;
> @@ -1452,6 +1461,13 @@ 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);
>  
> +		/* Two references are needed here: the socket one, dropped by
> +		 * sco_chan_del(), and the association one, dropped by
> +		 * sco_conn_del().  Unlike the outgoing path, the reference
> +		 * from sco_conn_add() cannot serve as the latter, because
> +		 * sco_connect_cfm() puts it as soon as this function returns.
> +		 */
> +		sco_conn_hold(conn);
>  		sco_conn_hold(conn);

I think it would be stylistically better to have the holds where they
are assigned to the fields that own the references, and the puts where
the variable is cleared.

>  		hci_conn_hold(conn->hcon);
>  		__sco_chan_add(conn, sk, parent);

-- 
Pauli Virtanen