From: "Siwei Zhang" <oss@fourdim.xyz>
To: "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>
Cc: "Marcel Holtmann" <marcel@holtmann.org>, linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v5 1/1] Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb()
Date: Wed, 27 May 2026 00:16:39 -0400 [thread overview]
Message-ID: <c45459c5-e7d9-4ea2-bd61-903a33f8778f@app.fastmail.com> (raw)
In-Reply-To: <CABBYNZL_ymHCE+uf6kJ2Mi=R4pEwy-wcSL3HKDP0krd7D73Kqw@mail.gmail.com>
Hi Luiz,
On Tue, May 26, 2026, at 4:46 PM, Luiz Augusto von Dentz wrote:
> Hi Siwei,
>
> On Wed, May 20, 2026 at 12:20 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.
>>
>> Allocate the channel outside the func to prevent this.
>>
>> Fixes: 8ffb929098a5 ("Bluetooth: Remove parent socket usage from l2cap_core.c")
>> Cc: stable@kernel.org
>> Assisted-by: Claude:claude-opus-4-6
>> Signed-off-by: Siwei Zhang <oss@fourdim.xyz>
>> ---
>> include/net/bluetooth/l2cap.h | 8 +++--
>> net/bluetooth/6lowpan.c | 14 ++++-----
>> net/bluetooth/l2cap_core.c | 58 ++++++++++++++++++++++++++++-------
>> net/bluetooth/l2cap_sock.c | 48 +++++++++++++++++------------
>> net/bluetooth/smp.c | 13 +++-----
>> 5 files changed, 91 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 5172afee5494..f7a11e6431f0 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -619,7 +619,8 @@ struct l2cap_chan {
>> struct l2cap_ops {
>> char *name;
>>
>> - struct l2cap_chan *(*new_connection) (struct l2cap_chan *chan);
>> + int (*new_connection)(struct l2cap_chan *chan,
>> + struct l2cap_chan *new_chan);
>> int (*recv) (struct l2cap_chan * chan,
>> struct sk_buff *skb);
>> void (*teardown) (struct l2cap_chan *chan, int err);
>> @@ -883,9 +884,10 @@ static inline __u16 __next_seq(struct l2cap_chan *chan, __u16 seq)
>> return (seq + 1) % (chan->tx_win_max + 1);
>> }
>>
>> -static inline struct l2cap_chan *l2cap_chan_no_new_connection(struct l2cap_chan *chan)
>> +static inline int l2cap_chan_no_new_connection(struct l2cap_chan *chan,
>> + struct l2cap_chan *new_chan)
>> {
>> - return NULL;
>> + return -EOPNOTSUPP;
>> }
>>
>> static inline int l2cap_chan_no_recv(struct l2cap_chan *chan, struct sk_buff *skb)
>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> index 23a229ab6a33..286c0b45055b 100644
>> --- a/net/bluetooth/6lowpan.c
>> +++ b/net/bluetooth/6lowpan.c
>> @@ -743,19 +743,19 @@ static inline void chan_ready_cb(struct l2cap_chan *chan)
>> ifup(dev->netdev);
>> }
>>
>> -static inline struct l2cap_chan *chan_new_conn_cb(struct l2cap_chan *pchan)
>> +static inline int chan_new_conn_cb(struct l2cap_chan *pchan,
>> + struct l2cap_chan *chan)
>> {
>> - struct l2cap_chan *chan;
>> -
>> - chan = chan_create();
>> - if (!chan)
>> - return NULL;
>> + l2cap_chan_set_defaults(chan);
>>
>> + chan->chan_type = L2CAP_CHAN_CONN_ORIENTED;
>> + chan->mode = L2CAP_MODE_LE_FLOWCTL;
>> + chan->imtu = 1280;
>
> The 3 lines above make no sense.
>
chan_create code in 6lowpan.c
static struct l2cap_chan *chan_create(void)
{
struct l2cap_chan *chan;
chan = l2cap_chan_create();
if (!chan)
return NULL;
l2cap_chan_set_defaults(chan);
chan->chan_type = L2CAP_CHAN_CONN_ORIENTED;
chan->mode = L2CAP_MODE_LE_FLOWCTL;
chan->imtu = 1280;
return chan;
}
Since we allocate chan outside and replace the chan_create here,
I do think these are needed and they are specific to 6lowpan only.
I can refactor it in this patch or in a follow-up patch. I would prefer it
to be in a follow-up patch.
>> chan->ops = pchan->ops;
>>
>> BT_DBG("chan %p pchan %p", chan, pchan);
>>
>> - return chan;
>> + return 0;
>> }
>>
>> static void unregister_dev(struct lowpan_btle_dev *dev)
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index fdccd62ccca8..505f32034971 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -4051,10 +4051,16 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
>> goto response;
>> }
>>
>> - chan = pchan->ops->new_connection(pchan);
>> + chan = l2cap_chan_create();
>> if (!chan)
>> goto response;
>>
>> + if (pchan->ops->new_connection(pchan, chan) < 0) {
>> + l2cap_chan_put(chan);
>> + chan = NULL;
>> + goto response;
>> + }
>> +
>> /* For certain devices (ex: HID mouse), support for authentication,
>> * pairing and bonding is optional. For such devices, inorder to avoid
>> * the ACL alive for too long after L2CAP disconnection, reset the ACL
>> @@ -4132,6 +4138,10 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
>> chan->num_conf_req++;
>> }
>>
>> + /* Drop our local ref; __l2cap_chan_add() pinned chan via the conn list. */
>> + if (chan)
>> + l2cap_chan_put(chan);
>> +
>> l2cap_chan_unlock(pchan);
>> l2cap_chan_put(pchan);
>> }
>> @@ -4881,6 +4891,7 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>> struct l2cap_le_conn_rsp rsp;
>> struct l2cap_chan *chan, *pchan;
>> u16 dcid, scid, credits, mtu, mps;
>> + u16 rsp_mtu, rsp_mps;
>> __le16 psm;
>> u8 result;
>>
>> @@ -4893,6 +4904,8 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>> psm = req->psm;
>> dcid = 0;
>> credits = 0;
>> + rsp_mtu = 0;
>> + rsp_mps = 0;
>>
>> if (mtu < 23 || mps < 23)
>> return -EPROTO;
>> @@ -4953,12 +4966,19 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>> goto response_unlock;
>> }
>>
>> - chan = pchan->ops->new_connection(pchan);
>> + chan = l2cap_chan_create();
>> if (!chan) {
>> result = L2CAP_CR_LE_NO_MEM;
>> goto response_unlock;
>> }
>>
>> + if (pchan->ops->new_connection(pchan, chan) < 0) {
>> + l2cap_chan_put(chan);
>> + chan = NULL;
>> + result = L2CAP_CR_LE_NO_MEM;
>> + goto response_unlock;
>> + }
>> +
>> bacpy(&chan->src, &conn->hcon->src);
>> bacpy(&chan->dst, &conn->hcon->dst);
>> chan->src_type = bdaddr_src_type(conn->hcon);
>> @@ -4974,6 +4994,8 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>>
>> dcid = chan->scid;
>> credits = chan->rx_credits;
>> + rsp_mtu = chan->imtu;
>> + rsp_mps = chan->mps;
>>
>> __set_chan_timer(chan, chan->ops->get_sndtimeo(chan));
>>
>> @@ -4993,6 +5015,9 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>> result = L2CAP_CR_LE_SUCCESS;
>> }
>>
>> + /* Drop our local ref; __l2cap_chan_add() pinned chan via the conn list. */
>> + l2cap_chan_put(chan);
>> +
>> response_unlock:
>> l2cap_chan_unlock(pchan);
>> l2cap_chan_put(pchan);
>> @@ -5001,13 +5026,8 @@ static int l2cap_le_connect_req(struct l2cap_conn *conn,
>> return 0;
>>
>> response:
>> - if (chan) {
>> - rsp.mtu = cpu_to_le16(chan->imtu);
>> - rsp.mps = cpu_to_le16(chan->mps);
>> - } else {
>> - rsp.mtu = 0;
>> - rsp.mps = 0;
>> - }
>> + rsp.mtu = cpu_to_le16(rsp_mtu);
>> + rsp.mps = cpu_to_le16(rsp_mps);
>>
>> rsp.dcid = cpu_to_le16(dcid);
>> rsp.credits = cpu_to_le16(credits);
>> @@ -5177,12 +5197,18 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> continue;
>> }
>>
>> - chan = pchan->ops->new_connection(pchan);
>> + chan = l2cap_chan_create();
>> if (!chan) {
>> result = L2CAP_CR_LE_NO_MEM;
>> continue;
>> }
>>
>> + if (pchan->ops->new_connection(pchan, chan) < 0) {
>> + l2cap_chan_put(chan);
>> + result = L2CAP_CR_LE_NO_MEM;
>> + continue;
>> + }
>> +
>> bacpy(&chan->src, &conn->hcon->src);
>> bacpy(&chan->dst, &conn->hcon->dst);
>> chan->src_type = bdaddr_src_type(conn->hcon);
>> @@ -5217,6 +5243,9 @@ static inline int l2cap_ecred_conn_req(struct l2cap_conn *conn,
>> } else {
>> l2cap_chan_ready(chan);
>> }
>> +
>> + /* Drop our local ref; __l2cap_chan_add() pinned chan via the conn list. */
>> + l2cap_chan_put(chan);
>> }
>>
>> unlock:
>> @@ -7399,7 +7428,11 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>> goto next;
>>
>> l2cap_chan_lock(pchan);
>> - chan = pchan->ops->new_connection(pchan);
>> + chan = l2cap_chan_create();
>> + if (chan && pchan->ops->new_connection(pchan, chan) < 0) {
>> + l2cap_chan_put(chan);
>> + chan = NULL;
>> + }
>> if (chan) {
>> bacpy(&chan->src, &hcon->src);
>> bacpy(&chan->dst, &hcon->dst);
>> @@ -7407,6 +7440,9 @@ static void l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>> chan->dst_type = dst_type;
>>
>> __l2cap_chan_add(conn, chan);
>> +
>> + /* Drop our local ref; __l2cap_chan_add() pinned chan via the conn list. */
>> + l2cap_chan_put(chan);
>> }
>>
>> l2cap_chan_unlock(pchan);
>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>> index dede550d6031..598f24c8f704 100644
>> --- a/net/bluetooth/l2cap_sock.c
>> +++ b/net/bluetooth/l2cap_sock.c
>> @@ -46,7 +46,8 @@ static struct bt_sock_list l2cap_sk_list = {
>> static const struct proto_ops l2cap_sock_ops;
>> static void l2cap_sock_init(struct sock *sk, struct sock *parent);
>> static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
>> - int proto, gfp_t prio, int kern);
>> + int proto, gfp_t prio, int kern,
>> + struct l2cap_chan *chan);
>> static void l2cap_sock_cleanup_listen(struct sock *parent);
>>
>> bool l2cap_is_socket(struct socket *sock)
>> @@ -1507,12 +1508,13 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
>> }
>> }
>>
>> -static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
>> +static int l2cap_sock_new_connection_cb(struct l2cap_chan *chan,
>> + struct l2cap_chan *new_chan)
>> {
>> struct sock *sk, *parent = chan->data;
>>
>> if (!parent)
>> - return NULL;
>> + return -EINVAL;
>>
>> lock_sock(parent);
>>
>> @@ -1520,15 +1522,15 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
>> if (sk_acceptq_is_full(parent)) {
>> BT_DBG("backlog full %d", parent->sk_ack_backlog);
>> release_sock(parent);
>> - return NULL;
>> + return -ENOBUFS;
>> }
>>
>> sk = l2cap_sock_alloc(sock_net(parent), NULL, BTPROTO_L2CAP,
>> - GFP_ATOMIC, 0);
>> + GFP_ATOMIC, 0, new_chan);
>> if (!sk) {
>> release_sock(parent);
>> - return NULL;
>> - }
>> + return -ENOMEM;
>> + }
>>
>> bt_sock_reclassify_lock(sk, BTPROTO_L2CAP);
>>
>> @@ -1538,7 +1540,7 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
>>
>> release_sock(parent);
>>
>> - return l2cap_pi(sk)->chan;
>> + return 0;
>> }
>>
>> static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
>> @@ -1939,10 +1941,10 @@ static struct proto l2cap_proto = {
>> };
>>
>> static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
>> - int proto, gfp_t prio, int kern)
>> + int proto, gfp_t prio, int kern,
>> + struct l2cap_chan *chan)
>> {
>> struct sock *sk;
>> - struct l2cap_chan *chan;
>>
>> sk = bt_sock_alloc(net, sock, &l2cap_proto, proto, prio, kern);
>> if (!sk)
>> @@ -1953,14 +1955,11 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
>>
>> INIT_LIST_HEAD(&l2cap_pi(sk)->rx_busy);
>>
>> - chan = l2cap_chan_create();
>> - if (!chan) {
>> - sk_free(sk);
>> - if (sock)
>> - sock->sk = NULL;
>> - return NULL;
>> - }
>> -
>> + /* The sock owns two refs on chan, matching the puts in
>> + * l2cap_sock_kill() and l2cap_sock_destruct(). The caller keeps
>> + * its own ref independent of these.
>> + */
>> + l2cap_chan_hold(chan);
>> l2cap_chan_hold(chan);
>>
>> l2cap_pi(sk)->chan = chan;
>> @@ -1972,6 +1971,7 @@ static int l2cap_sock_create(struct net *net, struct socket *sock, int protocol,
>> int kern)
>> {
>> struct sock *sk;
>> + struct l2cap_chan *chan;
>>
>> BT_DBG("sock %p", sock);
>>
>> @@ -1986,10 +1986,18 @@ static int l2cap_sock_create(struct net *net, struct socket *sock, int protocol,
>>
>> sock->ops = &l2cap_sock_ops;
>>
>> - sk = l2cap_sock_alloc(net, sock, protocol, GFP_ATOMIC, kern);
>> - if (!sk)
>> + chan = l2cap_chan_create();
>> + if (!chan)
>> return -ENOMEM;
>>
>> + sk = l2cap_sock_alloc(net, sock, protocol, GFP_ATOMIC, kern, chan);
>> + if (!sk) {
>> + l2cap_chan_put(chan);
>> + return -ENOMEM;
>> + }
>> + /* Sock has taken its own refs on chan; drop the chan_create() ref. */
>> + l2cap_chan_put(chan);
>> +
>> l2cap_sock_init(sk, NULL);
>> bt_sock_link(&l2cap_sk_list, sk);
>> return 0;
>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
>> index 1739c1989dbd..25cb5dc580bf 100644
>> --- a/net/bluetooth/smp.c
>> +++ b/net/bluetooth/smp.c
>> @@ -3204,16 +3204,11 @@ static const struct l2cap_ops smp_chan_ops = {
>> .get_sndtimeo = l2cap_chan_no_get_sndtimeo,
>> };
>>
>> -static inline struct l2cap_chan *smp_new_conn_cb(struct l2cap_chan *pchan)
>> +static inline int smp_new_conn_cb(struct l2cap_chan *pchan,
>> + struct l2cap_chan *chan)
>> {
>> - struct l2cap_chan *chan;
>> -
>> BT_DBG("pchan %p", pchan);
>>
>> - chan = l2cap_chan_create();
>> - if (!chan)
>> - return NULL;
>> -
>> chan->chan_type = pchan->chan_type;
>> chan->ops = &smp_chan_ops;
>> chan->scid = pchan->scid;
>> @@ -3229,9 +3224,9 @@ static inline struct l2cap_chan *smp_new_conn_cb(struct l2cap_chan *pchan)
>> */
>> atomic_set(&chan->nesting, L2CAP_NESTING_SMP);
>>
>> - BT_DBG("created chan %p", chan);
>> + BT_DBG("initialised chan %p", chan);
>>
>> - return chan;
>> + return 0;
>> }
>>
>> static const struct l2cap_ops smp_root_chan_ops = {
>> --
>> 2.54.0
>>
>
>
> --
> Luiz Augusto von Dentz
Best,
Siwei
next prev parent reply other threads:[~2026-05-27 4:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-20 16:20 [PATCH v5 0/1] Bluetooth: L2CAP: Fix use-after-free in l2cap_sock_new_connection_cb() Siwei Zhang
2026-05-20 16:20 ` [PATCH v5 1/1] " Siwei Zhang
2026-05-20 18:07 ` bluez.test.bot
2026-05-26 20:38 ` [PATCH v5 1/1] " Siwei Zhang
2026-05-26 20:46 ` Luiz Augusto von Dentz
2026-05-27 4:16 ` Siwei Zhang [this message]
2026-05-29 16:33 ` Siwei Zhang
2026-05-29 16:40 ` Luiz Augusto von Dentz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c45459c5-e7d9-4ea2-bd61-903a33f8778f@app.fastmail.com \
--to=oss@fourdim.xyz \
--cc=linux-bluetooth@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox