[Bluez,v2] audio: fix crash in a2dp_discover

Commit Message

Howard Chung March 8, 2022, 9:43 a.m. UTC

From: Yun-Hao Chung <[e-mail]>

Sample stack trace:
0x0000567c394e4c6b (bluetoothd - a2dp.c: 270) setup_cb_free
0x0000567c394e4a94 (bluetoothd - a2dp.c: 2884) a2dp_discover
0x0000567c394e3c03 (bluetoothd - sink.c: 275) sink_setup_stream
0x0000567c394e3d4f (bluetoothd - sink.c: 299) sink_connect
0x0000567c39535183 (bluetoothd - service.c: 294) btd_service_connect
0x0000567c39539f68 (bluetoothd - device.c: 2006) connect_next
0x0000567c3954086d (bluetoothd - device.c: 2060) service_state_changed
0x0000567c39534efb (bluetoothd - service.c: 111) change_state
0x0000567c3953559c (bluetoothd - service.c: 0)
btd_service_connecting_complete
0x0000567c39534a5c (bluetoothd - profile.c: 1641) record_cb
0x0000567c395197cd (bluetoothd - sdp-client.c: 298) connect_watch
0x00007b14bc8034f6 (libglib-2.0.so.0 - gmain.c: 3337)
g_main_context_dispatch
0x00007b14bc803801 (libglib-2.0.so.0 - gmain.c: 4131)
g_main_context_iterate
0x00007b14bc803a7d (libglib-2.0.so.0 - gmain.c: 4329) g_main_loop_run
0x0000567c39566af1 (bluetoothd - mainloop-glib.c: 79) mainloop_run
0x0000567c39566ddb (bluetoothd - mainloop-notify.c: 201)
mainloop_run_with_signal
0x0000567c3954bf4c (bluetoothd - main.c: 1222) main
0x00007b14bc579797 (libc.so.6 - libc-start.c: 332) __libc_start_main
0x0000567c394df449 (bluetoothd) _start
0x00007ffd70145737

This could be triggered from a2dp_discover -> avdtp_discover ->
send_request -> send_req -> l2cap_connect (return error) ->
avdtp_set_state (to disconnect state)-> channel_remove -> channel_free
-> finalize_setup_errno (discover cb is freed) -> error handling all
the way back to a2dp_discover -> a2dp_discover (discover cb is freed
again, crashed!).

The fix is to attach cb with setup after avdtp_discover success.

Suggested-by: [e-mail]

Reviewed-by: Archie Pusaka <[e-mail]>
---
Hi maintainers,

The fix is tested by forcing session->io to NULL in send_req in the code
and verifing the crash doesn't happen.

Changes in v2:
- adopted luiz's solution

 profiles/audio/a2dp.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Comments

[e-mail] March 8, 2022, 11:32 a.m. UTC | #1

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=621407

---Test result---

Test Summary:
CheckPatch                    PASS      1.57 seconds
GitLint                       PASS      1.06 seconds
Prep - Setup ELL              PASS      43.82 seconds
Build - Prep                  PASS      0.71 seconds
Build - Configure             PASS      8.66 seconds
Build - Make                  PASS      1737.37 seconds
Make Check                    PASS      11.41 seconds
Make Check w/Valgrind         PASS      459.80 seconds
Make Distcheck                PASS      248.64 seconds
Build w/ext ELL - Configure   PASS      9.35 seconds
Build w/ext ELL - Make        PASS      1311.54 seconds
Incremental Build with patchesPASS      0.00 seconds



---
Regards,
Linux Bluetooth

Patch

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 9fbcd35f7..f761dbe54 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -244,6 +244,15 @@  static struct a2dp_setup_cb *setup_cb_new(struct a2dp_setup *setup)
 	cb->setup = setup;
 	cb->id = ++cb_id;
 
+	return cb;
+}
+
+static struct a2dp_setup_cb *setup_cb_add(struct a2dp_setup *setup)
+{
+	struct a2dp_setup_cb *cb;
+
+	cb = setup_cb_new(setup);
+
 	setup->cb = g_slist_append(setup->cb, cb);
 	return cb;
 }
@@ -1840,7 +1849,7 @@  static int a2dp_reconfig(struct a2dp_channel *chan, const char *sender,
 	if (!setup)
 		return -ENOMEM;
 
-	cb_data = setup_cb_new(setup);
+	cb_data = setup_cb_add(setup);
 	cb_data->config_cb = reconfig_cb;
 	cb_data->user_data = user_data;
 
@@ -2878,12 +2887,17 @@  unsigned int a2dp_discover(struct avdtp *session, a2dp_discover_cb_t cb,
 	if (!setup)
 		return 0;
 
+	/* Don't add cb since avdtp_discover can end up disconnecting the
+	 * session causing the cb to be freed.
+	 */
 	cb_data = setup_cb_new(setup);
 	cb_data->discover_cb = cb;
 	cb_data->user_data = user_data;
 
-	if (avdtp_discover(session, discover_cb, setup) == 0)
+	if (avdtp_discover(session, discover_cb, setup) == 0) {
+		setup->cb = g_slist_append(setup->cb, cb_data);
 		return cb_data->id;
+	}
 
 	setup_cb_free(cb_data);
 	return 0;
@@ -2911,7 +2925,7 @@  unsigned int a2dp_select_capabilities(struct avdtp *session,
 	if (!setup)
 		return 0;
 
-	cb_data = setup_cb_new(setup);
+	cb_data = setup_cb_add(setup);
 	cb_data->select_cb = cb;
 	cb_data->user_data = user_data;
 
@@ -2984,7 +2998,7 @@  unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
 	if (!setup)
 		return 0;
 
-	cb_data = setup_cb_new(setup);
+	cb_data = setup_cb_add(setup);
 	cb_data->config_cb = cb;
 	cb_data->user_data = user_data;
 
@@ -3075,7 +3089,7 @@  unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
 	if (!setup)
 		return 0;
 
-	cb_data = setup_cb_new(setup);
+	cb_data = setup_cb_add(setup);
 	cb_data->resume_cb = cb;
 	cb_data->user_data = user_data;
 
@@ -3133,7 +3147,7 @@  unsigned int a2dp_suspend(struct avdtp *session, struct a2dp_sep *sep,
 	if (!setup)
 		return 0;
 
-	cb_data = setup_cb_new(setup);
+	cb_data = setup_cb_add(setup);
 	cb_data->suspend_cb = cb;
 	cb_data->user_data = user_data;