[PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Pauli Virtanen]

For L2CAP sockets without owning sk->sk_socket, reading
l2cap_pi(sk)->chan may race against concurrent l2cap_sock_kill() ->
l2cap_sock_put_chan().  This excludes simultaneous proto_ops callbacks,
but access in l2cap_sock_cleanup_listen() has unsafe lockless read.

Fix the race by taking lock_sock() in l2cap_sock_kill() to
synchronize with l2cap_sock_cleanup_listen().  hold_unless_zero() is not
needed here, l2cap_pi(sk)->chan owns reference if it is non-NULL.

Fixes: 0e2c0392b9dc ("Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()")
Reported-by: syzbot+e6382a2f53f5fc7453ac@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/l2cap.h |  5 +++++
 net/bluetooth/l2cap_sock.c    | 23 +++++++++++++----------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index ef6ce1c20a4f..3d9a32094347 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -699,7 +699,12 @@ struct l2cap_rx_busy {
 
 struct l2cap_pinfo {
 	struct bt_sock		bt;
+
+	/* With owning sk_socket chan may be read without lock, other access
+	 * should hold lock_sock.
+	 */
 	struct l2cap_chan	*chan;
+
 	struct list_head	rx_busy;
 };
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 735167f73f31..9540617a0e6c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1312,7 +1312,12 @@ static void l2cap_sock_kill(struct sock *sk)
 
 	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
 
+	/* Take lock to synchronize against access without owning sk->sk_socket,
+	 * eg. in l2cap_sock_cleanup_listen(). proto_ops etc. don't need lock.
+	 */
+	lock_sock(sk);
 	l2cap_sock_put_chan(sk);
+	release_sock(sk);
 
 	/* Kill poor orphan */
 	sock_set_flag(sk, SOCK_DEAD);
@@ -1516,14 +1521,10 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
 	 * establish sk_lock -> conn->lock and invert the established
 	 * conn->lock -> chan->lock -> sk_lock order (lockdep deadlock).
 	 *
-	 * Instead, briefly take the child sk lock to fetch and pin its chan.
-	 * l2cap_conn_del() reaches the chan free only via
-	 * l2cap_chan_del() -> l2cap_sock_teardown_cb(), which itself takes
-	 * the child sk lock; holding it across l2cap_chan_hold_unless_zero()
-	 * therefore guarantees the chan cannot be freed while we read and
-	 * pin it (hold_unless_zero() additionally skips a chan already past
-	 * its last reference).  We then drop the sk lock before taking
-	 * chan->lock, so sk and chan locks are never held together.
+	 * Instead, briefly take the child sk lock to synchronize vs.
+	 * l2cap_sock_kill that puts l2cap_pi(sk)->chan. We then drop the sk
+	 * lock before taking chan->lock, so sk and chan locks are never held
+	 * together.
 	 *
 	 * Since we cannot call l2cap_chan_close() without conn->lock,
 	 * schedule l2cap_chan_timeout to close the channel; it already
@@ -1533,10 +1534,12 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
 		struct l2cap_chan *chan;
 
 		lock_sock_nested(sk, L2CAP_NESTING_NORMAL);
-		chan = l2cap_chan_hold_unless_zero(l2cap_pi(sk)->chan);
+		chan = l2cap_pi(sk)->chan;
+		if (chan)
+			l2cap_chan_hold(chan);
 		release_sock(sk);
 		if (!chan) {
-			/* l2cap_conn_del() already tearing this child down */
+			/* Already torn down */
 			sock_put(sk);
 			continue;
 		}
-- 
2.55.0

RE: Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1235 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/project/bluetooth/list/?series=1138893

---Test result---

Test Summary:
CheckPatch                    PASS      9.03 seconds
VerifyFixes                   PASS      3.43 seconds
VerifySignedoff               PASS      1.99 seconds
GitLint                       PASS      0.78 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      24.55 seconds
CheckAllWarning               PASS      27.10 seconds
CheckSparse                   PASS      25.90 seconds
BuildKernel32                 PASS      23.93 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      451.75 seconds
TestRunner_l2cap-tester       PASS      63.97 seconds
IncrementalBuild              PASS      23.53 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


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

---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Hillf Danton]

