[PATCH] Bluetooth: fix UAF in bt_accept_dequeue()

[PATCH] Bluetooth: fix UAF in bt_accept_dequeue()

[Yousef Alhouseen]

bt_accept_get() takes a temporary reference before dropping the accept
queue lock. bt_accept_dequeue() currently drops that reference before
bt_accept_unlink(), leaving only the queue reference.

bt_accept_unlink() drops the queue reference. The subsequent
sock_hold() therefore accesses freed memory if it was the final
reference, as observed by KASAN during listening L2CAP socket cleanup.

Retain the temporary queue-walk reference through unlink and hand it to
the caller on success. Drop it explicitly on the closed and
not-yet-connected paths.

Fixes: ab1513597c6c ("Bluetooth: fix UAF in l2cap_sock_cleanup_listen() vs l2cap_conn_del()")
Reported-by: syzbot+674ff7e4d7fdfd572afc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=674ff7e4d7fdfd572afc
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 net/bluetooth/af_bluetooth.c | 17 +++--------------
 net/bluetooth/l2cap_sock.c   |  4 ++--
 2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index bcbc11c9cb15..a2290ffdc2c1 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -305,7 +305,7 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 
 restart:
 	for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
-		/* Prevent early freeing of sk due to unlink and sock_kill */
+		/* The reference from bt_accept_get() keeps sk alive. */
 		lock_sock(sk);
 
 		/* Check sk has not already been unlinked via
@@ -321,13 +321,11 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 
 		next = bt_accept_get(parent, sk);
 
-		/* sk is safely in the parent list so reduce reference count */
-		sock_put(sk);
-
 		/* FIXME: Is this check still needed */
 		if (sk->sk_state == BT_CLOSED) {
 			bt_accept_unlink(sk);
 			release_sock(sk);
+			sock_put(sk);
 			continue;
 		}
 
@@ -337,16 +335,6 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 			if (newsock)
 				sock_graft(sk, newsock);
 
-			/* Hand the caller a reference taken while sk is
-			 * still locked.  bt_accept_unlink() just dropped
-			 * the accept-queue reference; without this hold a
-			 * concurrent teardown (e.g. l2cap_conn_del() ->
-			 * l2cap_sock_kill()) could free sk between
-			 * release_sock() and the caller using it.  Every
-			 * caller drops this with sock_put() when done.
-			 */
-			sock_hold(sk);
-
 			release_sock(sk);
 			if (next)
 				sock_put(next);
@@ -354,6 +342,7 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 		}
 
 		release_sock(sk);
+		sock_put(sk);
 	}
 
 	return NULL;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 4853f1b33449..de56ca691afa 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1492,8 +1492,8 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
 
 	/* Close not yet accepted channels.
 	 *
-	 * bt_accept_dequeue() now returns sk with an extra reference held
-	 * (taken while sk was still locked) so a concurrent l2cap_conn_del()
+	 * bt_accept_dequeue() returns sk with its temporary queue-walk
+	 * reference held, so a concurrent l2cap_conn_del()
 	 * -> l2cap_sock_kill() cannot free sk under us.
 	 *
 	 * cleanup_listen() runs under the parent sk lock, so unlike
-- 
2.54.0

RE: Bluetooth: fix UAF in bt_accept_dequeue()

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2389 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=1117553

---Test result---

Test Summary:
CheckPatch                    PASS      1.36 seconds
VerifyFixes                   PASS      0.23 seconds
VerifySignedoff               PASS      0.22 seconds
GitLint                       PASS      0.63 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      27.92 seconds
CheckAllWarning               PASS      29.42 seconds
CheckSparse                   PASS      28.30 seconds
BuildKernel32                 PASS      26.06 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      495.64 seconds
TestRunner_l2cap-tester       PASS      57.70 seconds
TestRunner_iso-tester         PASS      83.51 seconds
TestRunner_bnep-tester        PASS      19.08 seconds
TestRunner_mgmt-tester        FAIL      206.56 seconds
TestRunner_rfcomm-tester      PASS      25.63 seconds
TestRunner_sco-tester         PASS      33.10 seconds
TestRunner_ioctl-tester       PASS      25.91 seconds
TestRunner_mesh-tester        FAIL      27.07 seconds
TestRunner_smp-tester         PASS      23.20 seconds
TestRunner_userchan-tester    PASS      22.72 seconds
TestRunner_6lowpan-tester     PASS      22.35 seconds
IncrementalBuild              PASS      29.80 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.237 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.629 seconds
Mesh - Send cancel - 2                               Timed out    1.985 seconds


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

---
Regards,
Linux Bluetooth