Re: [PATCH v2 net-next] Bluetooth: hci_conn: fix potential UAF in create_big_sync

[David CARLIER]

Hi Paul,

  Thanks a lot for taking the time to look at this.

  > (Using 75 characters per line would save a line.)

  Good catch, I'll rewrap it in v3.

  > I wonder if a debug message about the stale connection would be
useful.

  Yes, that makes sense — I'll add a bt_dev_dbg() right before the
  -ECANCELED return so it's visible when debugging.

  > gemini/gemini-3.1-pro-preview comments [1]:
  > > Could this introduce a time-of-check to time-of-use race
condition?
  > [ ... ]

  I had the same reflex when I first looked at it, but I think the tool
  is reading the check in isolation. The hci_conn_valid() at the top
of
  a *_sync callback is really just a "did this connection get torn down
  before the work ran?" guard — it's the same idiom already used by
  hci_le_create_conn_sync(), hci_le_pa_create_sync() and
  hci_le_big_create_sync() in hci_sync.c. It was never meant to fully
  serialize against a concurrent hci_conn_del().

  The piece that actually closes the UAF in v2 is in
  create_big_complete(): it now takes hci_dev_lock() and re-validates
  conn before dereferencing it, which mirrors what
create_pa_complete()
  does. That's the part doing the real work here.

  If there's a genuine strict-TOCTOU window between the valid check and
  the &conn->iso_qos access inside create_big_sync(), it would equally
  affect every other *_sync user of the same idiom, so I'd rather not
  try to rework that in this fix — happy to revisit it separately if
  you think it's worth digging into.

  > > + if (err == -ECANCELED)
  > > +         return;
  >
  > Should the error message still be printed in this case?

  I went back and forth on that one. In the end I kept it silent to
  stay consistent with create_pa_complete() in hci_sync.c, which also
  just returns on -ECANCELED without logging. Happy to add a print if
  you'd rather have one, though.

  I'll send a v3 with the rewrap and the debug message in a timely manner.

Cheers !

On Fri, 10 Apr 2026 at 21:25, Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
>
> Dear David,
>
>
> Thank you for the patch.
>
> Am 10.04.26 um 19:34 schrieb David Carlier:
> > Add hci_conn_valid() check in create_big_sync() to detect stale
> > connections before proceeding with BIG creation. Fix
> > create_big_complete() to handle the resulting -ECANCELED error
> > and validate the connection under hci_dev_lock() before
> > dereferencing, following the established pattern used by
> > create_le_conn_complete() and create_pa_complete().
>
> (Using 75 characters per line would save a line.)
>
> > Without this, create_big_complete() would unconditionally
> > dereference the stale conn pointer on error, causing a
> > use-after-free via hci_connect_cfm() and hci_conn_del().
> >
> > Fixes: eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> > ---
> >
> > v1 -> v2: fix create_big_complete() to handle -ECANCELED and
> >    validate conn under hci_dev_lock(), matching the pattern in
> >    create_le_conn_complete() and create_pa_complete().
> > v1: https://lore.kernel.org/r/20260408155638.95927-1-devnexen@xxxxxxxxx
> >   net/bluetooth/hci_conn.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 11d3ad8d2551..feebe933efc8 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -2130,6 +2130,9 @@ static int create_big_sync(struct hci_dev *hdev, void *data)
> >       u32 flags = 0;
> >       int err;
> >
> > +     if (!hci_conn_valid(hdev, conn))
> > +             return -ECANCELED;
> > +
>
> I wonder if a debug message about the stale connection would be useful.
>
> gemini/gemini-3.1-pro-preview comments [1]:
>
> > Could this introduce a time-of-check to time-of-use race condition?
> > Because create_big_sync() executes asynchronously in the cmd_sync_work
> > workqueue without holding hci_dev_lock(), hci_conn_valid() only protects
> > the lookup for the duration of its internal rcu_read_lock().
> > If a concurrent thread acquires hci_dev_lock() and calls hci_conn_del()
> > immediately after hci_conn_valid() returns true:
> > cpu 1
> > create_big_sync()
> >     if (!hci_conn_valid(hdev, conn))
> >         return -ECANCELED;
> > cpu 2
> > hci_dev_lock(hdev);
> > hci_conn_del(conn);
> > hci_dev_unlock(hdev);
> > cpu 1
> >     if (qos->bcast.out.phys == BIT(1))
> > Will this lead to a use-after-free on the conn pointer since qos resolves
> > to &conn->iso_qos?
> > Additionally, since conn is passed to the workqueue without holding a
> > reference via hci_conn_get(), could this be susceptible to a pointer reuse
> > problem?
> > If the original connection is freed and the memory is reallocated for a
> > new, unrelated connection before the work runs, hci_conn_valid() might
> > incorrectly return true. This could cause the new connection to be wrongly
> > operated on or deleted later in create_big_complete().
> > Does the caller queueing the work need to take a proper reference with
> > hci_conn_get() and release it in the completion callback instead?
> > [ ... ]
>
>
>
> >       if (qos->bcast.out.phys == BIT(1))
> >               flags |= MGMT_ADV_FLAG_SEC_2M;
> >
> > @@ -2204,11 +2207,22 @@ static void create_big_complete(struct hci_dev *hdev, void *data, int err)
> >
> >       bt_dev_dbg(hdev, "conn %p", conn);
> >
> > +     if (err == -ECANCELED)
> > +             return;
>
> Should the error message still be printed in this case?
>
>      bt_dev_err(hdev, "Unable to create BIG: ECANCELED");
>
> > +
> > +     hci_dev_lock(hdev);
> > +
> > +     if (!hci_conn_valid(hdev, conn))
> > +             goto done;
> > +
> >       if (err) {
> >               bt_dev_err(hdev, "Unable to create BIG: %d", err);
> >               hci_connect_cfm(conn, err);
> >               hci_conn_del(conn);
> >       }
> > +
> > +done:
> > +     hci_dev_unlock(hdev);
> >   }
> >
> >   struct hci_conn *hci_bind_bis(struct hci_dev *hdev, bdaddr_t *dst, __u8 sid,
>
>
> Kind regards,
>
> Paul
>
>
> [1]:
> https://sashiko.dev/#/patchset/20260410173451.4797-1-devnexen%40gmail.com