Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2761-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <afc30fd4d201bdd92ffbc682ac66f472ce837814.1711186824.git.pav@iki.fi>
 <ad76621e8c2d414989b4eb384cb9077ca55cb16d.1711186824.git.pav@iki.fi>
In-Reply-To: <ad76621e8c2d414989b4eb384cb9077ca55cb16d.1711186824.git.pav@iki.fi>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 25 Mar 2024 13:29:03 -0400
Message-ID: <CABBYNZJj2wt-xEu6_ZPz=uUHwy8s1eJSS85tWuc9_8V8xXnoxQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 2/2] bap: cancel ongoing SelectProperties()
 before freeing the ep
To: Pauli Virtanen <pav@iki.fi>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Pauli,

On Sat, Mar 23, 2024 at 5:40=E2=80=AFAM Pauli Virtanen <pav@iki.fi> wrote:
>
> select_cb() callback is called when the sound server replies. However,
> at that point the ep or session for which it was made may already be
> gone if e.g. device disconnects or adapter is powered off.
>
> Fix by implementing cancelling select() callbacks, and doing it before
> freeing ep.
>
> Fixes crash:
>
> =3D=3D889897=3D=3DERROR: AddressSanitizer: heap-use-after-free
> READ of size 8 at 0x60400006b098 thread T0
>     #0 0x55aeba in setup_new profiles/audio/bap.c:840
>     #1 0x562158 in select_cb profiles/audio/bap.c:1361
>     #2 0x47ad66 in pac_select_cb profiles/audio/media.c:920
>     #3 0x47661b in endpoint_reply profiles/audio/media.c:375
>     ...
> freed by thread T0 here:
>     #0 0x7fd20bcd7fb8 in __interceptor_free.part.0
>     #1 0x55f913 in ep_free profiles/audio/bap.c:1156
>     #2 0x7d696e in remove_interface gdbus/object.c:660
>     #3 0x7de622 in g_dbus_unregister_interface gdbus/object.c:1394
>     #4 0x554536 in ep_unregister profiles/audio/bap.c:193
>     #5 0x574455 in ep_remove profiles/audio/bap.c:2963
>     #6 0x7f5341 in queue_remove_if src/shared/queue.c:279
>     #7 0x7f5aba in queue_remove_all src/shared/queue.c:321
>     #8 0x57452b in bap_disconnect profiles/audio/bap.c:2972
>     #9 0x6cd107 in btd_service_disconnect src/service.c:305
>     ...
> previously allocated by thread T0 here:
>     #0 0x7fd20bcd92ef in malloc
>     #1 0x7f6e98 in util_malloc src/shared/util.c:46
>     #2 0x560d28 in ep_register profiles/audio/bap.c:1282
>     #3 0x562bdf in pac_register profiles/audio/bap.c:1386
>     #4 0x8cc834 in bap_foreach_pac src/shared/bap.c:4950
>     #5 0x8cccfc in bt_bap_foreach_pac src/shared/bap.c:4964
>     #6 0x56330b in bap_ready profiles/audio/bap.c:1457
>     ...
> ---
>  profiles/audio/bap.c   | 22 ++++++++++++++++++++++
>  profiles/audio/media.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index 315eff729..fdce275c9 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -1148,11 +1148,15 @@ static const GDBusMethodTable ep_methods[] =3D {
>         { },
>  };
>
> +static void ep_cancel_select(struct bap_ep *ep);
> +
>  static void ep_free(void *data)
>  {
>         struct bap_ep *ep =3D data;
>         struct queue *setups =3D ep->setups;
>
> +       ep_cancel_select(ep);
> +
>         ep->setups =3D NULL;
>         queue_destroy(setups, setup_free);
>         free(ep->path);
> @@ -1426,6 +1430,24 @@ static bool pac_select(struct bt_bap_pac *lpac, st=
ruct bt_bap_pac *rpac,
>         return true;
>  }
>
> +static bool pac_cancel_select(struct bt_bap_pac *lpac, struct bt_bap_pac=
 *rpac,
> +                                                       void *user_data)
> +{
> +       struct bap_ep *ep =3D user_data;
> +
> +       bt_bap_cancel_select(lpac, select_cb, ep);
> +
> +       return true;
> +}
> +
> +static void ep_cancel_select(struct bap_ep *ep)
> +{
> +       struct bt_bap *bap =3D ep->data->bap;
> +
> +       bt_bap_foreach_pac(bap, BT_BAP_SOURCE, pac_cancel_select, ep);
> +       bt_bap_foreach_pac(bap, BT_BAP_SINK, pac_cancel_select, ep);
> +}
> +
>  static bool pac_found_bcast(struct bt_bap_pac *lpac, struct bt_bap_pac *=
rpac,
>                                                         void *user_data)
>  {
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index edaff7867..3c5daf782 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -149,6 +149,11 @@ static void media_endpoint_cancel(struct endpoint_re=
quest *request)
>  {
>         struct media_endpoint *endpoint =3D request->endpoint;
>
> +       DBG("Canceling %s: name =3D %s path =3D %s",
> +                       dbus_message_get_member(request->msg),
> +                       dbus_message_get_destination(request->msg),
> +                       dbus_message_get_path(request->msg));
> +
>         if (request->call)
>                 dbus_pending_call_cancel(request->call);
>
> @@ -1028,6 +1033,30 @@ static int pac_select(struct bt_bap_pac *lpac, str=
uct bt_bap_pac *rpac,
>                                                                 data, fre=
e);
>  }
>
> +static void pac_cancel_select(struct bt_bap_pac *lpac, bt_bap_pac_select=
_t cb,
> +                                               void *cb_data, void *user=
_data)
> +{
> +       struct media_endpoint *endpoint =3D user_data;
> +       GSList *l;
> +
> +       for (l =3D endpoint->requests; l; l =3D g_slist_next(l)) {
> +               struct endpoint_request *req =3D l->data;
> +               struct pac_select_data *data;
> +
> +               if (req->cb !=3D pac_select_cb)
> +                       continue;
> +
> +               data =3D req->user_data;
> +               if (data->pac !=3D lpac || data->cb !=3D cb ||
> +                                               data->user_data !=3D cb_d=
ata)
> +                       continue;
> +
> +               req->cb =3D NULL;
> +               media_endpoint_cancel(req);
> +               l =3D endpoint->requests;

We probably want to do the l =3D g_gslist_next assignment inside the for
loop to guarantee the requests list in not freed in the process.

> +       }
> +}
> +
>  struct pac_config_data {
>         struct bt_bap_stream *stream;
>         bt_bap_pac_config_t cb;
> @@ -1195,6 +1224,7 @@ static void pac_clear(struct bt_bap_stream *stream,=
 void *user_data)
>
>  static struct bt_bap_pac_ops pac_ops =3D {
>         .select =3D pac_select,
> +       .cancel_select =3D pac_cancel_select,
>         .config =3D pac_config,
>         .clear =3D pac_clear,
>  };
> --
> 2.44.0
>
>


--=20
Luiz Augusto von Dentz