[PATCH 1/2] Bluetooth: L2CAP: use proto_lock for l2cap_data to fix l2cap_disconn_ind

[PATCH 1/2] Bluetooth: L2CAP: use proto_lock for l2cap_data to fix l2cap_disconn_ind

[Pauli Virtanen]

hci_conn::l2cap_data is accessed without locks in l2cap_disconn_ind via
hci_conn_timeout (disc_work) -> hci_proto_disconn_ind ->
l2cap_disconn_ind.  This is UAF if the l2cap_conn is deleted
concurrently.

disc_work is disabled sync in hci_conn_del(), so we cannot take
hci_dev_lock in disc_work.

Fix by using proto_lock to guard l2cap_data, in addition to hdev->lock
which is held in other access paths.

Fixes: ab4eedb790ca ("Bluetooth: L2CAP: Fix corrupted list in hci_chan_del")
Reported-by: syzbot+9c40ad7c6ed7165e46e8@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9c40ad7c6ed7165e46e8
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/l2cap_core.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 1156aba4e83c..30d7120d3a15 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1833,7 +1833,10 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 	hci_chan_del(conn->hchan);
 	conn->hchan = NULL;
 
+	spin_lock(&hcon->proto_lock);
 	hcon->l2cap_data = NULL;
+	spin_unlock(&hcon->proto_lock);
+
 	mutex_unlock(&conn->lock);
 	l2cap_conn_put(conn);
 }
@@ -7168,8 +7171,6 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 	}
 
 	kref_init(&conn->ref);
-	hcon->l2cap_data = conn;
-	conn->hcon = hci_conn_get(hcon);
 	conn->hchan = hchan;
 
 	BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
@@ -7198,6 +7199,11 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 
 	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
 
+	spin_lock(&hcon->proto_lock);
+	conn->hcon = hci_conn_get(hcon);
+	hcon->l2cap_data = conn;
+	spin_unlock(&hcon->proto_lock);
+
 	return conn;
 }
 
@@ -7582,13 +7588,18 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 
 int l2cap_disconn_ind(struct hci_conn *hcon)
 {
-	struct l2cap_conn *conn = hcon->l2cap_data;
+	struct l2cap_conn *conn;
+	int ret = HCI_ERROR_REMOTE_USER_TERM;
 
 	BT_DBG("hcon %p", hcon);
 
-	if (!conn)
-		return HCI_ERROR_REMOTE_USER_TERM;
-	return conn->disc_reason;
+	spin_lock(&hcon->proto_lock);
+	conn = hcon->l2cap_data;
+	if (conn)
+		ret = conn->disc_reason;
+	spin_unlock(&hcon->proto_lock);
+
+	return ret;
 }
 
 static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason)
-- 
2.55.0

[PATCH 2/2] Bluetooth: add annotations for l2cap_data locking context

[Pauli Virtanen]

Add context analysis annotations for hci_conn::l2cap_data locking.

Also add necessary lockdep_assert_held() and __must_hold annotations
to prove the access is safe.

The access in smp_conn_security() is supposed to be guarded by the
caller holding lock that blocks concurrent l2cap_conn_del() eg.
hdev->lock, conn->lock or chan->lock. Mark unsafe as can't be
automatically checked now.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/hci_core.h | 2 +-
 net/bluetooth/6lowpan.c          | 2 ++
 net/bluetooth/l2cap_core.c       | 9 +++++++++
 net/bluetooth/mgmt.c             | 2 ++
 net/bluetooth/smp.c              | 7 ++++++-
 net/bluetooth/smp.h              | 6 ++++--
 6 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 01b938c4b24a..c299daac7fbe 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -775,7 +775,7 @@ struct hci_conn {
 	struct hci_dev	*hdev;
 
 	spinlock_t	proto_lock; /* lock guarding protocol data */
-	void		*l2cap_data;
+	void		*l2cap_data __guarded_by(&proto_lock, &hdev->lock);
 	void		*sco_data;
 	void		*iso_data __guarded_by(&proto_lock);
 
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index d504a363a30f..30f4afa18bc8 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -1007,6 +1007,8 @@ static int get_l2cap_conn(char *buf, bdaddr_t *addr, u8 *addr_type,
 		return -ENOENT;
 	}
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	*conn = l2cap_conn_hold_unless_zero(hcon->l2cap_data);
 
 	BT_DBG("conn %p dst %pMR type %u", *conn, &hcon->dst, hcon->dst_type);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 30d7120d3a15..ee459dd411f5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1791,6 +1791,7 @@ static void l2cap_unregister_all_users(struct l2cap_conn *conn)
 }
 
 static void l2cap_conn_del(struct hci_conn *hcon, int err)
+	__must_hold(&hcon->hdev->lock)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct l2cap_chan *chan, *l;
@@ -7153,6 +7154,7 @@ static void process_pending_rx(struct work_struct *work)
 }
 
 static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
+	__must_hold(&hcon->hdev->lock)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct hci_chan *hchan;
@@ -7358,6 +7360,8 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 		goto done;
 	}
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	conn = l2cap_conn_add(hcon);
 	if (!conn) {
 		hci_conn_drop(hcon);
@@ -7528,6 +7532,7 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
 }
 
 static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