On Sun,  2 Aug 2026 15:12:28 +0300 Pauli Virtanen wrote:
> For L2CAP sockets without owning sk->sk_socket, reading
> l2cap_pi(sk)->chan may race against concurrent l2cap_sock_kill() ->
> l2cap_sock_put_chan().  This excludes simultaneous proto_ops callbacks,
> but access in l2cap_sock_cleanup_listen() has unsafe lockless read.
> 
> Fix the race by taking lock_sock() in l2cap_sock_kill() to
> synchronize with l2cap_sock_cleanup_listen().  hold_unless_zero() is not
> needed here, l2cap_pi(sk)->chan owns reference if it is non-NULL.
> 
> Fixes: 0e2c0392b9dc ("Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()")
> Reported-by: syzbot+e6382a2f53f5fc7453ac@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  include/net/bluetooth/l2cap.h |  5 +++++
>  net/bluetooth/l2cap_sock.c    | 23 +++++++++++++----------
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index ef6ce1c20a4f..3d9a32094347 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -699,7 +699,12 @@ struct l2cap_rx_busy {
>  
>  struct l2cap_pinfo {
>  	struct bt_sock		bt;
> +
> +	/* With owning sk_socket chan may be read without lock, other access
> +	 * should hold lock_sock.
> +	 */
>  	struct l2cap_chan	*chan;
> +
>  	struct list_head	rx_busy;
>  };
>  
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 735167f73f31..9540617a0e6c 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1312,7 +1312,12 @@ static void l2cap_sock_kill(struct sock *sk)
>  
>  	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
>  
> +	/* Take lock to synchronize against access without owning sk->sk_socket,
> +	 * eg. in l2cap_sock_cleanup_listen(). proto_ops etc. don't need lock.
> +	 */
> +	lock_sock(sk);
>  	l2cap_sock_put_chan(sk);
> +	release_sock(sk);
>  
>  	/* Kill poor orphan */
>  	sock_set_flag(sk, SOCK_DEAD);

In l2cap_sock_teardown_cb(), sock is only zapped after cleanup including unlink,
so why do you see a linked and zapped sock in l2cap_sock_cleanup_listen()?

> @@ -1516,14 +1521,10 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
>  	 * establish sk_lock -> conn->lock and invert the established
>  	 * conn->lock -> chan->lock -> sk_lock order (lockdep deadlock).
>  	 *
> -	 * Instead, briefly take the child sk lock to fetch and pin its chan.
> -	 * l2cap_conn_del() reaches the chan free only via
> -	 * l2cap_chan_del() -> l2cap_sock_teardown_cb(), which itself takes
> -	 * the child sk lock; holding it across l2cap_chan_hold_unless_zero()
> -	 * therefore guarantees the chan cannot be freed while we read and
> -	 * pin it (hold_unless_zero() additionally skips a chan already past
> -	 * its last reference).  We then drop the sk lock before taking
> -	 * chan->lock, so sk and chan locks are never held together.
> +	 * Instead, briefly take the child sk lock to synchronize vs.
> +	 * l2cap_sock_kill that puts l2cap_pi(sk)->chan. We then drop the sk
> +	 * lock before taking chan->lock, so sk and chan locks are never held
> +	 * together.
>  	 *
>  	 * Since we cannot call l2cap_chan_close() without conn->lock,
>  	 * schedule l2cap_chan_timeout to close the channel; it already
> @@ -1533,10 +1534,12 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
>  		struct l2cap_chan *chan;
>  
>  		lock_sock_nested(sk, L2CAP_NESTING_NORMAL);
> -		chan = l2cap_chan_hold_unless_zero(l2cap_pi(sk)->chan);
> +		chan = l2cap_pi(sk)->chan;
> +		if (chan)
> +			l2cap_chan_hold(chan);
>  		release_sock(sk);
>  		if (!chan) {
> -			/* l2cap_conn_del() already tearing this child down */
> +			/* Already torn down */
>  			sock_put(sk);
>  			continue;
>  		}
> -- 
> 2.55.0

Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Pauli Virtanen]

