diff mbox series

[v3,1/2] transport: Update transport release flow for bcast src

Message ID 20231004075646.4277-2-silviu.barbulescu@nxp.com
State Superseded
Headers show
Series Update transport acquire/release flow BAP bcast source | expand

Commit Message

Silviu Florian Barbulescu Oct. 4, 2023, 7:56 a.m. UTC 
Update transport release flow for broadcast source

---
 profiles/audio/transport.c | 65 ++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 27 deletions(-)

Comments

Jonas Dreßler Oct. 29, 2023, 9:49 p.m. UTC | #1 
Hi Silviu,

this patch introduced a use-after-free, reproducer:

- connect to a2dp sink
- start playing
- pause and wait until stream suspends
- bluetoothd crashes in a2dp_suspend_complete()

Here's the output running with address sanitizer:

bluetoothd[181120]: profiles/audio/a2dp.c:suspend_cfm() Source 
0x60600001e500: Suspend_Cfm
=================================================================
==181120==ERROR: AddressSanitizer: heap-use-after-free on address 
0x60300005a730 at pc 0xaaaaf9dbeea8 bp 0xfffff4d3b690 sp 0xfffff4d3b6a8
READ of size 8 at 0x60300005a730 thread T0
     #0 0xaaaaf9dbeea4 in a2dp_suspend_complete 
profiles/audio/transport.c:426
     #1 0xaaaaf9d7d37c in finalize_suspend profiles/audio/a2dp.c:376
     #2 0xaaaaf9d7e060 in suspend_cfm profiles/audio/a2dp.c:1276
     #3 0xaaaaf9da0ddc in avdtp_delay_report_resp 
profiles/audio/avdtp.c:2928
     #4 0xaaaaf9da0ddc in avdtp_parse_resp profiles/audio/avdtp.c:2997
     #5 0xaaaaf9da0ddc in session_cb profiles/audio/avdtp.c:2286
     #6 0xffff6e77030c in g_main_dispatch ../glib/gmain.c:3476
     #7 0xffff6e77030c in g_main_context_dispatch_unlocked 
../glib/gmain.c:4284
     #8 0xffff6e7ce598 in g_main_context_iterate_unlocked.isra.0 
../glib/gmain.c:4349
     #9 0xffff6e771a60 in g_main_loop_run 
(/lib64/libglib-2.0.so.0+0x61a60) (BuildId: 
7d17ee836a99abf35136ebb46126a95dee66511a)
     #10 0xaaaafa0ad0d8 in mainloop_run src/shared/mainloop-glib.c:66
     #11 0xaaaafa0ad920 in mainloop_run_with_signal 
src/shared/mainloop-notify.c:188
     #12 0xaaaaf9d5489c in main src/main.c:1452
     #13 0xffff6dd209d8 in __libc_start_call_main 
../sysdeps/nptl/libc_start_call_main.h:58
     #14 0xffff6dd20aac in __libc_start_main_impl ../csu/libc-start.c:360
     #15 0xaaaaf9d5686c in _start 
(/home/jonas/git/bluez/src/bluetoothd+0x55686c) (BuildId: 
8922b026a55aac729ed13de54b3a622a31b5bb2b)

0x60300005a730 is located 0 bytes inside of 32-byte region 
[0x60300005a730,0x60300005a750)
freed by thread T0 here:
     #0 0xffff6ea24638 in __interceptor_free.part.0 
(/lib64/libasan.so.8+0xc4638) (BuildId: 
a77e9fa1e1448d41e9a8ddaf28aa5612f3ffc397)
     #1 0xffff6e773114 in g_free (/lib64/libglib-2.0.so.0+0x63114) 
(BuildId: 7d17ee836a99abf35136ebb46126a95dee66511a)
     #2 0xaaaaf9dbc42c in media_transport_remove_owner 
profiles/audio/transport.c:322
     #3 0xaaaaf9dc0b64 in bap_disable_complete 
profiles/audio/transport.c:632
     #4 0xaaaaf9dc0b64 in release profiles/audio/transport.c:674
     #5 0xaaaaf9ff2574 in process_message gdbus/object.c:246
     #6 0xffff6e69de78 in _dbus_object_tree_dispatch_and_unlock 
../../dbus/dbus-object-tree.c:1021
     #7 0xaaaaf9fe44a4 in message_dispatch gdbus/mainloop.c:59
     #8 0xffff6e76c444 in g_idle_dispatch ../glib/gmain.c:6282
     #9 0xffff6e77030c in g_main_dispatch ../glib/gmain.c:3476
     #10 0xffff6e77030c in g_main_context_dispatch_unlocked 
