1. Patchew
  2. linux
  3. View series

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

David Carlier posted 1 patch 2 months ago
There is a newer version of this series
net/bluetooth/hci_conn.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
[PATCH v2 net-next] Bluetooth: hci_conn: fix potential UAF in create_big_sync
Posted by David Carlier 2 months ago 
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().

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@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---

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@gmail.com
 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;
+
 	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;
+
+	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,
-- 
2.53.0
Re: [PATCH v2 net-next] Bluetooth: hci_conn: fix potential UAF in create_big_sync
Posted by Paul Menzel 2 months ago 
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@vger.kernel.org
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> 
> 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@gmail.com
>   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
Re: [PATCH v2 net-next] Bluetooth: hci_conn: fix potential UAF in create_big_sync
Posted by David CARLIER 2 months ago 
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@molgen.mpg.de> 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@vger.kernel.org
> > Signed-off-by: David Carlier <devnexen@gmail.com>
> > ---
> >
> > 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@gmail.com
> >   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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.