linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Bluez PATCH v1] a2dp: Fix crash in channel_free while waiting cmd resp
@ 2021-07-12 12:38 Howard Chung
  2021-07-12 13:13 ` [Bluez,v1] " bluez.test.bot
  2021-07-12 20:40 ` [Bluez PATCH v1] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 3+ messages in thread
From: Howard Chung @ 2021-07-12 12:38 UTC (permalink / raw)
  To: linux-bluetooth, luiz.dentz; +Cc: Yun-Hao Chung, Archie Pusaka

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

When channel_free is called and we are waiting for a command response
from the peer, bluez NULL the setup->session but would not free its
setup_cb. Since setup_cb holds a ref of setup, the setup wouldn't be
freed and if service_removed is called after channel_free, a2dp_cancel
tries to abort the ongoing avdtp commands, which accesses the
setup->session and triggers a crash.

This change finalizes all avdtp commands before assigning setup->session
to NULL in channel_free.

Crash stack trace:
0x000059f01943e688	(bluetoothd -avdtp.c:3690)
avdtp_abort
0x000059f01943928a	(bluetoothd -a2dp.c:3069)
a2dp_cancel
0x000059f0194377fa	(bluetoothd -sink.c:324)
sink_unregister
0x000059f01948715a	(bluetoothd -service.c:177)
service_remove
0x000059f01948d77c	(bluetoothd -device.c:5346)
device_remove
0x000059f019476d14	(bluetoothd -adapter.c:7202)
adapter_remove
0x000059f019476c3e	(bluetoothd -adapter.c:10827)
adapter_cleanup
0x000059f01949d8d7	(bluetoothd -main.c:1114)		main
0x0000787b36185d74	(libc.so.6 -libc-start.c:308)
__libc_start_main
0x000059f019433e39	(bluetoothd + 0x00026e39)		_start
0x00007fff2d2c0127

Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---
There are two other options to fix this crash.
1. add a NULL check in a2dp_cancel before calling avdtp_abort.
2. call setup_cb_free to every setup_cb in setup->cb in channel_free.

Since each setup_cb needs setup->session, I think there is no need to
keep the setup_cb after assigning setup->session to NULL. So the first
option is not ideal. If the second option is adopted, there would be
some time that sink/source->connect_id/disconnect_id is not zero, but
there is no corresponding setup_cb.

Test steps:
Reproduce the crash with the following steps. Verify the crash is
no longer observed after this change.
1. ignore AVDTP_SET_CONFIGURATION resp by modifying avdtp.c
2. turn on a paired headset
3. check the bluetooth.log, while bluez is waiting for
   AVDTP_SET_CONFIGURATION resp, stop bluetoothd immediately.
   This will trigger:
   session_cb (I/O error) -> connection_lost
   -> avdtp_set_state (AVDTP_SESSION_STATE_DISCONNECTED)
   -> avdtp_state_cb -> channel_remove -> channel_free
   then:
   adapter_cleanup -> adapter_remove -> device_remove -> service_remove
   -> a2dp_sink_remove -> sink_unregister -> sink_free -> a2dp_cancel
4. check if bluetoothd crash
The above steps can trigger the crash 100%.

 profiles/audio/a2dp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index d31ed845cbe7..f201b98c79d0 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -1540,9 +1540,14 @@ static void channel_free(void *data)
 	setup = find_setup_by_session(chan->session);
 	if (setup) {
 		setup->chan = NULL;
+		/* Finalize pending commands before we NULL setup->session */
+		finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
+							finalize_select,
+							finalize_config,
+							finalize_resume,
+							finalize_suspend, NULL);
 		avdtp_unref(setup->session);
 		setup->session = NULL;
-		finalize_setup_errno(setup, -ENOTCONN, NULL);
 	}
 
 	g_free(chan);
-- 
2.32.0.93.g670b81a890-goog


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

* RE: [Bluez,v1] a2dp: Fix crash in channel_free while waiting cmd resp
  2021-07-12 12:38 [Bluez PATCH v1] a2dp: Fix crash in channel_free while waiting cmd resp Howard Chung
@ 2021-07-12 13:13 ` bluez.test.bot
  2021-07-12 20:40 ` [Bluez PATCH v1] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 3+ messages in thread
From: bluez.test.bot @ 2021-07-12 13:13 UTC (permalink / raw)
  To: linux-bluetooth, howardchung

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.27 seconds
GitLint                       FAIL      0.10 seconds
Prep - Setup ELL              PASS      39.38 seconds
Build - Prep                  PASS      0.09 seconds
Build - Configure             PASS      6.92 seconds
Build - Make                  FAIL      130.35 seconds
Make Check                    FAIL      1.09 seconds
Make Distcheck                PASS      202.13 seconds
Build w/ext ELL - Configure   PASS      6.99 seconds
Build w/ext ELL - Make        FAIL      120.25 seconds

Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf

##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
a2dp: Fix crash in channel_free while waiting cmd resp
14: B3 Line contains hard tab characters (\t): "0x000059f01943e688	(bluetoothd -avdtp.c:3690)"
16: B3 Line contains hard tab characters (\t): "0x000059f01943928a	(bluetoothd -a2dp.c:3069)"
18: B3 Line contains hard tab characters (\t): "0x000059f0194377fa	(bluetoothd -sink.c:324)"
20: B3 Line contains hard tab characters (\t): "0x000059f01948715a	(bluetoothd -service.c:177)"
22: B3 Line contains hard tab characters (\t): "0x000059f01948d77c	(bluetoothd -device.c:5346)"
24: B3 Line contains hard tab characters (\t): "0x000059f019476d14	(bluetoothd -adapter.c:7202)"
26: B3 Line contains hard tab characters (\t): "0x000059f019476c3e	(bluetoothd -adapter.c:10827)"
28: B3 Line contains hard tab characters (\t): "0x000059f01949d8d7	(bluetoothd -main.c:1114)		main"
29: B3 Line contains hard tab characters (\t): "0x0000787b36185d74	(libc.so.6 -libc-start.c:308)"
31: B3 Line contains hard tab characters (\t): "0x000059f019433e39	(bluetoothd + 0x00026e39)		_start"


##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
profiles/audio/a2dp.c: In function ‘channel_free’:
profiles/audio/a2dp.c:1544:42: error: passing argument 3 of ‘finalize_setup_errno’ from incompatible pointer type [-Werror=incompatible-pointer-types]
 1544 |   finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
      |                                          ^~~~~~~~~~~~~~~~~
      |                                          |
      |                                          void (*)(struct a2dp_setup *)
profiles/audio/a2dp.c:262:18: note: expected ‘GSourceFunc’ {aka ‘int (*)(void *)’} but argument is of type ‘void (*)(struct a2dp_setup *)’
  262 |      GSourceFunc cb1, ...)
      |      ~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8531: profiles/audio/bluetoothd-a2dp.o] Error 1
make: *** [Makefile:4134: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
profiles/audio/a2dp.c: In function ‘channel_free’:
profiles/audio/a2dp.c:1544:42: error: passing argument 3 of ‘finalize_setup_errno’ from incompatible pointer type [-Werror=incompatible-pointer-types]
 1544 |   finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
      |                                          ^~~~~~~~~~~~~~~~~
      |                                          |
      |                                          void (*)(struct a2dp_setup *)
profiles/audio/a2dp.c:262:18: note: expected ‘GSourceFunc’ {aka ‘int (*)(void *)’} but argument is of type ‘void (*)(struct a2dp_setup *)’
  262 |      GSourceFunc cb1, ...)
      |      ~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8531: profiles/audio/bluetoothd-a2dp.o] Error 1
make: *** [Makefile:10406: check] Error 2


##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
profiles/audio/a2dp.c: In function ‘channel_free’:
profiles/audio/a2dp.c:1544:42: error: passing argument 3 of ‘finalize_setup_errno’ from incompatible pointer type [-Werror=incompatible-pointer-types]
 1544 |   finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
      |                                          ^~~~~~~~~~~~~~~~~
      |                                          |
      |                                          void (*)(struct a2dp_setup *)
profiles/audio/a2dp.c:262:18: note: expected ‘GSourceFunc’ {aka ‘int (*)(void *)’} but argument is of type ‘void (*)(struct a2dp_setup *)’
  262 |      GSourceFunc cb1, ...)
      |      ~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8531: profiles/audio/bluetoothd-a2dp.o] Error 1
make: *** [Makefile:4134: all] Error 2




---
Regards,
Linux Bluetooth


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

* Re: [Bluez PATCH v1] a2dp: Fix crash in channel_free while waiting cmd resp
  2021-07-12 12:38 [Bluez PATCH v1] a2dp: Fix crash in channel_free while waiting cmd resp Howard Chung
  2021-07-12 13:13 ` [Bluez,v1] " bluez.test.bot
@ 2021-07-12 20:40 ` Luiz Augusto von Dentz
  1 sibling, 0 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2021-07-12 20:40 UTC (permalink / raw)
  To: Howard Chung; +Cc: linux-bluetooth, Yun-Hao Chung, Archie Pusaka

Hi Howard,

On Mon, Jul 12, 2021 at 5:39 AM Howard Chung <howardchung@google.com> wrote:
>
> From: Yun-Hao Chung <howardchung@chromium.org>
>
> When channel_free is called and we are waiting for a command response
> from the peer, bluez NULL the setup->session but would not free its
> setup_cb. Since setup_cb holds a ref of setup, the setup wouldn't be
> freed and if service_removed is called after channel_free, a2dp_cancel
> tries to abort the ongoing avdtp commands, which accesses the
> setup->session and triggers a crash.
>
> This change finalizes all avdtp commands before assigning setup->session
> to NULL in channel_free.
>
> Crash stack trace:
> 0x000059f01943e688      (bluetoothd -avdtp.c:3690)
> avdtp_abort
> 0x000059f01943928a      (bluetoothd -a2dp.c:3069)
> a2dp_cancel
> 0x000059f0194377fa      (bluetoothd -sink.c:324)
> sink_unregister
> 0x000059f01948715a      (bluetoothd -service.c:177)
> service_remove
> 0x000059f01948d77c      (bluetoothd -device.c:5346)
> device_remove
> 0x000059f019476d14      (bluetoothd -adapter.c:7202)
> adapter_remove
> 0x000059f019476c3e      (bluetoothd -adapter.c:10827)
> adapter_cleanup
> 0x000059f01949d8d7      (bluetoothd -main.c:1114)               main
> 0x0000787b36185d74      (libc.so.6 -libc-start.c:308)
> __libc_start_main
> 0x000059f019433e39      (bluetoothd + 0x00026e39)               _start
> 0x00007fff2d2c0127
>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> There are two other options to fix this crash.
> 1. add a NULL check in a2dp_cancel before calling avdtp_abort.
> 2. call setup_cb_free to every setup_cb in setup->cb in channel_free.
>
> Since each setup_cb needs setup->session, I think there is no need to
> keep the setup_cb after assigning setup->session to NULL. So the first
> option is not ideal. If the second option is adopted, there would be
> some time that sink/source->connect_id/disconnect_id is not zero, but
> there is no corresponding setup_cb.
>
> Test steps:
> Reproduce the crash with the following steps. Verify the crash is
> no longer observed after this change.
> 1. ignore AVDTP_SET_CONFIGURATION resp by modifying avdtp.c
> 2. turn on a paired headset
> 3. check the bluetooth.log, while bluez is waiting for
>    AVDTP_SET_CONFIGURATION resp, stop bluetoothd immediately.
>    This will trigger:
>    session_cb (I/O error) -> connection_lost
>    -> avdtp_set_state (AVDTP_SESSION_STATE_DISCONNECTED)
>    -> avdtp_state_cb -> channel_remove -> channel_free
>    then:
>    adapter_cleanup -> adapter_remove -> device_remove -> service_remove
>    -> a2dp_sink_remove -> sink_unregister -> sink_free -> a2dp_cancel
> 4. check if bluetoothd crash
> The above steps can trigger the crash 100%.
>
>  profiles/audio/a2dp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index d31ed845cbe7..f201b98c79d0 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -1540,9 +1540,14 @@ static void channel_free(void *data)
>         setup = find_setup_by_session(chan->session);
>         if (setup) {
>                 setup->chan = NULL;
> +               /* Finalize pending commands before we NULL setup->session */
> +               finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
> +                                                       finalize_select,
> +                                                       finalize_config,
> +                                                       finalize_resume,
> +                                                       finalize_suspend, NULL);

While the analysis seems correct I wonder if wouldn't be a better idea
to have a finalize_all so we don't have to iterate the callback list
multiple times, other than that the patch looks good and I would
prioritize it to have it included asap.

>                 avdtp_unref(setup->session);
>                 setup->session = NULL;
> -               finalize_setup_errno(setup, -ENOTCONN, NULL);
>         }
>
>         g_free(chan);
> --
> 2.32.0.93.g670b81a890-goog
>


-- 
Luiz Augusto von Dentz

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

end of thread, other threads:[~2021-07-12 20:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 12:38 [Bluez PATCH v1] a2dp: Fix crash in channel_free while waiting cmd resp Howard Chung
2021-07-12 13:13 ` [Bluez,v1] " bluez.test.bot
2021-07-12 20:40 ` [Bluez PATCH v1] " Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).