Hi,

ma, 2026-08-03 kello 14:13 +0800, Hillf Danton kirjoitti:
> On Sun,  2 Aug 2026 15:12:28 +0300 Pauli Virtanen wrote:
> > For L2CAP sockets without owning sk->sk_socket, reading
> > l2cap_pi(sk)->chan may race against concurrent l2cap_sock_kill() ->
> > l2cap_sock_put_chan().  This excludes simultaneous proto_ops callbacks,
> > but access in l2cap_sock_cleanup_listen() has unsafe lockless read.
> > 
> > Fix the race by taking lock_sock() in l2cap_sock_kill() to
> > synchronize with l2cap_sock_cleanup_listen().  hold_unless_zero() is not
> > needed here, l2cap_pi(sk)->chan owns reference if it is non-NULL.
> > 
> > Fixes: 0e2c0392b9dc ("Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()")
> > Reported-by: syzbot+e6382a2f53f5fc7453ac@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  include/net/bluetooth/l2cap.h |  5 +++++
> >  net/bluetooth/l2cap_sock.c    | 23 +++++++++++++----------
> >  2 files changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index ef6ce1c20a4f..3d9a32094347 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -699,7 +699,12 @@ struct l2cap_rx_busy {
> >  
> >  struct l2cap_pinfo {
> >  	struct bt_sock		bt;
> > +
> > +	/* With owning sk_socket chan may be read without lock, other access
> > +	 * should hold lock_sock.
> > +	 */
> >  	struct l2cap_chan	*chan;
> > +
> >  	struct list_head	rx_busy;
> >  };
> >  
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 735167f73f31..9540617a0e6c 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -1312,7 +1312,12 @@ static void l2cap_sock_kill(struct sock *sk)
> >  
> >  	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
> >  
> > +	/* Take lock to synchronize against access without owning sk->sk_socket,
> > +	 * eg. in l2cap_sock_cleanup_listen(). proto_ops etc. don't need lock.
> > +	 */
> > +	lock_sock(sk);
> >  	l2cap_sock_put_chan(sk);
> > +	release_sock(sk);
> >  
> >  	/* Kill poor orphan */
> >  	sock_set_flag(sk, SOCK_DEAD);
> 
> In l2cap_sock_teardown_cb(), sock is only zapped after cleanup including unlink,
> so why do you see a linked and zapped sock in l2cap_sock_cleanup_listen()?

l2cap_sock_cleanup_listen() is not a single critical section.

There is the following race:

   [Task 1]                         [Task 2 (hdev->workqueue)]
   l2cap_sock_release(parent)       l2cap_disconn_cfm
     l2cap_sock_cleanup_listen        l2cap_conn_del
       bt_accept_dequeue                l2cap_chan_del
         lock_sock(sk)                    l2cap_sock_teardown_cb
         bt_accept_unlink
           bt_sk(sk)->parent = NULL
         release_sock(sk) ----------------> lock_sock(sk)
                                            parent = bt_sk(sk)->parent /* == NULL */
       lock_sock(sk) <--------------------- release_sock(sk)
                                            sock_set_flag(sk, SOCK_ZAPPED)
                                        l2cap_sock_close_cb
                                          l2cap_sock_kill(sk)
                                            l2cap_sock_put_chan
       chan = READ l2cap_pi(sk)->chan         l2cap_pi(sk)->chan = NULL
       l2cap_chan_hold_unless_zero            l2cap_put_chan(chan)
         kref_get_unless_zero(&chan->ref)

> > @@ -1516,14 +1521,10 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
> >  	 * establish sk_lock -> conn->lock and invert the established
> >  	 * conn->lock -> chan->lock -> sk_lock order (lockdep deadlock).
> >  	 *
> > -	 * Instead, briefly take the child sk lock to fetch and pin its chan.
> > -	 * l2cap_conn_del() reaches the chan free only via
> > -	 * l2cap_chan_del() -> l2cap_sock_teardown_cb(), which itself takes
> > -	 * the child sk lock; holding it across l2cap_chan_hold_unless_zero()
> > -	 * therefore guarantees the chan cannot be freed while we read and
> > -	 * pin it (hold_unless_zero() additionally skips a chan already past
> > -	 * its last reference).  We then drop the sk lock before taking
> > -	 * chan->lock, so sk and chan locks are never held together.
> > +	 * Instead, briefly take the child sk lock to synchronize vs.
> > +	 * l2cap_sock_kill that puts l2cap_pi(sk)->chan. We then drop the sk
> > +	 * lock before taking chan->lock, so sk and chan locks are never held
> > +	 * together.
> >  	 *
> >  	 * Since we cannot call l2cap_chan_close() without conn->lock,
> >  	 * schedule l2cap_chan_timeout to close the channel; it already
> > @@ -1533,10 +1534,12 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
> >  		struct l2cap_chan *chan;
> >  
> >  		lock_sock_nested(sk, L2CAP_NESTING_NORMAL);
> > -		chan = l2cap_chan_hold_unless_zero(l2cap_pi(sk)->chan);
> > +		chan = l2cap_pi(sk)->chan;
> > +		if (chan)
> > +			l2cap_chan_hold(chan);
> >  		release_sock(sk);
> >  		if (!chan) {
> > -			/* l2cap_conn_del() already tearing this child down */
> > +			/* Already torn down */
> >  			sock_put(sk);
> >  			continue;
> >  		}
> > -- 
> > 2.55.0

Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Hillf Danton]

