[PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[Pauli Virtanen]

Rebase to fix conflict in remaining commits from series
https://lore.kernel.org/linux-bluetooth/cover.1788013041.git.pav@iki.fi/

Fixed minor conflict vs. moved mutex_lock + l2cap_chan_lock in diff
context
commit ddaccd985bb0 ("Bluetooth: L2CAP: fix out-of-bounds write in l2cap_ecred_connect")

No other changes than resolving that.

Pauli Virtanen (4):
  Bluetooth: L2CAP: take lock for l2cap_chan_del in
    l2cap_ecred_rsp_defer
  Bluetooth: L2CAP: annotate locking for l2cap_chan_del()
  Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks
  Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn

 include/net/bluetooth/l2cap.h |  65 +++++++++++--------
 net/bluetooth/6lowpan.c       |   2 +
 net/bluetooth/l2cap_core.c    | 113 ++++++++++++++++++++++++++++++++--
 net/bluetooth/l2cap_sock.c    |   2 +
 4 files changed, 150 insertions(+), 32 deletions(-)

-- 
2.55.0

[PATCH 1/4] Bluetooth: L2CAP: take lock for l2cap_chan_del in l2cap_ecred_rsp_defer

[Pauli Virtanen]

l2cap_ecred_rsp_defer() calls l2cap_chan_del without holding chan->lock,
which ends up calling ops->teardown() with wrong lock context.

Fix by taking chan->lock in l2cap_ecred_rsp_defer().  AB-BA deadlocks
between sibling l2cap_chan are avoided here via requiring l2cap_conn::lock
to serialize all nested l2cap_chan locking on same nesting level.

In current code, there is no nested l2cap_chan locking on same nesting
level, so we can add this new requirement.

Also return early from  __l2cap_ecred_conn_rsp_defer() if chan did not
have FLAG_DEFER_SETUP, as then no RSP shall be sent for it, to make sure
SMP channels are excluded.

Also hold chan reference over l2cap_chan_del(), in case chan_l reference
was the last.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h |  4 +++
 net/bluetooth/l2cap_core.c    | 60 +++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 2315a3993c7b..c7e642abe404 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -758,6 +758,10 @@ enum {
  * otherwise considers all channels equal and will e.g. complain about a
  * connection oriented channel triggering SMP procedures or a listening
  * channel creating and locking a child channel.
+ *
+ * Lock nesting of channels at the same nesting level is allowed if the channels
+ * have the same l2cap_chan::conn and l2cap_chan::conn.lock is taken before the
+ * nested locks. l2cap_chan_try_sibling_lock() must be used.
  */
 enum {
 	L2CAP_NESTING_SMP,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9da689f0a50a..805623a48bae 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -848,6 +848,8 @@ static void __l2cap_chan_close(struct l2cap_chan *chan, int reason)
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
+	lockdep_assert_held(&chan->lock);
+
 	switch (chan->state) {
 	case BT_LISTEN:
 		chan->ops->teardown(chan, 0);
@@ -3950,6 +3952,7 @@ static void l2cap_ecred_list_defer(struct l2cap_chan *chan, void *data)
 }
 
 struct l2cap_ecred_rsp_data {
+	struct l2cap_chan *locked_chan;
 	struct {
 		struct l2cap_ecred_conn_rsp_hdr rsp;
 		__le16 scid[L2CAP_ECRED_MAX_CID];
@@ -3957,11 +3960,42 @@ struct l2cap_ecred_rsp_data {
 	int count;
 };
 
+/* Lock @chan if it is not @locked_chan, and has same or lower nesting level.
+ *
+ * They must have the same chan->conn, and conn->lock must be held.
+ *
+ * Caller must ensure @chan has lock nesting level <= that of @locked_chan, as
+ * nested locking of l2cap_chan of different levels is allowed also without
+ * holding conn->lock.
+ *
+ * See l2cap.h for the global l2cap_chan locking rules.
+ */
+static bool l2cap_chan_try_sibling_lock(struct l2cap_chan *chan,
+					struct l2cap_chan *locked_chan)
+	__must_hold(&locked_chan->lock)
+	__must_hold(&locked_chan->conn->lock)
+	__cond_acquires(true, &chan->lock)
+{
+	if (chan == locked_chan)
+		return false;
+
+	if (WARN_ON_ONCE(locked_chan->conn != chan->conn))
+		return false;
+
+	if (WARN_ON_ONCE(atomic_read(&locked_chan->nesting)
+			 < atomic_read(&chan->nesting)))
+		return false;
+
+	mutex_lock_nest_lock(&chan->lock, &locked_chan->conn->lock);
+	return true;
+}
+
 static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
 {
 	struct l2cap_ecred_rsp_data *rsp = data;
 	struct l2cap_ecred_conn_rsp *rsp_flex =
 		container_of(&rsp->pdu.rsp, struct l2cap_ecred_conn_rsp, hdr);
+	bool locked;
 
 	if (chan->mode != L2CAP_MODE_EXT_FLOWCTL)
 		return;
@@ -3973,6 +4007,22 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
 	    !test_and_clear_bit(FLAG_DEFER_SETUP, &chan->flags))
 		return;
 
+	lockdep_assert_held(&rsp->locked_chan->lock);
+	lockdep_assert_held(&rsp->locked_chan->conn->lock);
+
+	l2cap_chan_hold(chan);
+
+	locked = l2cap_chan_try_sibling_lock(chan, rsp->locked_chan);
+
+	/* Cannot occur: PARENT channels do not appear in chan_l, and SMP
+	 * channels never have FLAG_DEFER_SETUP.
+	 */
+	if (context_unsafe(!locked && chan != rsp->locked_chan))
+		goto done;
+
+	lockdep_assert_held(&chan->lock);
+	lockdep_assert_held(&chan->conn->lock);
+
 	/* Reset ident so only one response is sent */
 	chan->ident = 0;
 
@@ -3985,6 +4035,12 @@ static void l2cap_ecred_rsp_defer(struct l2cap_chan *chan, void *data)
 		rsp_flex->dcid[rsp->count++] = cpu_to_le16(chan->scid);
 	else
 		l2cap_chan_del(chan, ECONNRESET);
+
+done:
+	if (locked)
+		l2cap_chan_unlock(chan);
+
+	l2cap_chan_put(chan);
 }
 
 void __l2cap_ecred_conn_rsp_defer(struct l2cap_chan *chan)
@@ -3996,11 +4052,15 @@ void __l2cap_ecred_conn_rsp_defer(struct l2cap_chan *chan)
 
 	if (!id)
 		return;
+	if (!test_bit(FLAG_DEFER_SETUP, &chan->flags))
+		return;
 
 	BT_DBG("chan %p id %d", chan, id);
 
 	memset(&data, 0, sizeof(data));
 
+	data.locked_chan = chan;
+
 	data.pdu.rsp.mtu     = cpu_to_le16(chan->imtu);
 	data.pdu.rsp.mps     = cpu_to_le16(chan->mps);
 	data.pdu.rsp.credits = cpu_to_le16(chan->rx_credits);
-- 
2.55.0

[PATCH 2/4] Bluetooth: L2CAP: annotate locking for l2cap_chan_del()

[Pauli Virtanen]

Add context analysis annotations for chan->lock and chan->conn->lock
involving l2cap_chan_del() usage.

Add necessary annotations and related lockdep_assert_held to callers.

Move struct l2cap_ops definition after struct l2cap_conn, so that the
callbacks can be annotated.

In l2cap_chan_close_unlocked() we consider chan->conn->lock as locked
even if chan->conn == NULL, to avoid needing to define separate
__l2cap_chan_close/del for this NULL case.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h | 56 +++++++++++++++++++----------------
 net/bluetooth/6lowpan.c       |  2 ++
 net/bluetooth/l2cap_core.c    | 49 +++++++++++++++++++++++++-----
 3 files changed, 74 insertions(+), 33 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index c7e642abe404..3e1e2b36d7b6 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -614,31 +614,6 @@ struct l2cap_chan {
 	struct mutex		lock;
 };
 
-struct l2cap_ops {
-	char			*name;
-
-	int			(*new_connection)(struct l2cap_chan *chan,
-						  struct l2cap_chan *new_chan);
-	int			(*recv) (struct l2cap_chan * chan,
-					 struct sk_buff *skb);
-	void			(*teardown) (struct l2cap_chan *chan, int err);
-	void			(*close) (struct l2cap_chan *chan);
-	void			(*state_change) (struct l2cap_chan *chan,
-						 int state, int err);
-	void			(*ready) (struct l2cap_chan *chan);
-	void			(*defer) (struct l2cap_chan *chan);
-	void			(*resume) (struct l2cap_chan *chan);
-	void			(*suspend) (struct l2cap_chan *chan);
-	void			(*set_shutdown) (struct l2cap_chan *chan);
-	long			(*get_sndtimeo) (struct l2cap_chan *chan);
-	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);
-	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
-					       unsigned long hdr_len,
-					       unsigned long len, int nb);
-	int			(*filter) (struct l2cap_chan * chan,
-					   struct sk_buff *skb);
-};
-
 struct l2cap_conn {
 	struct hci_conn		*hcon;
 	struct hci_chan		*hchan;
@@ -674,6 +649,34 @@ struct l2cap_conn {
 	struct list_head	users;
 };
 
+struct l2cap_ops {
+	char			*name;
+
+	int			(*new_connection)(struct l2cap_chan *chan,
+						  struct l2cap_chan *new_chan);
+	int			(*recv) (struct l2cap_chan * chan,
+					 struct sk_buff *skb);
+	void			(*teardown) (struct l2cap_chan *chan, int err)
+					__must_hold(&chan->lock);
+	void			(*close) (struct l2cap_chan *chan);
+	void			(*state_change) (struct l2cap_chan *chan,
+						 int state, int err);
+	void			(*ready) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock)
+					__must_hold(&chan->conn->lock);
+	void			(*defer) (struct l2cap_chan *chan);
+	void			(*resume) (struct l2cap_chan *chan);
+	void			(*suspend) (struct l2cap_chan *chan);
+	void			(*set_shutdown) (struct l2cap_chan *chan);
+	long			(*get_sndtimeo) (struct l2cap_chan *chan);
+	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);
+	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
+					       unsigned long hdr_len,
+					       unsigned long len, int nb);
+	int			(*filter) (struct l2cap_chan * chan,
+					   struct sk_buff *skb);
+};
+
 struct l2cap_user {
 	struct list_head list;
 	int (*probe) (struct l2cap_conn *conn, struct l2cap_user *user);
@@ -983,7 +986,8 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 typedef void (*l2cap_chan_func_t)(struct l2cap_chan *chan, void *data);
 void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
 		     void *data);
