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 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.

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