[PATCH] Bluetooth: L2CAP: take chan_l lock in ecred defer recvmsg path to fix list corruption

[PATCH] Bluetooth: L2CAP: take chan_l lock in ecred defer recvmsg path to fix list corruption

[Doruk Tan Ozturk]

When a deferred L2CAP_MODE_EXT_FLOWCTL connection is accepted,
l2cap_sock_recvmsg() (BT_CONNECT2 + BT_SK_DEFER_SETUP branch) calls
__l2cap_ecred_conn_rsp_defer() while holding only lock_sock(sk).  That
function walks conn->chan_l via __l2cap_chan_list_id() and, on the
authorization/refuse path, removes channels with l2cap_chan_del() ->
list_del(&chan->list) -- all without conn->lock.

conn->chan_l is serialised by conn->lock and is concurrently mutated by
the RX worker, which processes inbound signalling (e.g. an
L2CAP_DISCONN_REQ -> l2cap_chan_del()) under conn->lock.  A lockless
traversal/removal racing the RX worker's list_del() corrupts the list:

  list_del corruption, ...->next is LIST_POISON1 (dead000000000100)
  WARNING lib/list_debug.c:56 __list_del_entry_valid_or_report
   l2cap_chan_del+0xdb/0x1140
   __l2cap_ecred_conn_rsp_defer+0x61d/0x700
   l2cap_sock_recvmsg+0x758/0x8b0
  Oops: general protection fault
  KASAN: maybe wild-memory-access in range [dead000000000100-...]
   __l2cap_ecred_conn_rsp_defer+0x3c6/0x700
   l2cap_sock_recvmsg+0x758/0x8b0

This is the recvmsg-path sibling of commit 41c2713b204e ("Bluetooth:
L2CAP: Fix possible crash on l2cap_ecred_conn_rsp"), which addressed the
same conn->chan_l manipulation on the signalling (l2cap_ecred_conn_rsp)
side; the deferred-accept path was left unguarded.

Take conn->lock around __l2cap_ecred_conn_rsp_defer().  The established
lock order is conn->lock -> chan->lock -> sk_lock (the RX worker reaches
the socket via l2cap_chan_del() -> l2cap_sock_teardown_cb() ->
lock_sock_nested()), so the socket lock is dropped before conn->lock is
taken, mirroring l2cap_sock_shutdown() and l2cap_sock_cleanup_listen().
The conn is pinned with l2cap_conn_hold_unless_zero() across the
unlocked window.  Only the EXT_FLOWCTL branch needs this; the LE and
BR/EDR defer paths respond for a single channel and do not walk
conn->chan_l.

Found by 0sec (https://0sec.ai).
Fixes: 15f02b910562 ("Bluetooth: L2CAP: Add initial code for Enhanced Credit Based Mode")
Cc: stable@vger.kernel.org
Assisted-by: 0sec
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 net/bluetooth/l2cap_sock.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 735167f73f31..796b9a4eee24 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1227,9 +1227,39 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	if (sk->sk_state == BT_CONNECT2 && test_bit(BT_SK_DEFER_SETUP,
 						    &bt_sk(sk)->flags)) {
 		if (pi->chan->mode == L2CAP_MODE_EXT_FLOWCTL) {
+			struct l2cap_chan *chan = pi->chan;
+			struct l2cap_conn *conn;
+
 			sk->sk_state = BT_CONNECTED;
-			pi->chan->state = BT_CONNECTED;
-			__l2cap_ecred_conn_rsp_defer(pi->chan);
+			chan->state = BT_CONNECTED;
+
+			/* __l2cap_ecred_conn_rsp_defer() walks and mutates
+			 * conn->chan_l (via __l2cap_chan_list_id() and
+			 * l2cap_chan_del()), which is serialised by conn->lock
+			 * and is concurrently modified by the RX worker.  The
+			 * established lock order is
+			 * conn->lock -> chan->lock -> sk_lock, so the socket
+			 * lock must be dropped before taking conn->lock to
+			 * avoid inverting it (lockdep deadlock).  Pin the conn
+			 * across the unlocked window.
+			 */
+			conn = l2cap_conn_hold_unless_zero(chan->conn);
+			release_sock(sk);
+			if (conn) {
+				mutex_lock(&conn->lock);
+				/* The RX worker may have torn the channel down
+				 * (FLAG_DEL, removed from conn->chan_l) while the
+				 * socket lock was dropped; skip the response in
+				 * that case. conn->lock below serialises the
+				 * chan_l walk against the RX worker's
+				 * l2cap_chan_del().
+				 */
+				if (!test_bit(FLAG_DEL, &chan->flags))
+					__l2cap_ecred_conn_rsp_defer(chan);
+				mutex_unlock(&conn->lock);
+				l2cap_conn_put(conn);
+			}
+			lock_sock(sk);
 		} else if (bdaddr_type_is_le(pi->chan->src_type)) {
 			sk->sk_state = BT_CONNECTED;
 			pi->chan->state = BT_CONNECTED;
-- 
2.43.0

RE: Bluetooth: L2CAP: take chan_l lock in ecred defer recvmsg path to fix list corruption

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      0.59 seconds
VerifyFixes                   PASS      0.08 seconds
VerifySignedoff               PASS      0.07 seconds
GitLint                       FAIL      0.21 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      26.65 seconds
CheckAllWarning               PASS      29.65 seconds
CheckSparse                   PASS      27.95 seconds
BuildKernel32                 PASS      26.37 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      505.78 seconds
TestRunner_l2cap-tester       PASS      63.67 seconds
IncrementalBuild              PASS      26.67 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: L2CAP: take chan_l lock in ecred defer recvmsg path to fix list corruption
WARNING: Assisted-by expects 'AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]' format
#139: 
Assisted-by: 0sec

total: 0 errors, 1 warnings, 0 checks, 41 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14686349.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: L2CAP: take chan_l lock in ecred defer recvmsg path to fix list corruption

1: T1 Title exceeds max length (85>80): "Bluetooth: L2CAP: take chan_l lock in ecred defer recvmsg path to fix list corruption"
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


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

---
Regards,
Linux Bluetooth