[PATCH 5.15.y 0/2] Bluetooth: L2CAP: fix connectionless receive path

[PATCH 5.15.y 0/2] Bluetooth: L2CAP: fix connectionless receive path

[Karl Mehltretter]

5.15.y has two problems on the L2CAP connectionless receive path. The first
comes from a backport that landed without its prerequisite, the second from
a fix that never landed at all. Upstream contains both fixes.

1) c531e63871c0 ("Bluetooth: l2cap: always unlock channel in
   l2cap_conless_channel()") was backported as 5caf0ffaf915 without
   f1a8f402f13f ("Bluetooth: L2CAP: Fix deadlock"), which is what adds the
   matching lock. l2cap_conless_channel() therefore calls
   l2cap_chan_unlock() on a mutex it never acquired, and
   l2cap_sock_recv_cb() runs with no chan->lock at all on that path.
   Patch 1 applies the l2cap_core.c hunks of f1a8f402f13f, which supply
   the lock.

2) 89e856e124f9 ("bluetooth/l2cap: sync sock recv cb and release") never
   landed here, so l2cap_sock_recv_cb() has no NULL check on chan->data,
   while l2cap_sock_destruct() still sets it to NULL. Closing a receiving
   socket while L2CAP data is inbound gives lock_sock(NULL). Patch 2
   restores that guard in the form the commit has after f1a8f402f13f, that
   is without the channel locking that caused the recursive chan->lock
   deadlock.

89e856e124f9 is the fix for CVE-2024-41062. It went to 6.1.101, 6.6.42,
6.9.11 and 6.10 in July 2024 and to 5.10.270 this month. 5.15.y has never
carried it.

On why patch 1 is only part of f1a8f402f13f: as posted, that patch touched
only net/bluetooth/l2cap_core.c and net/bluetooth/l2cap_sock.c.

  https://lore.kernel.org/linux-bluetooth/20240624134637.3790278-1-luiz.dentz@gmail.com/

The hci_core.c, hci_sync.c and hci_sync.h changes in the merged commit come
from a separate patch that was squashed into it while the pull request was
prepared. The author noted this when the AUTOSEL backport came up in 2024
and said that for stable it would be better to unmerge them:

  https://lore.kernel.org/linux-bluetooth/CABBYNZLzf2x6cScmjGv2Rxk-i3F9=QKVWosrSEBgmHBdHqOWtg@mail.gmail.com/

Of the two posted files, only the l2cap_core.c side applies here, because
l2cap_sock_recv_cb() in 5.15.y has no channel locking to remove.

Tested in QEMU with two virtual BR/EDR controllers, PROVE_LOCKING,
DEBUG_MUTEXES and KASAN. Three runs of each variant:

  v5.15.221 as released   bad unlock balance, and a fatal NULL dereference
                          in l2cap_sock_recv_cb() from hci_rx_work, 3/3
  + patch 1               unbalanced unlock gone, NULL deref remains 3/3
  + patch 1 and 2         clean 3/3 over 300 socket close cycles

  BUG: kernel NULL pointer dereference, address: 000000000000008c
  Workqueue: hci0 hci_rx_work
    lock_sock_nested
    l2cap_sock_recv_cb+0x36/0xf0
    l2cap_recv_frame

BlueZ l2cap-tester, rfcomm-tester, smp-tester, bnep-tester, sco-tester and
hci-tester give identical results before and after.

Also on a Raspberry Pi 400 with a second board as the L2CAP peer, each test
from its own boot so lockdep was armed for each. The released kernel
reproduced the connectionless lockdep warning, and both it and the
patch-1-only kernel died under teardown stress. With both patches, 300
receiver close cycles against 25,877 peer datagram floods completed with no
warning and debug_locks still 1. Connected L2CAP and A2DP playback showed
no regression on any of the three. The hardware crashes left no readable
trace, so the attributed NULL dereference above is from QEMU.

The 5.10.y fix has a different shape and was sent separately as
20260916193454.9996-1-kmehltretter@gmail.com. That tree still has
89e856e124f9, so both L2CAP hunks of f1a8f402f13f apply there unchanged and
one patch covers it.

base-commit: 0248c33e835ecbec3a591f93fdaae53f7b90a4d6

[PATCH 5.15.y 1/2] Bluetooth: L2CAP: Fix deadlock

[Karl Mehltretter]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

[ Upstream commit f1a8f402f13f94263cf349216c257b2985100927 ]

This fixes the following deadlock introduced by 39a92a55be13
("bluetooth/l2cap: sync sock recv cb and release")

