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

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;
-	}
 
 	/* Kill socket */
 	lock_sock(sk);
-- 
2.43.0