Re: [PATCH RESEND v4 1/1] Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()

["Siwei Zhang" <oss@fourdim.xyz>]

Hi Luiz,

On Mon, May 18, 2026, at 5:57 PM, Luiz Augusto von Dentz wrote:
> Hi Siwei,
>
> On Thu, May 14, 2026 at 8:23 AM Siwei Zhang <oss@fourdim.xyz> wrote:
>>
>> Hi Luiz,
>>
>> On Mon, May 11, 2026, at 3:17 PM, Luiz Augusto von Dentz wrote:
>> > Hi,
>> >
>> > On Mon, May 11, 2026 at 1:09 PM Siwei Zhang <oss@fourdim.xyz> wrote:
>> >>
>> >> l2cap_sock_new_connection_cb() accesses l2cap_pi(sk)->chan after
>> >> release_sock(parent). Once the parent lock is released, the child
>> >> socket sk can be freed by another task.
>> >>
>> >> Save the channel pointer into a local variable while the parent lock
>> >> is still held to prevent this.
>> >>
>> >> Fixes: 8ffb929098a5 ("Bluetooth: Remove parent socket usage from l2cap_core.c")
>> >> Cc: stable@kernel.org
>> >> Assisted-by: Claude:claude-opus-4-7
>> >> Signed-off-by: Siwei Zhang <oss@fourdim.xyz>
>> >> ---
>> >>  net/bluetooth/6lowpan.c    |  5 +++++
>> >>  net/bluetooth/l2cap_core.c | 12 ++++++++++++
>> >>  net/bluetooth/l2cap_sock.c | 13 ++++++++++++-
>> >>  net/bluetooth/smp.c        |  5 +++++
>> >>  4 files changed, 34 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> >> index 23a229ab6a33..71c1c04b61e5 100644
>> >> --- a/net/bluetooth/6lowpan.c
>> >> +++ b/net/bluetooth/6lowpan.c
>> >> @@ -755,6 +755,11 @@ static inline struct l2cap_chan *chan_new_conn_cb(struct l2cap_chan *pchan)
>> >>
>> >>         BT_DBG("chan %p pchan %p", chan, pchan);
>> >>
>> >> +       /* Match the put that the caller of ops->new_connection() performs
>> >> +        * once it is done with the returned channel pointer.
>> >> +        */
>> >> +       l2cap_chan_hold(chan);
>> >> +
>> >>         return chan;
>> >>  }
>> >>
>> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> >> index 7701528f1167..0f6c3c651207 100644
>> >> --- a/net/bluetooth/l2cap_core.c
>> >> +++ b/net/bluetooth/l2cap_core.c
>> >> @@ -4071,6 +4071,9 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
>> >>
>> >>         __l2cap_chan_add(conn, chan);
>> >>
>> >> +       /* Drop the ops->new_connection() ref; conn list now pins chan. */
>> >> +       l2cap_chan_put(chan);
>> >> +
>> >>         dcid = chan->scid;
>> >>
>> >>         __set_chan_timer(chan, chan->ops->get_sndtimeo(chan));
>> >> @@ -4970,6 +4973,9 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>> >>
>> >>         __l2cap_chan_add(conn, chan);
>> >>
>> >> +       /* Drop the ops->new_connection() ref; conn list now pins chan. */
>> >> +       l2cap_chan_put(chan);
>> >> +
>> >>         l2cap_le_flowctl_init(chan, __le16_to_cpu(req->credits));
>> >>
>> >>         dcid = chan->scid;
>> >> @@ -5194,6 +5200,9 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> >>
>> >>                 __l2cap_chan_add(conn, chan);
>> >>
>> >> +               /* Drop the ops->new_connection() ref; conn list now pins chan. */
>> >> +               l2cap_chan_put(chan);
>> >> +
>> >>                 l2cap_ecred_init(chan, __le16_to_cpu(req->credits));
>> >>
>> >>                 /* Init response */
>> >> @@ -7407,6 +7416,9 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>> >>                         chan->dst_type = dst_type;
>> >>
>> >>                         __l2cap_chan_add(conn, chan);
>> >> +
>> >> +                       /* Drop the ops->new_connection() ref; conn list now pins chan. */
>> >> +                       l2cap_chan_put(chan);
>> >>                 }
>> >>
>> >>                 l2cap_chan_unlock(pchan);
>> >> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>> >> index cf590a67d364..295c79cf5cf3 100644
>> >> --- a/net/bluetooth/l2cap_sock.c
>> >> +++ b/net/bluetooth/l2cap_sock.c
>> >> @@ -1497,6 +1497,7 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
>> >>  static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
>> >>  {
>> >>         struct sock *sk, *parent = chan->data;
>> >> +       struct l2cap_chan *child_chan;
>> >>
>> >>         if (!parent)
>> >>                 return NULL;
>> >> @@ -1523,9 +1524,19 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
>> >>
>> >>         bt_accept_enqueue(parent, sk, false);
>> >>
>> >> +       child_chan = l2cap_pi(sk)->chan;
>> >> +
>> >> +       /* Pin the channel for the caller. Once release_sock(parent) returns,
>> >> +        * userspace can accept(2) and immediately close(2) the child socket,
>> >> +        * which would drop the socket's references on the channel and free
>> >> +        * it before the caller (e.g. l2cap_connect_req()) is done using the
>> >> +        * returned pointer. The matching put is the caller's responsibility.
>> >> +        */
>> >> +       l2cap_chan_hold(child_chan);
>> >
>> > The entire problem might be solvable by not removing `list_add` from
>> > `l2cap_create_chan`. This way, it only allocates but does not attach
>> > to global_l until __l2cap_chan_add is called which then handles the
>> > addition.
>>
>> Could you please clarify what is "not removing `list_add` from
>> `l2cap_chan_create`"? I am not touching that part of code nor removing
>> the `list_add`. Do you want me to correspond the `chan` lifetime to `chan_list`?
>>
>> > Alternatively, we could allocate it first, given the
>> > circular dependency involving `l2cap_core`.c->l2cap_sock.c)
>> > new_connection -> l2cap_chan_create(l2cap_sock.c->l2cap_core.c) which
>>
>> This will change the signature of `l2cap_ops.new_connection`, which will be a large
>> refactoring across multiple files.
>>
>> Signature will be changed from
>> `struct l2cap_chan      *(*new_connection) (struct l2cap_chan *chan);`
>> to
>> `void (*new_connection) (struct l2cap_chan *chan, struct l2cap_chan *new_chan)`
>> Do you want me to this in this patch or in the follow up patch?
>> If in this patch, I will drop the backport cc to stable.
>>
>> > makes the code rather hard to follow.
>> >
>>
>> I totally understand this will create a maintenance problem. I created this patch
>> mainly because it can be easily to be backported to the stable branches and it
>> is the safest fix (though ugly). I can send a follow up patch for this to refactor it
>> according to your suggestions.
>
> I don't think adding a new argument would be complicated. This
> argument could replace the current void return, and although it will
> affect a couple more places, it shouldn't be hard to backport if it
> really needs to be backported to begin with.
>