============================================
WARNING: possible recursive locking detected
6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
--------------------------------------------
kworker/u5:0/35 is trying to acquire lock:
ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_sock_recv_cb+0x44/0x1e0

but task is already holding lock:
ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_get_chan_by_scid+0xaf/0xd0

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&chan->lock#2/1);
  lock(&chan->lock#2/1);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/u5:0/35:
 #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
process_one_work+0x750/0x930
 #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
at: process_one_work+0x44e/0x930
 #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_get_chan_by_scid+0xaf/0xd0

To fix the original problem this introduces l2cap_chan_lock at
l2cap_conless_channel to ensure that l2cap_sock_recv_cb is called with
chan->lock held.

Fixes: 89e856e124f9 ("bluetooth/l2cap: sync sock recv cb and release")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
[ Karl Mehltretter: backport only the l2cap_core.c changes. The HCI
  changes are from an unrelated patch accidentally squashed into this
  commit. The l2cap_sock.c change removes locking added by 89e856e124f9,
  which is absent from 5.15.y, so the quoted deadlock cannot occur. The
  core changes are needed because c531e63871c0 was backported without
  the matching lock. ]
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 34f89f7f993b..f1d7a6cdd8aa 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7992,6 +7992,8 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
+	l2cap_chan_lock(chan);
+
 	if (chan->state != BT_BOUND && chan->state != BT_CONNECTED)
 		goto drop;
 
@@ -8009,6 +8011,7 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
 	}
 
 drop:
+	l2cap_chan_unlock(chan);
 	l2cap_chan_put(chan);
 free_skb:
 	kfree_skb(skb);
-- 
2.51.0

[PATCH 5.15.y 2/2] bluetooth/l2cap: sync sock recv cb and release

[Karl Mehltretter]

From: Edward Adam Davis <eadavis@qq.com>

[ Upstream commit 89e856e124f9ae548572c56b1b70c2255705f8fe ]

The problem occurs between the system call to close the sock and hci_rx_work,
where the former releases the sock and the latter accesses it without lock protection.

           CPU0                       CPU1
           ----                       ----
           sock_close                 hci_rx_work
	   l2cap_sock_release         hci_acldata_packet
	   l2cap_sock_kill            l2cap_recv_frame
	   sk_free                    l2cap_conless_channel
	                              l2cap_sock_recv_cb

If hci_rx_work processes the data that needs to be received before the sock is
closed, then everything is normal; Otherwise, the work thread may access the
released sock when receiving data.

Add a chan mutex in the rx callback of the sock to achieve synchronization between
the sock release and recv cb.

Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer.

Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
[ Karl Mehltretter: applied in the form this commit has after
  f1a8f402f13f ("Bluetooth: L2CAP: Fix deadlock"), that is the
  chan->data clearing in l2cap_sock_kill() and the guard in
  l2cap_sock_recv_cb(), without the channel locking in the callback.
  That locking is what caused the recursive chan->lock deadlock.
  f1a8f402f13f removes it and moves the lock to l2cap_conless_channel(),
  which the previous patch does here. l2cap_data_channel() already
  obtains the channel locked from l2cap_get_chan_by_scid(). ]
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0b51c3e0f469..bef6a948d7d5 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1237,6 +1237,10 @@ static void l2cap_sock_kill(struct sock *sk)
 
 	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
 
+	/* Sock is dead, so set chan data to NULL, avoid other task use invalid
+	 * sock pointer.
+	 */
+	l2cap_pi(sk)->chan->data = NULL;
 	/* Kill poor orphan */
 
 	l2cap_chan_put(l2cap_pi(sk)->chan);
@@ -1519,9 +1523,13 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
 
 static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 {
-	struct sock *sk = chan->data;
+	struct sock *sk;
 	int err;
 
+	sk = chan->data;
+	if (!sk)
+		return -ENXIO;
+
 	lock_sock(sk);
 
 	if (l2cap_pi(sk)->rx_busy_skb) {
-- 
2.51.0

Re: [PATCH 5.15.y 0/2] Bluetooth: L2CAP: fix connectionless receive path

[Sasha Levin]

> 5.15.y has two problems on the L2CAP connectionless receive path. The first
> comes from a backport that landed without its prerequisite, the second from
> a fix that never landed at all. Upstream contains both fixes.

Queued the series for 5.15, thanks. The write-up of why only the
l2cap_core.c half of f1a8f402f13f ("Bluetooth: L2CAP: Fix deadlock")
belongs here was useful.

-- 
Thanks,
Sasha