-void l2cap_chan_del(struct l2cap_chan *chan, int err);
+void l2cap_chan_del(struct l2cap_chan *chan, int err)
+	__must_hold(&chan->lock) __must_hold(&chan->conn->lock);
 void l2cap_send_conn_req(struct l2cap_chan *chan);
 
 struct l2cap_conn *l2cap_conn_get(struct l2cap_conn *conn);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index ddcdd2aff91f..836add41f5d1 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -722,6 +722,8 @@ static int setup_netdev(struct l2cap_chan *chan, struct lowpan_btle_dev **dev)
 }
 
 static inline void chan_ready_cb(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
+	__must_hold(&chan->conn->lock)
 {
 	struct lowpan_btle_dev *dev;
 	bool new_netdev = false;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 805623a48bae..219d92431be0 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -59,7 +59,8 @@ static void l2cap_tx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
 static void l2cap_retrans_timeout(struct work_struct *work);
 static void l2cap_monitor_timeout(struct work_struct *work);
 static void l2cap_ack_timeout(struct work_struct *work);
-static void __l2cap_chan_close(struct l2cap_chan *chan, int reason);
+static void __l2cap_chan_close(struct l2cap_chan *chan, int reason)
+	__must_hold(&chan->lock) __must_hold(&chan->conn->lock);
 
 static inline u8 bdaddr_type(u8 link_type, u8 bdaddr_type)
 {
@@ -681,6 +682,8 @@ void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 void l2cap_chan_del(struct l2cap_chan *chan, int err)
 {
+	lockdep_assert(!chan->conn || lockdep_is_held(&chan->conn->lock));
+
 	__clear_chan_timer(chan);
 
 	BT_DBG("chan %p, err %d, state %s", chan, err,
@@ -812,12 +815,11 @@ static void l2cap_chan_le_connect_reject(struct l2cap_chan *chan)
 }
 
 static void l2cap_chan_ecred_connect_reject(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
+	__must_hold(&chan->conn->lock)
 {
 	l2cap_state_change(chan, BT_DISCONN);
 
-	lockdep_assert_held(&chan->lock);
-	lockdep_assert_held(&chan->conn->lock);
-
 	__l2cap_ecred_conn_rsp_defer(chan);
 }
 
@@ -848,8 +850,6 @@ static void __l2cap_chan_close(struct l2cap_chan *chan, int reason)
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
-	lockdep_assert_held(&chan->lock);
-
 	switch (chan->state) {
 	case BT_LISTEN:
 		chan->ops->teardown(chan, 0);
@@ -934,7 +934,10 @@ void l2cap_chan_close_unlocked(struct l2cap_chan *chan, int reason)
 	bool have_conn;
 
 	have_conn = l2cap_chan_lock_conn(chan);
-	__l2cap_chan_close(chan, reason);
+
+	/* Context analysis: consider chan->conn->lock held also if conn NULL */
+	context_unsafe(__l2cap_chan_close(chan, reason));
+
 	l2cap_chan_unlock_conn(chan, have_conn);
 }
 EXPORT_SYMBOL(l2cap_chan_close_unlocked);
@@ -1336,6 +1339,8 @@ void l2cap_send_conn_req(struct l2cap_chan *chan)
 }
 
 static void l2cap_chan_ready(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
+	__must_hold(&chan->conn->lock)
 {
 	/* The channel may have already been flagged as connected in
 	 * case of receiving data before the L2CAP info req/rsp
@@ -1471,6 +1476,7 @@ static void l2cap_ecred_connect(struct l2cap_chan *chan)
 }
 
 static void l2cap_le_start(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
 	__must_hold(&chan->conn->lock)
 {
 	struct l2cap_conn *conn = chan->conn;
@@ -1492,6 +1498,7 @@ static void l2cap_le_start(struct l2cap_chan *chan)
 }
 
 static void l2cap_start_connection(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
 	__must_hold(&chan->conn->lock)
 {
 	if (chan->conn->hcon->type == LE_LINK) {
@@ -1542,6 +1549,7 @@ static bool l2cap_check_enc_key_size(struct hci_conn *hcon,
 }
 
 static void l2cap_do_start(struct l2cap_chan *chan)
+	__must_hold(&chan->lock)
 	__must_hold(&chan->conn->lock)
 {
 	struct l2cap_conn *conn = chan->conn;
@@ -1893,6 +1901,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 		l2cap_chan_hold(chan);
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		l2cap_chan_del(chan, err);
 
 		chan->ops->close(chan);
@@ -4218,6 +4228,8 @@ static struct l2cap_chan *l2cap_new_connection(struct l2cap_conn *conn,
 
 	__l2cap_chan_add(conn, chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	if (pchan->ops->new_connection &&
 	    pchan->ops->new_connection(pchan, chan) < 0) {
 		l2cap_chan_del(chan, 0);
@@ -4419,6 +4431,8 @@ static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	switch (result) {
 	case L2CAP_CR_SUCCESS:
 		if (__l2cap_get_chan_by_dcid(conn, dcid)) {
@@ -4520,6 +4534,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2 &&
 	    chan->state != BT_CONNECTED) {
 		cmd_reject_invalid_cid(conn, cmd->ident, chan->scid,
@@ -4634,6 +4650,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
 		l2cap_conf_rfc_get(chan, rsp->data, len);
@@ -4743,6 +4761,8 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	rsp.dcid = cpu_to_le16(chan->scid);
 	rsp.scid = cpu_to_le16(chan->dcid);
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
@@ -4783,6 +4803,8 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	if (chan->state != BT_DISCONN) {
 		l2cap_chan_unlock(chan);
 		l2cap_chan_put(chan);
@@ -4995,6 +5017,8 @@ static int l2cap_le_connect_rsp(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	switch (result) {
 	case L2CAP_CR_LE_SUCCESS:
 		if (__l2cap_get_chan_by_dcid(conn, dcid)) {
@@ -5213,6 +5237,8 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	bacpy(&chan->src, &conn->hcon->src);
 	bacpy(&chan->dst, &conn->hcon->dst);
 	chan->src_type = bdaddr_src_type(conn->hcon);
@@ -5444,6 +5470,8 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
 
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		bacpy(&chan->src, &conn->hcon->src);
 		bacpy(&chan->dst, &conn->hcon->dst);
 		chan->src_type = bdaddr_src_type(conn->hcon);
@@ -5533,6 +5561,8 @@ static inline int l2cap_ecred_conn_rsp(struct l2cap_conn *conn,
 		l2cap_chan_hold(chan);
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		/* Check that there is a dcid for each pending channel */
 		if (cmd_len < sizeof(dcid)) {
 			l2cap_chan_del(chan, ECONNREFUSED);
@@ -5760,6 +5790,8 @@ static inline int l2cap_ecred_reconf_rsp(struct l2cap_conn *conn,
 			continue;
 		l2cap_chan_lock(chan);
 
+		lockdep_assert_held(&chan->conn->lock);
+
 		l2cap_chan_del(chan, ECONNRESET);
 
 		l2cap_chan_unlock(chan);
@@ -5789,6 +5821,7 @@ static inline int l2cap_le_command_rej(struct l2cap_conn *conn,
 		goto done;
 
 	l2cap_chan_lock(chan);
+	lockdep_assert_held(&chan->conn->lock);
 	l2cap_chan_del(chan, ECONNREFUSED);
 	l2cap_chan_unlock(chan);
 	l2cap_chan_put(chan);
@@ -7176,6 +7209,8 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
 
 	l2cap_chan_lock(chan);
 
+	lockdep_assert_held(&chan->conn->lock);
+
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
 	/* If we receive data on a fixed channel before the info req/rsp
-- 
2.55.0

[PATCH 3/4] Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks

[Pauli Virtanen]

Annotate current locking context for l2cap_ops callbacks.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h | 13 +++++++++----
 net/bluetooth/l2cap_core.c    |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 3e1e2b36d7b6..efb9b7f422d1 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -653,21 +653,26 @@ struct l2cap_ops {
 	char			*name;
 
 	int			(*new_connection)(struct l2cap_chan *chan,
-						  struct l2cap_chan *new_chan);
+						  struct l2cap_chan *new_chan)
+					__must_hold(&chan->lock)
+					__must_hold(&new_chan->lock);
 	int			(*recv) (struct l2cap_chan * chan,
 					 struct sk_buff *skb);
 	void			(*teardown) (struct l2cap_chan *chan, int err)
 					__must_hold(&chan->lock);
-	void			(*close) (struct l2cap_chan *chan);
+	void			(*close) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock);
 	void			(*state_change) (struct l2cap_chan *chan,
 						 int state, int err);
 	void			(*ready) (struct l2cap_chan *chan)
 					__must_hold(&chan->lock)
 					__must_hold(&chan->conn->lock);
 	void			(*defer) (struct l2cap_chan *chan);
-	void			(*resume) (struct l2cap_chan *chan);
+	void			(*resume) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock);
 	void			(*suspend) (struct l2cap_chan *chan);
-	void			(*set_shutdown) (struct l2cap_chan *chan);
+	void			(*set_shutdown) (struct l2cap_chan *chan)
+					__must_hold(&chan->lock);
 	long			(*get_sndtimeo) (struct l2cap_chan *chan);
 	struct pid		*(*get_peer_pid) (struct l2cap_chan *chan);
 	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 219d92431be0..b7d5fa6f6a83 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4214,6 +4214,7 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn,
 static struct l2cap_chan *l2cap_new_connection(struct l2cap_conn *conn,
 					       struct l2cap_chan *pchan)
 	__must_hold(&conn->lock)
+	__must_hold(&pchan->lock)
 {
 	struct l2cap_chan *chan;
 
-- 
2.55.0

[PATCH 4/4] Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn

[Pauli Virtanen]

l2cap_chan may be linked to l2cap_conn at most once. This is assumed in
several places, eg l2cap_chan_del cleanup.

There is a TOCTOU race where the invariant is violated:

    [Task 1]                          [Task 2]
    l2cap_chan_connect                l2cap_sock_bind
      l2cap_chan_lock                   lock_sock
      l2cap_state_change                if (sk->sk_state != BT_OPEN)
        chan->state = BT_CONNECT
        l2cap_sock_state_change_cb      chan->state = BT_BOUND
                                        sk->sk_state = BT_BOUND
          lock_sock <------------------ release_sock
          sk->sk_state = BT_CONNECT

l2cap_sock_connect() does not check sk->sk_state, so since chan->state
is now BT_BOUND, subsequent connect() ends up with second
__l2cap_chan_add.

Explicitly document and check the invariant in __l2cap_chan_add with
WARN_ON_ONCE. The only callsite where it could be hit is
l2cap_chan_connect, so add pre-check there to avoid relying on
chan->state. chan->state read/write is not properly guarded currently so
there can be other TOCTOUC problems.

Add l2cap_lock_chan in l2cap_sock_bind() to guard chan->state write.

Fixes: b66774b48dd9 ("Bluetooth: L2CAP: Fix UAF in channel timeout by holding conn ref")
Assisted-by: deepseek-4-flash # finding the race condition
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/l2cap_core.c | 7 ++++++-
 net/bluetooth/l2cap_sock.c | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b7d5fa6f6a83..a22edd2baf5e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -623,6 +623,10 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 	BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
 	       __le16_to_cpu(chan->psm), chan->dcid);
 
+	/* Caller must ensure l2cap_chan is linked to l2cap_conn only once */
+	if (WARN_ON_ONCE(chan->conn || test_bit(FLAG_DEL, &chan->flags)))
+		return;
+
 	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
 
 	chan->conn = l2cap_conn_get(conn);
@@ -7623,7 +7627,8 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 		}
 	}
 
-	if (cid && __l2cap_get_chan_by_dcid(conn, cid)) {
+	if ((cid && __l2cap_get_chan_by_dcid(conn, cid)) || chan->conn ||
+	    test_bit(FLAG_DEL, &chan->flags)) {
 		hci_conn_drop(hcon);
 		err = -EBUSY;
 		goto chan_unlock;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index dee3025f0ec2..278adb05c4c9 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -109,6 +109,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr_unsized *addr, i
 			return -EINVAL;
 	}
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN) {
@@ -174,6 +175,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr_unsized *addr, i
 
 done:
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return err;
 }
 
-- 
2.55.0

Re: [PATCH 0/4] Bluetooth: L2CAP: part 2 of l2cap_conn::chan_l locking fixes

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed,  2 Sep 2026 00:04:32 +0300 you wrote:
> Rebase to fix conflict in remaining commits from series
> https://lore.kernel.org/linux-bluetooth/cover.1788013041.git.pav@iki.fi/
> 
> Fixed minor conflict vs. moved mutex_lock + l2cap_chan_lock in diff
> context
> commit ddaccd985bb0 ("Bluetooth: L2CAP: fix out-of-bounds write in l2cap_ecred_connect")
> 
> [...]

Here is the summary with links:
  - [1/4] Bluetooth: L2CAP: take lock for l2cap_chan_del in l2cap_ecred_rsp_defer
    https://git.kernel.org/bluetooth/bluetooth-next/c/c6d60c24cd7b
  - [2/4] Bluetooth: L2CAP: annotate locking for l2cap_chan_del()
    https://git.kernel.org/bluetooth/bluetooth-next/c/6873eb51dcdd
  - [3/4] Bluetooth: L2CAP: annotate locking for l2cap_ops callbacks
    https://git.kernel.org/bluetooth/bluetooth-next/c/4149ba2a806c
  - [4/4] Bluetooth: L2CAP: refuse __l2cap_chan_add if chan already has conn
    https://git.kernel.org/bluetooth/bluetooth-next/c/6696072ffe07

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html