All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH BlueZ] media: fix crash when clearing BAP transport
@ 2023-02-13 21:24 Pauli Virtanen
  2023-02-13 22:08 ` Luiz Augusto von Dentz
  2023-02-13 22:35 ` [BlueZ] " bluez.test.bot
  0 siblings, 2 replies; 3+ messages in thread
From: Pauli Virtanen @ 2023-02-13 21:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

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.

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH BlueZ] media: fix crash when clearing BAP transport
  2023-02-13 21:24 [PATCH BlueZ] media: fix crash when clearing BAP transport Pauli Virtanen
@ 2023-02-13 22:08 ` Luiz Augusto von Dentz
  2023-02-13 22:35 ` [BlueZ] " bluez.test.bot
  1 sibling, 0 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2023-02-13 22:08 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hi Pauli,

On Mon, Feb 13, 2023 at 1:28 PM Pauli Virtanen <pav@iki.fi> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [BlueZ] media: fix crash when clearing BAP transport
  2023-02-13 21:24 [PATCH BlueZ] media: fix crash when clearing BAP transport Pauli Virtanen
  2023-02-13 22:08 ` Luiz Augusto von Dentz
@ 2023-02-13 22:35 ` bluez.test.bot
  1 sibling, 0 replies; 3+ messages in thread
From: bluez.test.bot @ 2023-02-13 22:35 UTC (permalink / raw)
  To: linux-bluetooth, pav

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=721448

---Test result---

Test Summary:
CheckPatch                    PASS      0.43 seconds
GitLint                       PASS      0.26 seconds
BuildEll                      PASS      27.10 seconds
BluezMake                     PASS      868.57 seconds
MakeCheck                     PASS      11.43 seconds
MakeDistcheck                 PASS      148.31 seconds
CheckValgrind                 PASS      243.86 seconds
CheckSmatch                   PASS      325.37 seconds
bluezmakeextell               PASS      97.78 seconds
IncrementalBuild              PASS      713.08 seconds
ScanBuild                     PASS      1006.56 seconds



---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-02-13 22:35 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-13 21:24 [PATCH BlueZ] media: fix crash when clearing BAP transport Pauli Virtanen
2023-02-13 22:08 ` Luiz Augusto von Dentz
2023-02-13 22:35 ` [BlueZ] " bluez.test.bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.