../glib/gmain.c:4284
     #11 0xffff6e7ce598 in g_main_context_iterate_unlocked.isra.0 
../glib/gmain.c:4349
     #12 0xffff6e771a60 in g_main_loop_run 
(/lib64/libglib-2.0.so.0+0x61a60) (BuildId: 
7d17ee836a99abf35136ebb46126a95dee66511a)
     #13 0xaaaafa0ad0d8 in mainloop_run src/shared/mainloop-glib.c:66
     #14 0xaaaafa0ad920 in mainloop_run_with_signal 
src/shared/mainloop-notify.c:188
     #15 0xaaaaf9d5489c in main src/main.c:1452
     #16 0xffff6dd209d8 in __libc_start_call_main 
../sysdeps/nptl/libc_start_call_main.h:58
     #17 0xffff6dd20aac in __libc_start_main_impl ../csu/libc-start.c:360
     #18 0xaaaaf9d5686c in _start 
(/home/jonas/git/bluez/src/bluetoothd+0x55686c) (BuildId: 
8922b026a55aac729ed13de54b3a622a31b5bb2b)

previously allocated by thread T0 here:
     #0 0xffff6ea25218 in calloc (/lib64/libasan.so.8+0xc5218) (BuildId: 
a77e9fa1e1448d41e9a8ddaf28aa5612f3ffc397)
     #1 0xffff6e7769e4 in g_malloc0 (/lib64/libglib-2.0.so.0+0x669e4) 
(BuildId: 7d17ee836a99abf35136ebb46126a95dee66511a)
     #2 0xaaaaf9db89c4 in media_owner_create profiles/audio/transport.c:514
     #3 0xaaaaf9dbd760 in acquire profiles/audio/transport.c:552
     #4 0xaaaaf9dbd760 in acquire profiles/audio/transport.c:538
     #5 0xaaaaf9ff2574 in process_message gdbus/object.c:246
     #6 0xffff6e69de78 in _dbus_object_tree_dispatch_and_unlock 
../../dbus/dbus-object-tree.c:1021
     #7 0xaaaaf9fe44a4 in message_dispatch gdbus/mainloop.c:59
     #8 0xffff6e76c444 in g_idle_dispatch ../glib/gmain.c:6282
     #9 0xffff6e77030c in g_main_dispatch ../glib/gmain.c:3476
     #10 0xffff6e77030c in g_main_context_dispatch_unlocked 
../glib/gmain.c:4284
     #11 0xffff6e7ce598 in g_main_context_iterate_unlocked.isra.0 
../glib/gmain.c:4349
     #12 0xffff6e771a60 in g_main_loop_run 
(/lib64/libglib-2.0.so.0+0x61a60) (BuildId: 
7d17ee836a99abf35136ebb46126a95dee66511a)
     #13 0xaaaafa0ad0d8 in mainloop_run src/shared/mainloop-glib.c:66
     #14 0xaaaafa0ad920 in mainloop_run_with_signal 
src/shared/mainloop-notify.c:188
     #15 0xaaaaf9d5489c in main src/main.c:1452
     #16 0xffff6dd209d8 in __libc_start_call_main 
../sysdeps/nptl/libc_start_call_main.h:58
     #17 0xffff6dd20aac in __libc_start_main_impl ../csu/libc-start.c:360
     #18 0xaaaaf9d5686c in _start 
(/home/jonas/git/bluez/src/bluetoothd+0x55686c) (BuildId: 
8922b026a55aac729ed13de54b3a622a31b5bb2b)

SUMMARY: AddressSanitizer: heap-use-after-free 
profiles/audio/transport.c:426 in a2dp_suspend_complete
Ronan Pigott Dec. 16, 2023, 12:29 a.m. UTC | #2 
Hi Silviu,

I encountered a use-after-free in the 5.71 release of Bluez which I
bisected to this patch. The backtrace is below:

