[PATCH v2] Bluetooth: SCO: give the socket its own sco_conn reference

[Aldo Ariel Panzardo <qwe.aldo@gmail.com>]

sco_conn_del() drops a reference it does not own. It takes one transient
reference via sco_conn_hold_unless_zero() and releases it with the
sco_conn_put() that follows sco_sock_hold(); the additional put in the
!sk branch releases a second one:

    conn = sco_conn_hold_unless_zero(conn);
    ...
    sk = sco_sock_hold(conn);
    sco_conn_unlock(conn);
    sco_conn_put(conn);

    if (!sk) {
            sco_conn_put(conn);
            return;
    }

When close() races the controller's Disconnection Complete, sco_chan_del()
clears conn->sk and drops the socket's reference while sco_conn_del() is
running. sco_conn_del() then sees 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
    Freed by task 413:
     sco_conn_del.isra.0+0x3f/0xf0
     hci_disconn_complete_evt+0x1ee/0x3e0
    refcount_t: underflow; use-after-free.

Simply deleting the extra put is not enough, because the reference it
releases is not always accounted for elsewhere. __sco_chan_add() stores
the connection in the socket without taking a reference:

    sco_pi(sk)->conn = conn;

so the socket inherits whatever reference its caller happened to hold.
That works out for sco_conn_ready(), which takes an explicit
sco_conn_hold() beforehand and whose caller puts its own reference, and
for the success path of sco_connect(), where the reference returned by
sco_conn_add() is silently handed over and later released by
sco_sock_destruct(). It does not work out for the two error paths of
sco_connect(): if the socket state changed while the lock was dropped, or
if sco_chan_add() returns -EBUSY, the reference from sco_conn_add() is
never released and the connection is leaked. The extra put in
sco_conn_del() is what eventually reclaims those orphans, which is why
removing it in isolation trades a use-after-free for a leak.

Make the ownership explicit instead. __sco_chan_add() now takes the
socket's reference itself, sco_connect() releases the one it got from
sco_conn_add() on every path, and the now redundant hold in
sco_conn_ready() is dropped. With the socket holding a counted reference,
a connection can no longer reach zero while conn->sk is set, so
sco_conn_free() no longer has to clear sco_pi(conn->sk)->conn. Every
reference then has exactly one owner: the one sco_conn_add() returns
belongs to its caller, the socket's is taken and released with the
channel, and sco_conn_del() and sco_sock_timeout() only ever hold
transient ones.

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>
---
v2:
 - Do not just delete the extra put: make the socket own its reference,
   balance sco_connect()'s error paths and drop the redundant hold in
   sco_conn_ready(), per Pauli Virtanen's review.
 - Drop the now unreachable sco_pi(conn->sk)->conn clearing in
   sco_conn_free().
 - Indent the quoted code with spaces so gitlint stops complaining.

On hci_conn_drop() vs hci_connect_sco(), which was also asked about: the
reference hci_connect_sco() returns is released by hci_conn_drop() on
each error path of sco_connect(), and on the success path it is handed to
the connection and released by sco_conn_free(). That side looks balanced.
There is a separate asymmetry that this patch does not touch: when
sco_conn_add() returns a connection that already existed for the hcon,
hci_connect_sco() has taken a fresh hci_conn reference but sco_conn_free()
only ever issues one hci_conn_drop(). That looks like a pre-existing
hci_conn leak rather than an sco_conn one; I did not want to fold it into
this fix.

Testing: the original defect reproduced 45 times across 2 independent
runs on unmodified v7.2-rc1-240-g71dfdfb0209b with KASAN, driven through
/dev/vhci by racing close() of an SCO socket against an injected
Disconnection Complete; both KASAN and the refcount_t underflow fired
every time. The BlueZ CI ran sco-tester against v1 with no regression.

 net/bluetooth/sco.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index fcc597be5bbd..21f829575803 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -81,9 +81,6 @@ static void sco_conn_free(struct kref *r
 
 	BT_DBG("conn %p", conn);
 
-	if (conn->sk)
-		sco_pi(conn->sk)->conn = NULL;
-
 	if (conn->hcon) {
 		conn->hcon->sco_data = NULL;
 		hci_conn_drop(conn->hcon);
@@ -265,10 +262,8 @@ static void sco_conn_del(struct hci_conn
 	sco_conn_unlock(conn);
 	sco_conn_put(conn);
 
-	if (!sk) {
-		sco_conn_put(conn);
+	if (!sk)
 		return;
-	}
 
 	/* Kill socket */
 	lock_sock(sk);
@@ -283,7 +278,7 @@ static void __sco_chan_add(struct sco_co
 {
 	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 +361,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);
@@ -1439,7 +1436,6 @@ static void sco_conn_ready(struct sco_co
 		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);
 
-- 
2.43.0