From: Ying Hsu <yinghsu@chromium.org>
To: linux-bluetooth@vger.kernel.org
Cc: chromeos-bluetooth-upstreaming@chromium.org,
Ying Hsu <yinghsu@chromium.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
Marcel Holtmann <marcel@holtmann.org>,
Min Li <lm0963hack@gmail.com>, Paolo Abeni <pabeni@redhat.com>,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: [PATCH v6] Bluetooth: Fix l2cap_disconnect_req deadlock
Date: Thu, 1 Jun 2023 02:38:32 +0000 [thread overview]
Message-ID: <20230601023835.1117866-1-yinghsu@chromium.org> (raw)
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 <yinghsu@chromium.org>
---
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
next reply other threads:[~2023-06-01 2:38 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-01 2:38 Ying Hsu [this message]
2023-06-01 3:03 ` [v6] Bluetooth: Fix l2cap_disconnect_req deadlock bluez.test.bot
2023-06-01 3:57 ` Luiz Augusto von Dentz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230601023835.1117866-1-yinghsu@chromium.org \
--to=yinghsu@chromium.org \
--cc=chromeos-bluetooth-upstreaming@chromium.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=johan.hedberg@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lm0963hack@gmail.com \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).