+	__must_hold(&hcon->hdev->lock)
 {
 	struct hci_dev *hdev = hcon->hdev;
 	struct l2cap_conn *conn;
@@ -7603,6 +7608,7 @@ int l2cap_disconn_ind(struct hci_conn *hcon)
 }
 
 static void l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason)
+	__must_hold(&hcon->hdev->lock)
 {
 	if (hcon->type != ACL_LINK && hcon->type != LE_LINK)
 		return;
@@ -7630,6 +7636,7 @@ static inline void l2cap_check_encryption(struct l2cap_chan *chan, u8 encrypt)
 }
 
 static void l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
+	__must_hold(&hcon->hdev->lock)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct l2cap_chan *chan;
@@ -7814,6 +7821,8 @@ int l2cap_recv_acldata(struct hci_dev *hdev, u16 handle,
 		return -ENOENT;
 	}
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	hci_conn_enter_active_mode(hcon, BT_POWER_FORCE_ACTIVE_OFF);
 
 	conn = hcon->l2cap_data;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 09edd72acc22..0d6b41fe0b34 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3876,6 +3876,8 @@ static int user_pairing_resp(struct sock *sk, struct hci_dev *hdev,
 	}
 
 	if (addr->type == BDADDR_LE_PUBLIC || addr->type == BDADDR_LE_RANDOM) {
+		lockdep_assert_held(&conn->hdev->lock);
+
 		err = smp_user_confirm_reply(conn, mgmt_op, passkey);
 		if (!err)
 			err = mgmt_cmd_complete(sk, hdev->id, mgmt_op,
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index c4470958b0d5..f23b695c487b 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -2327,12 +2327,15 @@ static void smp_send_security_req(struct smp_chan *smp, __u8 auth)
 
 int smp_conn_security(struct hci_conn *hcon, __u8 sec_level)
 {
-	struct l2cap_conn *conn = hcon->l2cap_data;
+	struct l2cap_conn *conn;
 	struct l2cap_chan *chan;
 	struct smp_chan *smp;
 	__u8 authreq;
 	int ret;
 
+	/* Caller shall ensure there can be no race with l2cap_conn_del() */
+	conn = context_unsafe(hcon->l2cap_data);
+
 	bt_dev_dbg(hcon->hdev, "conn %p hcon %p level 0x%2.2x", conn, hcon,
 		   sec_level);
 
@@ -2421,6 +2424,8 @@ int smp_cancel_and_remove_pairing(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	if (!hcon)
 		goto done;
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	conn = hcon->l2cap_data;
 	if (!conn)
 		goto done;
diff --git a/net/bluetooth/smp.h b/net/bluetooth/smp.h
index eac27bd541bb..c86c46389007 100644
--- a/net/bluetooth/smp.h
+++ b/net/bluetooth/smp.h
@@ -180,11 +180,13 @@ enum smp_key_pref {
 
 /* SMP Commands */
 int smp_cancel_and_remove_pairing(struct hci_dev *hdev, bdaddr_t *bdaddr,
-				  u8 addr_type);
+				  u8 addr_type)
+	__must_hold(&hdev->lock);
 bool smp_sufficient_security(struct hci_conn *hcon, u8 sec_level,
 			     enum smp_key_pref key_pref);
 int smp_conn_security(struct hci_conn *hcon, __u8 sec_level);
-int smp_user_confirm_reply(struct hci_conn *conn, u16 mgmt_op, __le32 passkey);
+int smp_user_confirm_reply(struct hci_conn *conn, u16 mgmt_op, __le32 passkey)
+	__must_hold(&conn->hdev->lock);
 
 bool smp_irk_matches(struct hci_dev *hdev, const u8 irk[16],
 		     const bdaddr_t *bdaddr);
-- 
2.55.0

RE: [1/2] Bluetooth: L2CAP: use proto_lock for l2cap_data to fix l2cap_disconn_ind

[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=1138701

---Test result---

Test Summary:
CheckPatch                    PASS      2.03 seconds
VerifyFixes                   PASS      0.09 seconds
VerifySignedoff               PASS      0.09 seconds
GitLint                       PASS      0.45 seconds
SubjectPrefix                 PASS      0.16 seconds
BuildKernel                   PASS      24.87 seconds
CheckAllWarning               PASS      27.09 seconds
CheckSparse                   PASS      25.93 seconds
BuildKernel32                 PASS      23.94 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      453.23 seconds
TestRunner_l2cap-tester       PASS      63.44 seconds
TestRunner_iso-tester         PASS      78.00 seconds
TestRunner_bnep-tester        PASS      18.54 seconds
TestRunner_mgmt-tester        FAIL      224.32 seconds
TestRunner_rfcomm-tester      PASS      25.16 seconds
TestRunner_sco-tester         PASS      31.61 seconds
TestRunner_ioctl-tester       PASS      26.40 seconds
TestRunner_mesh-tester        FAIL      25.86 seconds
TestRunner_smp-tester         PASS      23.10 seconds
TestRunner_userchan-tester    PASS      24.23 seconds
TestRunner_6lowpan-tester     PASS      22.52 seconds
IncrementalBuild              PASS      39.03 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: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.258 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.415 seconds
Mesh - Send cancel - 2                               Timed out    1.990 seconds


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

---
Regards,
Linux Bluetooth