Re: [PATCH v2] Bluetooth: ISO: fix UAF on socket close before shutdown completes

Hi Pauli,

On Mon, Jul 20, 2026 at 4:08 PM Pauli Virtanen <[email protected]> wrote:
>
> Hi Luiz,
>
> ma, 2026-07-20 kello 15:37 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Mon, Jul 20, 2026 at 2:30 PM Pauli Virtanen <[email protected]> wrote:
> > >
> > > iso_sock_disconn() aims to disconnect the hcon by dropping it, which
> > > triggers iso_conn_del() once the HCI operation completes, requiring
> > > valid hcon->iso_data to do socket cleanup.  iso_sock_disconn() sets
> > > conn->hcon = NULL to avoid a second drop, but also preventing clearing
> > > hcon->iso_data on socket release. Closing the socket before
> > > iso_conn_del() runs then results to UAF.
> > >
> > > Fix by using a separate flag to track the hcon drop status, instead of
> > > clearing conn->hcon.
> > >
> > > Log: (BlueZ iso-tester ISO Connect Close - Success)
> > > BUG: KASAN: slab-use-after-free in iso_conn_hold_unless_zero
> > > ...
> > >  iso_conn_hold_unless_zero (net/bluetooth/iso.c:138)
> > >  iso_conn_del (net/bluetooth/iso.c:270)
> > >  hci_conn_failed (net/bluetooth/hci_conn.c:1408)
> > >  hci_abort_conn_sync (net/bluetooth/hci_sync.c:5817)
> > >
> > > Allocated by task 34:
> > >  iso_conn_add (net/bluetooth/iso.c:216)
> > >  iso_connect_cis (net/bluetooth/iso.c:507)
> > >  iso_sock_connect (net/bluetooth/iso.c:1211)
> > >  __sys_connect (net/socket.c:2148)
> > >
> > > Freed by task 34:
> > >  iso_chan_del (net/bluetooth/iso.c:248)
> > >  iso_sock_close (net/bluetooth/iso.c:885)
> > >  iso_sock_release (net/bluetooth/iso.c:2022)
> > >  sock_close (net/socket.c:722)
> > >
> > > Fixes: fbdc4bc47268 ("Bluetooth: ISO: Use defer setup to separate PA sync and BIG sync")
> > > Signed-off-by: Pauli Virtanen <[email protected]>
> > > ---
> > >  net/bluetooth/iso.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > index 2e95a153912c..6abc2b1f59bc 100644
> > > --- a/net/bluetooth/iso.c
> > > +++ b/net/bluetooth/iso.c
> > > @@ -30,6 +30,7 @@ struct iso_conn {
> > >         /* @lock: spinlock protecting changes to iso_conn fields */
> > >         spinlock_t      lock;
> > >         struct sock     *sk;
> > > +       bool            hcon_dropped;
> >
> > It might be safer to add a flag or something so we can perform atomic
> > operations to check if dropping is necessary.
>
> It is serialized by iso_conn::lock + kref, so AFAICS it is safe. I can
> change it to test_and_set_bit() regardless if still preferable.

I'm afraid in today's world that is sort of required; otherwise,
someone using an AI model might miss the serialization and send
patches to 'fix' it anyway, just creating more work for us to review.

> The lock is not taken in iso_conn_free() --- at that point nobody is
> holding a reference so there should be no concurrent modification, so
> AFAIK it is taken care of by the refcount memory ordering (see comment
> in include/linux/refcount.h), otherwise conn->hcon etc would also be
> wrong.
>
> The
> comment https://sashiko.dev/#/patchset/4d96c545ad1a2fed02440a3478905853286aa0c7.1784571683.git.pav%40iki.fi
> on Sashiko seems to miss that iso_connect_cfm / iso_disconn_cfm ->
> iso_conn_del() are called before hci_conn is deleted, and should make
> sure there's no dangling reference to the hci_conn. It would be same as
> for remote disconnect. Maybe the prompts it uses in
> https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/bluetooth.md
> could be improved vs hci_conn and socket life cycles.
>
> > >         struct delayed_work     timeout_work;
> > >
> > > @@ -107,7 +108,8 @@ static void iso_conn_free(struct kref *ref)
> > >
> > >         if (conn->hcon) {
> > >                 conn->hcon->iso_data = NULL;
> > > -               hci_conn_drop(conn->hcon);
> > > +               if (!conn->hcon_dropped)
> > > +                       hci_conn_drop(conn->hcon);
> > >         }
> > >
> > >         /* Ensure no more work items will run since hci_conn has been dropped */
> > > @@ -306,6 +308,7 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
> > >
> > >         iso_pi(sk)->conn = conn;
> > >         conn->sk = sk;
> > > +       conn->hcon_dropped = false;
> > >
> > >         if (parent)
> > >                 bt_accept_enqueue(parent, sk, true);
> > > @@ -836,8 +839,10 @@ static void iso_sock_disconn(struct sock *sk)
> > >
> > >         sk->sk_state = BT_DISCONN;
> > >         iso_conn_lock(iso_pi(sk)->conn);
> > > -       hci_conn_drop(iso_pi(sk)->conn->hcon);
> > > -       iso_pi(sk)->conn->hcon = NULL;
> > > +       if (!iso_pi(sk)->conn->hcon_dropped) {
> > > +               iso_pi(sk)->conn->hcon_dropped = true;
> > > +               hci_conn_drop(iso_pi(sk)->conn->hcon);
> > > +       }
> > >         iso_conn_unlock(iso_pi(sk)->conn);
> > >  }
> > >
> > > --
> > > 2.55.0
> > >
> >



-- 
Luiz Augusto von Dentz