On Mon, 03 Aug 2026 19:53:31 +0300 Pauli Virtanen wrote:
> ma, 2026-08-03 kello 14:13 +0800, Hillf Danton kirjoitti:
> > On Sun,  2 Aug 2026 15:12:28 +0300 Pauli Virtanen wrote:
> > > For L2CAP sockets without owning sk->sk_socket, reading
> > > l2cap_pi(sk)->chan may race against concurrent l2cap_sock_kill() ->
> > > l2cap_sock_put_chan().  This excludes simultaneous proto_ops callbacks,
> > > but access in l2cap_sock_cleanup_listen() has unsafe lockless read.
> > > 
> > > Fix the race by taking lock_sock() in l2cap_sock_kill() to
> > > synchronize with l2cap_sock_cleanup_listen().  hold_unless_zero() is not
> > > needed here, l2cap_pi(sk)->chan owns reference if it is non-NULL.
> > > 
> > > Fixes: 0e2c0392b9dc ("Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()")
> > > Reported-by: syzbot+e6382a2f53f5fc7453ac@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > ---
> > >  include/net/bluetooth/l2cap.h |  5 +++++
> > >  net/bluetooth/l2cap_sock.c    | 23 +++++++++++++----------
> > >  2 files changed, 18 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > > index ef6ce1c20a4f..3d9a32094347 100644
> > > --- a/include/net/bluetooth/l2cap.h
> > > +++ b/include/net/bluetooth/l2cap.h
> > > @@ -699,7 +699,12 @@ struct l2cap_rx_busy {
> > >  
> > >  struct l2cap_pinfo {
> > >  	struct bt_sock		bt;
> > > +
> > > +	/* With owning sk_socket chan may be read without lock, other access
> > > +	 * should hold lock_sock.
> > > +	 */
> > >  	struct l2cap_chan	*chan;
> > > +
> > >  	struct list_head	rx_busy;
> > >  };
> > >  
> > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > > index 735167f73f31..9540617a0e6c 100644
> > > --- a/net/bluetooth/l2cap_sock.c
> > > +++ b/net/bluetooth/l2cap_sock.c
> > > @@ -1312,7 +1312,12 @@ static void l2cap_sock_kill(struct sock *sk)
> > >  
> > >  	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
> > >  
> > > +	/* Take lock to synchronize against access without owning sk->sk_socket,
> > > +	 * eg. in l2cap_sock_cleanup_listen(). proto_ops etc. don't need lock.
> > > +	 */
> > > +	lock_sock(sk);
> > >  	l2cap_sock_put_chan(sk);
> > > +	release_sock(sk);
> > >  
> > >  	/* Kill poor orphan */
> > >  	sock_set_flag(sk, SOCK_DEAD);
> > 
> > In l2cap_sock_teardown_cb(), sock is only zapped after cleanup including unlink,
> > so why do you see a linked and zapped sock in l2cap_sock_cleanup_listen()?
> 
> l2cap_sock_cleanup_listen() is not a single critical section.
> 
> There is the following race:
> 
>    [Task 1]                         [Task 2 (hdev->workqueue)]
>    l2cap_sock_release(parent)       l2cap_disconn_cfm
>      l2cap_sock_cleanup_listen        l2cap_conn_del
>        bt_accept_dequeue                l2cap_chan_del
>          lock_sock(sk)                    l2cap_sock_teardown_cb
>          bt_accept_unlink
>            bt_sk(sk)->parent = NULL
>          release_sock(sk) ----------------> lock_sock(sk)
>                                             parent = bt_sk(sk)->parent /* == NULL */
>        lock_sock(sk) <--------------------- release_sock(sk)
>                                             sock_set_flag(sk, SOCK_ZAPPED)
>                                         l2cap_sock_close_cb
>                                           l2cap_sock_kill(sk)
>                                             l2cap_sock_put_chan
>        chan = READ l2cap_pi(sk)->chan         l2cap_pi(sk)->chan = NULL
>        l2cap_chan_hold_unless_zero            l2cap_put_chan(chan)
>          kref_get_unless_zero(&chan->ref)
>
The race window is still open after this work.

					     release_sock(sk)
                                             sock_set_flag(sk, SOCK_ZAPPED)
                                         l2cap_sock_close_cb
                                           l2cap_sock_kill(sk)
                                             l2cap_sock_put_chan
					       l2cap_pi(sk)->chan = NULL
					       l2cap_put_chan(chan)
					     sock_set_flag(sk, SOCK_DEAD);
					     sock_put(sk); // free sk
        lock_sock(sk) // uaf
        chan = READ l2cap_pi(sk)->chan
        l2cap_chan_hold_unless_zero

Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Pauli Virtanen]

