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

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.

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