#0  0x000055e93dbc8785 in a2dp_suspend_complete (session=<optimized out>, err=0, user_data=0x55e93e432520) at profiles/audio/transport.c:431
#1  0x000055e93dbb97ea in finalize_suspend (data=data@entry=0x55e93e435880) at profiles/audio/a2dp.c:376
#2  0x000055e93dbb98c0 in suspend_cfm (session=0x55e93e4317b0, sep=<optimized out>, stream=<optimized out>, err=0x0, user_data=0x55e93e41e850) at profiles/audio/a2dp.c:1276
#3  0x000055e93dbbfa4b in avdtp_suspend_resp (data=0x55e93e431823, size=<optimized out>, stream=0x55e93e433e60, session=0x55e93e4317b0) at profiles/audio/avdtp.c:2900
#4  avdtp_parse_resp (transaction=<optimized out>, size=<optimized out>, buf=0x55e93e431823, signal_id=<optimized out>, stream=0x55e93e433e60, session=0x55e93e4317b0) at profiles/audio/avdtp.c:2985
#5  session_cb (chan=<optimized out>, cond=<optimized out>, data=0x55e93e4317b0) at profiles/audio/avdtp.c:2286
#6  0x00007f5e225b9f69 in g_main_dispatch (context=0x55e93e3c6800) at ../glib/glib/gmain.c:3476
[...]

Originally reported in [1]

[1] https://gitlab.archlinux.org/archlinux/packaging/packages/bluez/-/issues/3

Cheers,

Ronan
diff mbox series

Patch

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 1e03b7b51..23ea267f6 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -606,11 +606,38 @@  static DBusMessage *try_acquire(DBusConnection *conn, DBusMessage *msg,
 	return NULL;
 }
 
+static void bap_stop_complete(struct bt_bap_stream *stream,
+					uint8_t code, uint8_t reason,
+					void *user_data)
+{
+	struct media_owner *owner = user_data;
+	struct media_request *req = owner->pending;
+	struct media_transport *transport = owner->transport;
+
+	/* Release always succeeds */
+	if (req) {
+		req->id = 0;
+		media_request_reply(req, 0);
+		media_owner_remove(owner);
+	}
+
+	transport_set_state(transport, TRANSPORT_STATE_IDLE);
+	media_transport_remove_owner(transport);
+}
+
+static void bap_disable_complete(struct bt_bap_stream *stream,
+					uint8_t code, uint8_t reason,
+					void *user_data)
+{
+	bap_stop_complete(stream, code, reason, user_data);
+}
+
 static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
 					void *data)
 {
 	struct media_transport *transport = data;
 	struct media_owner *owner = transport->owner;
+	struct bap_transport *bap = transport->data;
 	const char *sender;
 	struct media_request *req;
 	guint id;
@@ -642,6 +669,11 @@  static DBusMessage *release(DBusConnection *conn, DBusMessage *msg,
 	req = media_request_create(msg, id);
 	media_owner_add(owner, req);
 
+	if (bt_bap_stream_get_type(bap->stream) ==
+			BT_BAP_STREAM_TYPE_BCAST) {
+		bap_disable_complete(bap->stream, 0x00, 0x00, owner);
+	}
+
 	return NULL;
 }
 
@@ -1370,32 +1402,6 @@  static guint resume_bap(struct media_transport *transport,
 	return id;
 }
 
-static void bap_stop_complete(struct bt_bap_stream *stream,
-					uint8_t code, uint8_t reason,
-					void *user_data)
-{
-	struct media_owner *owner = user_data;
-	struct media_request *req = owner->pending;
-	struct media_transport *transport = owner->transport;
-
-	/* Release always succeeds */
-	if (req) {
-		req->id = 0;
-		media_request_reply(req, 0);
-		media_owner_remove(owner);
-	}
-
-	transport_set_state(transport, TRANSPORT_STATE_IDLE);
-	media_transport_remove_owner(transport);
-}
-
-static void bap_disable_complete(struct bt_bap_stream *stream,
-					uint8_t code, uint8_t reason,
-					void *user_data)
-{
-	bap_stop_complete(stream, code, reason, user_data);
-}
-
 static guint suspend_bap(struct media_transport *transport,
 				struct media_owner *owner)
 {
@@ -1499,9 +1505,14 @@  static void bap_state_changed(struct bt_bap_stream *stream, uint8_t old_state,
 			return;
 		break;
 	case BT_BAP_STREAM_STATE_STREAMING:
-		if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE)
+		if ((bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE) ||
+			(bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK))
 			bap_update_bcast_qos(transport);
 		break;
+	case BT_BAP_STREAM_STATE_RELEASING:
+		if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK)
+			return;
+		break;
 	}
 
 	io = bt_bap_stream_get_io(stream);