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

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

[Aldo Ariel Panzardo]

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
 
 

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1133573

---Test result---

Test Summary:
CheckPatch                    PASS      0.75 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 PASS      0.15 seconds
BuildKernel                   PASS      25.23 seconds
CheckAllWarning               PASS      28.01 seconds
CheckSparse                   PASS      26.67 seconds
BuildKernel32                 PASS      24.81 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      461.05 seconds
TestRunner_sco-tester         PASS      31.22 seconds
IncrementalBuild              PASS      24.43 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/486

---
Regards,
Linux Bluetooth