Hi,

ti, 2026-08-04 kello 08:47 +0800, Hillf Danton kirjoitti:
> On Mon, 03 Aug 2026 19:53:31 +0300 Pauli Virtanen wrote:
> > ma, 2026-08-03 kello 14:13 +0800, Hillf Danton kirjoitti:
> > > On Sun,  2 Aug 2026 15:12:28 +0300 Pauli Virtanen wrote:
> > > > For L2CAP sockets without owning sk->sk_socket, reading
> > > > l2cap_pi(sk)->chan may race against concurrent
> > > > l2cap_sock_kill() ->
> > > > l2cap_sock_put_chan().  This excludes simultaneous proto_ops
> > > > callbacks,
> > > > but access in l2cap_sock_cleanup_listen() has unsafe lockless
> > > > read.
> > > > 
> > > > Fix the race by taking lock_sock() in l2cap_sock_kill() to
> > > > synchronize with l2cap_sock_cleanup_listen(). 
> > > > hold_unless_zero() is not
> > > > needed here, l2cap_pi(sk)->chan owns reference if it is non-
> > > > NULL.
> > > > 
> > > > Fixes: 0e2c0392b9dc ("Bluetooth: L2CAP: Fix use-after-free in
> > > > l2cap_sock_new_connection_cb()")
> > > > Reported-by:
> > > > syzbot+e6382a2f53f5fc7453ac@syzkaller.appspotmail.com
> > > > Closes:
> > > > https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
> > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > ---
> > > >  include/net/bluetooth/l2cap.h |  5 +++++
> > > >  net/bluetooth/l2cap_sock.c    | 23 +++++++++++++----------
> > > >  2 files changed, 18 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/include/net/bluetooth/l2cap.h
> > > > b/include/net/bluetooth/l2cap.h
> > > > index ef6ce1c20a4f..3d9a32094347 100644
> > > > --- a/include/net/bluetooth/l2cap.h
> > > > +++ b/include/net/bluetooth/l2cap.h
> > > > @@ -699,7 +699,12 @@ struct l2cap_rx_busy {
> > > >  
> > > >  struct l2cap_pinfo {
> > > >  	struct bt_sock		bt;
> > > > +
> > > > +	/* With owning sk_socket chan may be read without
> > > > lock, other access
> > > > +	 * should hold lock_sock.
> > > > +	 */
> > > >  	struct l2cap_chan	*chan;
> > > > +
> > > >  	struct list_head	rx_busy;
> > > >  };
> > > >  
> > > > diff --git a/net/bluetooth/l2cap_sock.c
> > > > b/net/bluetooth/l2cap_sock.c
> > > > index 735167f73f31..9540617a0e6c 100644
> > > > --- a/net/bluetooth/l2cap_sock.c
> > > > +++ b/net/bluetooth/l2cap_sock.c
> > > > @@ -1312,7 +1312,12 @@ static void l2cap_sock_kill(struct sock
> > > > *sk)
> > > >  
> > > >  	BT_DBG("sk %p state %s", sk, state_to_string(sk-
> > > > >sk_state));
> > > >  
> > > > +	/* Take lock to synchronize against access without
> > > > owning sk->sk_socket,
> > > > +	 * eg. in l2cap_sock_cleanup_listen(). proto_ops etc.
> > > > don't need lock.
> > > > +	 */
> > > > +	lock_sock(sk);
> > > >  	l2cap_sock_put_chan(sk);
> > > > +	release_sock(sk);
> > > >  
> > > >  	/* Kill poor orphan */
> > > >  	sock_set_flag(sk, SOCK_DEAD);
> > > 
> > > In l2cap_sock_teardown_cb(), sock is only zapped after cleanup
> > > including unlink,
> > > so why do you see a linked and zapped sock in
> > > l2cap_sock_cleanup_listen()?
> > 
> > l2cap_sock_cleanup_listen() is not a single critical section.
> > 
> > There is the following race:
> > 
> >    [Task 1]                         [Task 2 (hdev->workqueue)]
> >    l2cap_sock_release(parent)       l2cap_disconn_cfm
> >      l2cap_sock_cleanup_listen        l2cap_conn_del
> >        bt_accept_dequeue                l2cap_chan_del
> >          lock_sock(sk)                    l2cap_sock_teardown_cb
> >          bt_accept_unlink
> >            bt_sk(sk)->parent = NULL
> >          release_sock(sk) ----------------> lock_sock(sk)
> >                                             parent = bt_sk(sk)-
> > >parent /* == NULL */
> >        lock_sock(sk) <--------------------- release_sock(sk)
> >                                             sock_set_flag(sk,
> > SOCK_ZAPPED)
> >                                         l2cap_sock_close_cb
> >                                           l2cap_sock_kill(sk)
> >                                             l2cap_sock_put_chan
> >        chan = READ l2cap_pi(sk)->chan         l2cap_pi(sk)->chan =
> > NULL
> >        l2cap_chan_hold_unless_zero            l2cap_put_chan(chan)
> >          kref_get_unless_zero(&chan->ref)
> > 
> The race window is still open after this work.
> 
> 					     release_sock(sk)
>                                              sock_set_flag(sk,
> SOCK_ZAPPED)
>                                          l2cap_sock_close_cb
>                                            l2cap_sock_kill(sk)
>                                              l2cap_sock_put_chan
> 					       l2cap_pi(sk)->chan =
> NULL
> 					       l2cap_put_chan(chan)
> 					     sock_set_flag(sk,
> SOCK_DEAD);
> 					     sock_put(sk); // free
> sk
>         lock_sock(sk) // uaf
>         chan = READ l2cap_pi(sk)->chan
>         l2cap_chan_hold_unless_zero

There is no UAF there, Task 1 holds a reference on sk at this point, if
you look at the code sock_put() follows.

I don't think there is a remaining problem.

-- 
Pauli Virtanen

Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Hillf Danton]

On Tue, 04 Aug 2026 08:40:16 +0300 Pauli Virtanen wrote:
>
> There is no UAF there, Task 1 holds a reference on sk at this point, if
> you look at the code sock_put() follows.
> 
If the subsequent put works without both race and mm leak, then it is the
very evidence of uaf with race.

Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Pauli Virtanen]

