Re: [PATCH BlueZ] media: fix crash when clearing BAP transport

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Pauli,

On Mon, Feb 13, 2023 at 1:28 PM Pauli Virtanen <pav@xxxxxx> wrote:
>
> The BAP stream user data used for transport paths in media.c is set both
> in media.c:pac_config and in profiles/bap.c. In the latter, it gets set
> to a string owned by bap_ep, whose lifetime can be shorter than that of
> the transport.
>
> Under some conditions, bap.c:bap_disconnect is hit while there are
> transports and the user data is owned by the endpoint. In this case, the
> path string owned by the endpoints gets freed first, and ASAN
> use-after-free crash is encountered when clearing the transports.

Hmm, I didn't hit while testing, anyway that means the transport is
being freed before we clean up? It might be a good idea to run this
under valgrind to check where the path is freed.

> Fix this by matching the transports by the stream pointer, and not by
> the transport/endpoint path.
>
> Fixes: 7b1b1a499cf3 ("media: clear the right transport when clearing BAP endpoint")
> ---
>
> Notes:
>     The v2 version of the patch in commit 7b1b1a499cf334 was buggy, and its
>     v3 sent to the list was supposed to replace it. However, I resubmitted
>     only that patch and not the full series, which maybe would have been the
>     right thing.  Sorry for the mess.
>
>  profiles/audio/media.c     | 21 +++++++++++----------
>  profiles/audio/transport.c | 14 ++++++++++++++
>  profiles/audio/transport.h |  1 +
>  3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 3eb038cb7..8728b69e0 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1085,19 +1085,20 @@ static int pac_config(struct bt_bap_stream *stream, struct iovec *cfg,
>  static void pac_clear(struct bt_bap_stream *stream, void *user_data)
>  {
>         struct media_endpoint *endpoint = user_data;
> -       struct media_transport *transport;
> -       const char *path;
> +       GSList *item;
>
> -       path = bt_bap_stream_get_user_data(stream);
> -       if (!path)
> -               return;
> +       DBG("endpoint %p stream %p", endpoint, stream);
>
> -       DBG("endpoint %p path %s", endpoint, path);
> +       item = endpoint->transports;
> +       while (item) {
> +               struct media_transport *transport = item->data;
>
> -       transport = find_transport(endpoint, path);
> -       if (transport) {
> -               clear_configuration(endpoint, transport);
> -               bt_bap_stream_set_user_data(stream, NULL);
> +               if (media_transport_get_stream(transport) == stream) {
> +                       clear_configuration(endpoint, transport);
> +                       item = endpoint->transports;
> +               } else {
> +                       item = item->next;
> +               }
>         }
>  }
>
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index 5e057e2a5..cd91669c6 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -1483,6 +1483,20 @@ const char *media_transport_get_path(struct media_transport *transport)
>         return transport->path;
>  }
>
> +void *media_transport_get_stream(struct media_transport *transport)
> +{
> +       struct bap_transport *bap;
> +       const char *uuid;
> +
> +       uuid = media_endpoint_get_uuid(transport->endpoint);
> +       if (strcasecmp(uuid, PAC_SINK_UUID) &&
> +                       strcasecmp(uuid, PAC_SOURCE_UUID))
> +               return NULL;

This should probably be made generic, perhaps with a get_stream
callback so we don't have to check the UUID like above.

> +
> +       bap = transport->data;
> +       return bap->stream;
> +}
> +
>  void media_transport_update_delay(struct media_transport *transport,
>                                                         uint16_t delay)
>  {
> diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
> index 102fc3cf1..5ca9b8f9e 100644
> --- a/profiles/audio/transport.h
> +++ b/profiles/audio/transport.h
> @@ -19,6 +19,7 @@ struct media_transport *media_transport_create(struct btd_device *device,
>
>  void media_transport_destroy(struct media_transport *transport);
>  const char *media_transport_get_path(struct media_transport *transport);
> +void *media_transport_get_stream(struct media_transport *transport);
>  struct btd_device *media_transport_get_dev(struct media_transport *transport);
>  int8_t media_transport_get_volume(struct media_transport *transport);
>  void media_transport_update_delay(struct media_transport *transport,
> --
> 2.39.1
>


-- 
Luiz Augusto von Dentz



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux