[Bluez PATCH v1] audio: fix crash in a2dp_discover

[Bluez PATCH v1] audio: fix crash in a2dp_discover

[Howard Chung]

From: Yun-Hao Chung <howardchung@chromium.org>

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 add setup_ref/setup_unref around a2dp_discover to ensure
setup alive, and check if setup->chan to see if channel_free has been
called or not.

Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---
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.

 profiles/audio/a2dp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 9fbcd35f7..39e1e9624 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -2878,14 +2878,22 @@ unsigned int a2dp_discover(struct avdtp *session, a2dp_discover_cb_t cb,
 	if (!setup)
 		return 0;
 
+	setup_ref(setup);
 	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_unref(setup);
 		return cb_data->id;
+	}
 
-	setup_cb_free(cb_data);
+	/* Check if the channel is still there before freeing setup_cb, since it
+	 * could be freed by channel_free().
+	 */
+	if (setup->chan)
+		setup_cb_free(cb_data);
+	setup_unref(setup);
 	return 0;
 }
 
-- 
2.35.1.616.g0bdcbb4464-goog

RE: [Bluez,v1] audio: fix crash in a2dp_discover

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 997 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=620823

---Test result---

Test Summary:
CheckPatch                    PASS      1.54 seconds
GitLint                       PASS      1.09 seconds
Prep - Setup ELL              PASS      52.28 seconds
Build - Prep                  PASS      0.91 seconds
Build - Configure             PASS      10.71 seconds
Build - Make                  PASS      1460.20 seconds
Make Check                    PASS      13.01 seconds
Make Check w/Valgrind         PASS      538.68 seconds
Make Distcheck                PASS      282.48 seconds
Build w/ext ELL - Configure   PASS      10.73 seconds
Build w/ext ELL - Make        PASS      1428.08 seconds
Incremental Build with patchesPASS      0.00 seconds



---
Regards,
Linux Bluetooth

Re: [Bluez PATCH v1] audio: fix crash in a2dp_discover

[Luiz Augusto von Dentz]

Hi Howard,

On Sun, Mar 6, 2022 at 10:06 PM Howard Chung <howardchung@google.com> wrote:
>
> From: Yun-Hao Chung <howardchung@chromium.org>
>
> 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 add setup_ref/setup_unref around a2dp_discover to ensure
> setup alive, and check if setup->chan to see if channel_free has been
> called or not.
>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 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.
>
>  profiles/audio/a2dp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index 9fbcd35f7..39e1e9624 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -2878,14 +2878,22 @@ unsigned int a2dp_discover(struct avdtp *session, a2dp_discover_cb_t cb,
>         if (!setup)
>                 return 0;
>
> +       setup_ref(setup);
>         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_unref(setup);
>                 return cb_data->id;
> +       }
>
> -       setup_cb_free(cb_data);
> +       /* Check if the channel is still there before freeing setup_cb, since it
> +        * could be freed by channel_free().
> +        */
> +       if (setup->chan)
> +               setup_cb_free(cb_data);
> +       setup_unref(setup);
>         return 0;

This sounds a little too complicated and I'm afraid we may run into
other problems if we start taking extra references, so how about we
don't attach the cb_data to begin with:

https://gist.github.com/Vudentz/7f1a3eeea7decdb17b67d288b1dff77e

>  }
>
> --
> 2.35.1.616.g0bdcbb4464-goog
>


-- 
Luiz Augusto von Dentz