[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: [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