[PATCH] Bluetooth: L2CAP: don't reserve an ident for L2CAP_LE_CREDITS

[PATCH] Bluetooth: L2CAP: don't reserve an ident for L2CAP_LE_CREDITS

[Nguyen Ngoc Thang]

l2cap_chan_le_send_credits() allocates an ident from conn->tx_ida, sends
L2CAP_LE_CREDITS and then frees the ident unconditionally, since the
command has no response.

But the ident is visible in tx_ida while the command is in flight, and
the remote may answer it with L2CAP_COMMAND_REJ anyway. The RX path then
releases it in l2cap_put_ident() under conn->lock, while the credits path
runs from recvmsg() with only the socket lock held. Whichever ida_free()
comes second hits:

  ida_free called for id=3 which is not allocated.
  WARNING: lib/idr.c:594 at ida_free+0x24c/0x2d4 lib/idr.c:594
  Call trace:
   ida_free+0x24c/0x2d4 lib/idr.c:594
   l2cap_chan_le_send_credits+0x17c/0x29c net/bluetooth/l2cap_core.c:7113
   l2cap_chan_rx_avail+0xb4/0x134 net/bluetooth/l2cap_core.c:7126
   l2cap_publish_rx_avail+0x110/0x168 net/bluetooth/l2cap_sock.c
   l2cap_sock_recvmsg+0x280/0x7b8 net/bluetooth/l2cap_sock.c:1317

Worse, if the ident is reallocated for a real request in between, the
credits path frees that request's ident instead.

tx_ida only exists to track requests awaiting a response, so a command
without one has no business in it. Give L2CAP_LE_CREDITS a plain cyclic
ident instead, which never needs releasing. The ident is still never 0
and never exhausts the range, which is what commit 6e1930ece855
("Bluetooth: L2CAP: fix tx ident leak for commands without a response")
set out to fix.

Fixes: 6e1930ece855 ("Bluetooth: L2CAP: fix tx ident leak for commands without a response")
Reported-by: syzbot+9dc8780713b11155dd74@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9dc8780713b11155dd74
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
Tested in QEMU (x86_64, KASAN + lockdep) with the syzbot C reproducer,
15 runs per kernel:
  base (origin/master 23c62304dde2): 31x "ida_free called for id=N ..."
  patched:                           0 warnings, 1200 LE credits sent,
                                     none with ident 0 (ident wraps fine)

 net/bluetooth/l2cap_core.c | 44 +++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7dcb0586a10b..0b861ced5703 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1028,21 +1028,36 @@ int l2cap_chan_check_security(struct l2cap_chan *chan, bool initiator)
 				 initiator);
 }
 
-static int l2cap_get_ident(struct l2cap_conn *conn)
+static u8 l2cap_ident_max(struct l2cap_conn *conn)
 {
-	u8 max;
-	int ident;
-
 	/* LE link does not support tools like l2ping so use the full range */
 	if (conn->hcon->type == LE_LINK)
-		max = 255;
+		return 255;
+
 	/* Get next available identificator.
 	 *    1 - 128 are used by kernel.
 	 *  129 - 199 are reserved.
 	 *  200 - 254 are used by utilities like l2ping, etc.
 	 */
-	else
-		max = 128;
+	return 128;
+}
+
+/* Ident for a command that has no response: it is not reserved in tx_ida,
+ * so nothing has to release it and it can't be released twice.
+ */
+static u8 l2cap_next_ident(struct l2cap_conn *conn)
+{
+	u8 ident = READ_ONCE(conn->tx_ident) % l2cap_ident_max(conn) + 1;
+
+	WRITE_ONCE(conn->tx_ident, ident);
+
+	return ident;
+}
+
+static int l2cap_get_ident(struct l2cap_conn *conn)
+{
+	u8 max = l2cap_ident_max(conn);
+	int ident;
 
 	/* Allocate ident using min as last used + 1 (cyclic) */
 	ident = ida_alloc_range(&conn->tx_ida, READ_ONCE(conn->tx_ident) + 1,
@@ -7081,7 +7096,6 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
 	struct l2cap_conn *conn = chan->conn;
 	struct l2cap_le_credits pkt;
 	u16 return_credits = l2cap_le_rx_credits(chan);
-	int ident;
 
 	if (chan->mode != L2CAP_MODE_LE_FLOWCTL &&
 	    chan->mode != L2CAP_MODE_EXT_FLOWCTL)
@@ -7099,18 +7113,8 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
 	pkt.cid     = cpu_to_le16(chan->scid);
 	pkt.credits = cpu_to_le16(return_credits);
 
-	ident = l2cap_get_ident(conn);
-
-	l2cap_send_cmd(conn, ident, L2CAP_LE_CREDITS, sizeof(pkt), &pkt);
-
-	/* L2CAP_LE_CREDITS has no response so the ident is never released by
-	 * l2cap_put_ident() - release it right away, otherwise the tx_ida
-	 * range is exhausted after 254 packets and from then on credits are
-	 * sent with the invalid ident 0, which some remote stacks ignore,
-	 * stalling the channel.
-	 */
-	if (ident > 0)
-		ida_free(&conn->tx_ida, ident);
+	l2cap_send_cmd(conn, l2cap_next_ident(conn), L2CAP_LE_CREDITS,
+		       sizeof(pkt), &pkt);
 }
 
 void l2cap_chan_rx_avail(struct l2cap_chan *chan, ssize_t rx_avail)
-- 
2.43.0

RE: Bluetooth: L2CAP: don't reserve an ident for L2CAP_LE_CREDITS

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1074 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/series/1211811/

---Test result---

Test Summary:
CheckPatch                    PASS      0.60 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       PASS      0.29 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      20.46 seconds
CheckAllWarning               PASS      23.38 seconds
CheckSparse                   PASS      24.58 seconds
BuildKernel32                 PASS      20.47 seconds
CheckKernelLLVM               PASS      21.01 seconds
TestRunnerSetup               PASS      484.24 seconds
TestRunner_l2cap-tester       PASS      13.81 seconds
IncrementalBuild              PASS      19.03 seconds



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

---
Regards,
Linux Bluetooth