OK, I will do that.
Also, could you please check emails titled with
[PATCH v7] Bluetooth: L2CAP: Fix slab-use-after-free in l2cap_sock_cleanup_listen()
I sent it to you privately with CC to security@kernel.org.

>>
>> >>         release_sock(parent);
>> >>
>> >> -       return l2cap_pi(sk)->chan;
>> >> +       return child_chan;
>> >>  }
>> >>
>> >>  static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
>> >> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
>> >> index 1739c1989dbd..9796c3030434 100644
>> >> --- a/net/bluetooth/smp.c
>> >> +++ b/net/bluetooth/smp.c
>> >> @@ -3231,6 +3231,11 @@ static inline struct l2cap_chan *smp_new_conn_cb(struct l2cap_chan *pchan)
>> >>
>> >>         BT_DBG("created chan %p", chan);
>> >>
>> >> +       /* Match the put that the caller of ops->new_connection() performs
>> >> +        * once it is done with the returned channel pointer.
>> >> +        */
>> >> +       l2cap_chan_hold(chan);
>> >> +
>> >>         return chan;
>> >>  }
>> >>
>> >> --
>> >> 2.54.0
>> >>
>> >
>> >
>> > --
>> > Luiz Augusto von Dentz
>>
>> Best,
>> Siwei
>
>
>
> -- 
> Luiz Augusto von Dentz

Best,
Siwei