[PATCH v6] Bluetooth: Fix l2cap_disconnect_req deadlock
From: Ying Hsu <hidden>
Date: 2023-06-01 02:38:56
Also in:
linux-bluetooth, lkml
Subsystem:
bluetooth subsystem, the rest · Maintainers:
Marcel Holtmann, Luiz Augusto von Dentz, Linus Torvalds
L2CAP assumes that the locks conn->chan_lock and chan->lock are
acquired in the order conn->chan_lock, chan->lock to avoid
potential deadlock.
For example, l2sock_shutdown acquires these locks in the order:
mutex_lock(&conn->chan_lock)
l2cap_chan_lock(chan)
However, l2cap_disconnect_req acquires chan->lock in
l2cap_get_chan_by_scid first and then acquires conn->chan_lock
before calling l2cap_chan_del. This means that these locks are
acquired in unexpected order, which leads to potential deadlock:
l2cap_chan_lock(c)
mutex_lock(&conn->chan_lock)
This patch releases chan->lock before acquiring the conn_chan_lock
to avoid the potential deadlock.
Fixes: a2a9339e1c9d ("Bluetooth: L2CAP: Fix use-after-free in l2cap_disconnect_{req,rsp}")
Signed-off-by: Ying Hsu <redacted>
---
This commit has been tested on a Chromebook device.
Changes in v6:
- Fixing format of the fixes tag.
Changes in v5:
- Fixing the merge conflict by removing l2cap_del_chan_by_scid.
Changes in v4:
- Using l2cap_get_chan_by_scid to avoid repeated code.
- Releasing chan->lock before acquiring conn->chan_lock.
Changes in v3:
- Adding the fixes tag.
Changes in v2:
- Adding the prefix "Bluetooth:" to subject line.
net/bluetooth/l2cap_core.c | 37 +++++++++++++++----------------------
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 036bc147f4de..16ac4aac0638 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c@@ -4634,26 +4634,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, return err; } -static struct l2cap_chan *l2cap_del_chan_by_scid(struct l2cap_conn *conn, - u16 cid, int err) -{ - struct l2cap_chan *c; - - mutex_lock(&conn->chan_lock); - c = __l2cap_get_chan_by_scid(conn, cid); - if (c) { - /* Only lock if chan reference is not 0 */ - c = l2cap_chan_hold_unless_zero(c); - if (c) { - l2cap_chan_lock(c); - l2cap_chan_del(c, err); - } - } - mutex_unlock(&conn->chan_lock); - - return c; -} - static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data)
@@ -4671,7 +4651,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid); - chan = l2cap_del_chan_by_scid(conn, dcid, ECONNRESET); + chan = l2cap_get_chan_by_scid(conn, dcid); if (!chan) { cmd_reject_invalid_cid(conn, cmd->ident, dcid, scid); return 0;
@@ -4682,6 +4662,13 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp); chan->ops->set_shutdown(chan); + + l2cap_chan_unlock(chan); + mutex_lock(&conn->chan_lock); + l2cap_chan_lock(chan); + l2cap_chan_del(chan, ECONNRESET); + mutex_unlock(&conn->chan_lock); + chan->ops->close(chan); l2cap_chan_unlock(chan);
@@ -4706,7 +4693,7 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid); - chan = l2cap_del_chan_by_scid(conn, scid, 0); + chan = l2cap_get_chan_by_scid(conn, scid); if (!chan) return 0;
@@ -4716,6 +4703,12 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, return 0; } + l2cap_chan_unlock(chan); + mutex_lock(&conn->chan_lock); + l2cap_chan_lock(chan); + l2cap_chan_del(chan, 0); + mutex_unlock(&conn->chan_lock); + chan->ops->close(chan); l2cap_chan_unlock(chan);
--
2.41.0.rc0.172.g3f132b7071-goog