Re: [PATCH v2 1/1] Bluetooth: Fix parent socket UAF in accept queues

[zihan xi]

Hi Pauli,

On Thu, Sep 3, 2026 at 11:11 PM Pauli Virtanen <pav@xxxxxx> wrote:
>
> Hi,
>
> to, 2026-09-03 kello 11:11 +0000, Zihan Xi kirjoitti:
> > Bluetooth children queued on a listening socket store the listener in
> > bt_sk(sk)->parent, but the accept queue did not hold a reference on
> > that parent socket.  The child side can later fetch that pointer and
> > unlink itself from the accept queue while still needing to notify the
> > listener, for example from L2CAP, ISO or RFCOMM teardown/state-change
> > callbacks.
>
> Please revalidate the KASAN crash on current bluetooth-next/master,
> there have been related fixes since v1 of the patch and the v7.2-rc6
> shown in the KASAN crash in the cover letter.
>
> With commit d4bfa78fd679 ("Bluetooth: L2CAP: reject accept queue add
> unless BT_LISTEN") in v7.3-rc1 cherry-picked on v7.2-rc6 the POC no
> longer reproduces for me.
>

 Thanks for the review.

We revalidated this. On bluetooth-next with 9db7e5fffbae ("Bluetooth:
L2CAP: reject accept queue add unless BT_LISTEN") — the same change as
d4bfa78fd679 — our PoC no longer hits the l2cap_sock_ready_cb UAF.
That matches what you saw after cherry-picking d4bfa78fd679 onto
v7.2-rc6.

The KASAN report in the cover letter was captured on v7.2-rc6 after
reverting that commit. I should not have presented it as a crash on
current bluetooth-next/master.

> ***
>
> The design intent AFAICS is that the accept queue of the parent socket
> shall be empty when the parent socket is freed.
>
> Otherwise, the child sockets in accept queue would leak.
>
> There must then be parent->sk_state == BT_LISTEN check before
> bt_accept_enqueue() and some were missing in v7.2-rc6.
>
> bt_sk(sk)->parent read/write is guarded by lock_sock(sk), and it is set
> to NULL when removed from accept queue.
>
> Dangling bt_sk(sk)->parent should then not occur.
>
> If bt_sk(sk)->parent != NULL is observed under lock_sock(sk), the
> parent socket is valid during that critical section.
>
> The sock_hold/put(parent) in this patch are in lock_sock(sk) critical
> sections, so should be no-ops.
>
> The accept queue items owning reference to parent also should be no-
> ops.

Agreed. Once the BT_LISTEN check is in place, close() empties the
accept queue before the parent is freed, and lock_sock(sk) serializes
ready/teardown against unlink. The extra parent references do not
change the lifetime of the listener, so this patch is not needed on
current bluetooth-next.

I will drop this series.

Thanks,
Zihan Xi
>
> > If the listener is closed concurrently, removing the child from the
> > accept queue can drop the last listener reference before those
> > callbacks call parent->sk_data_ready(parent), leaving a stale parent
> > pointer and a use-after-free.
> >
> > Take a reference on the parent when a child is queued and drop it when
> > the child is unlinked.  Since unlinking now drops the accept-queue
> > parent reference, take a temporary parent reference in the callbacks
> > that continue to notify the parent after bt_accept_unlink().
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Vega <vega@xxxxxxxxxx>
> > Assisted-by: Codex:gpt-5.4
> > Signed-off-by: Zihan Xi <zihanx@xxxxxxxxxx>
> > ---
> > changes in v2:
> >   - rebase onto current bluetooth-next
> >   - refresh trailers to the current submission template
> >   - retarget author identity to Zihan Xi <zihanx@xxxxxxxxxx>
> >   - v1 Link: https://lore.kernel.org/all/65767989c644f8adf52f35334f4034c66f47881f.1784383243.git.xizh2024@xxxxxxxxxx/
> >
> >  net/bluetooth/af_bluetooth.c | 2 ++
> >  net/bluetooth/iso.c          | 2 ++
> >  net/bluetooth/l2cap_sock.c   | 2 ++
> >  net/bluetooth/rfcomm/sock.c  | 2 ++
> >  4 files changed, 8 insertions(+)
> >
> > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> > index 411d66f24393..61a232378e6c 100644
> > --- a/net/bluetooth/af_bluetooth.c
> > +++ b/net/bluetooth/af_bluetooth.c
> > @@ -218,6 +218,7 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
> >       BT_DBG("parent %p, sk %p", parent, sk);
> >
> >       sock_hold(sk);
> > +     sock_hold(parent);
> >
> >       if (bh)
> >               bh_lock_sock_nested(sk);
> > @@ -266,6 +267,7 @@ void bt_accept_unlink(struct sock *sk)
> >       spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
> >       bt_sk(sk)->parent = NULL;
> >       sock_put(sk);
> > +     sock_put(parent);
> >  }
> >  EXPORT_SYMBOL(bt_accept_unlink);
> >
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 75bfd5938b2e..e709292aa112 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -286,8 +286,10 @@ static void iso_chan_del(struct sock *sk, int err)
> >
> >       parent = bt_sk(sk)->parent;
> >       if (parent) {
> > +             sock_hold(parent);
> >               bt_accept_unlink(sk);
> >               parent->sk_data_ready(parent);
> > +             sock_put(parent);
> >       } else {
> >               sk->sk_state_change(sk);
> >       }
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index b553b6356af8..3720ab2e39ed 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -1748,8 +1748,10 @@ static void l2cap_sock_teardown_cb(struct l2cap_chan *chan, int err)
> >               sk->sk_err = err;
> >
> >               if (parent) {
> > +                     sock_hold(parent);
> >                       bt_accept_unlink(sk);
> >                       parent->sk_data_ready(parent);
> > +                     sock_put(parent);
> >               } else {
> >                       sk->sk_state_change(sk);
> >               }
> > diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> > index 958081adb9b5..a16daa68ecb8 100644
> > --- a/net/bluetooth/rfcomm/sock.c
> > +++ b/net/bluetooth/rfcomm/sock.c
> > @@ -78,11 +78,13 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
> >
> >       parent = bt_sk(sk)->parent;
> >       if (parent) {
> > +             sock_hold(parent);
> >               if (d->state == BT_CLOSED) {
> >                       sock_set_flag(sk, SOCK_ZAPPED);
> >                       bt_accept_unlink(sk);
> >               }
> >               parent->sk_data_ready(parent);
> > +             sock_put(parent);
> >       } else {
> >               if (d->state == BT_CONNECTED)
> >                       rfcomm_session_getaddr(d->session,
>
> --
> Pauli Virtanen