ti, 2026-08-04 kello 16:16 +0800, Hillf Danton kirjoitti:
> On Tue, 04 Aug 2026 08:40:16 +0300 Pauli Virtanen wrote:
> > ti, 2026-08-04 kello 08:47 +0800, Hillf Danton kirjoitti:
> > > On Mon, 03 Aug 2026 19:53:31 +0300 Pauli Virtanen wrote:
> > > >
> > > >    [Task 1]                         [Task 2 (hdev->workqueue)]
> > > >    l2cap_sock_release(parent)       l2cap_disconn_cfm
> > > >      l2cap_sock_cleanup_listen        l2cap_conn_del
> > > >        bt_accept_dequeue                l2cap_chan_del
> > > >          lock_sock(sk)                    l2cap_sock_teardown_cb
> > > >          bt_accept_unlink
> > > >            bt_sk(sk)->parent = NULL
> > > >          release_sock(sk) ----------------> lock_sock(sk)
> > > >                                             parent = bt_sk(sk)->parent /* == NULL */
> > > >        lock_sock(sk) <--------------------- release_sock(sk)
> > > >                                             sock_set_flag(sk, SOCK_ZAPPED)
> > > >                                         l2cap_sock_close_cb
> > > >                                           l2cap_sock_kill(sk)
> > > >                                             l2cap_sock_put_chan
> > > >        chan = READ l2cap_pi(sk)->chan         l2cap_pi(sk)->chan = NULL
> > > >        l2cap_chan_hold_unless_zero            l2cap_put_chan(chan)
> > > >          kref_get_unless_zero(&chan->ref)
> > >
> > > The race window is still open after this work.
> > >
> > > 					     release_sock(sk)
> > >                                              sock_set_flag(sk, SOCK_ZAPPED)
> > >                                          l2cap_sock_close_cb
> > >                                            l2cap_sock_kill(sk)
> > >                                              l2cap_sock_put_chan
> > > 					       l2cap_pi(sk)->chan = NULL
> > > 					       l2cap_put_chan(chan)
> > > 					     sock_set_flag(sk, SOCK_DEAD);
> > > 					     sock_put(sk); // free sk
> > >         lock_sock(sk) // uaf
> > >         chan = READ l2cap_pi(sk)->chan
> > >         l2cap_chan_hold_unless_zero
> > 
> > There is no UAF there, Task 1 holds a reference on sk at this point, if
> > you look at the code sock_put() follows.
> > 
> If the subsequent put works without both race and mm leak, then it is the
> very evidence of uaf with race.

This and the "// uaf" and "// free sk" above appear to ignore what is
explained also in the comment in l2cap_sock_cleanup_listen():

	 * bt_accept_dequeue() returns sk with its temporary queue-
walk
	 * reference held, so a concurrent l2cap_conn_del()
	 * -> l2cap_sock_kill() cannot free sk under us.

bt_accept_dequeue() returns a new temporary reference to sk, which is
owned by the caller who needs to put it after use.

The "sock_put(sk); // free sk" cannot free sk, because
bt_accept_dequeue(sk) got a new reference on sk before it, so the
refcount is at least 2 here. Then "lock_sock(sk) // uaf" cannot be UAF.

If chan != NULL in Task 1, it schedules l2cap_chan teardown. That
eventually ends up in l2cap_sock_kill() to put the reference owned by
l2cap_chan & sk_socket.

If chan == NULL in Task 1, l2cap_chan cleanup is completed or
concurrent, and nothing more needs to be done.

-- 
Pauli Virtanen