[PATCH] Bluetooth: hci_sync: Use safe loop when adding accept list

[PATCH] Bluetooth: hci_sync: Use safe loop when adding accept list

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

When in the middle of adding accept list, the userspace can still
remove devices, therefore causing crash if the removed device is
the one being processed.

Use a safe loop mechanism to guard against deletion while iterating
the pending items.

Below is a sample btsnoop log when user enters wrong passkey when
pairing a LE keyboard and the corresponding stacktrace.
@ MGMT Event: Command Complete (0x0001) plen 10
      Add Device (0x0033) plen 7
        Status: Success (0x00)
        LE Address: CA:CA:BD:78:37:F9 (Static)
< HCI Command: LE Add Device To Accept List (0x08|0x0011) plen 7
        Address type: Random (0x01)
        Address: CA:CA:BD:78:37:F9 (Static)
@ MGMT Event: Device Removed (0x001b) plen 7
        LE Address: CA:CA:BD:78:37:F9 (Static)
> HCI Event: Command Complete (0x0e) plen 4
      LE Add Device To Accept List (0x08|0x0011) ncmd 1
        Status: Success (0x00)

[  167.409813] Call trace:
[  167.409983]  hci_le_add_accept_list_sync+0x64/0x26c
[  167.410150]  hci_update_passive_scan_sync+0x5f0/0x6dc
[  167.410318]  add_device_sync+0x18/0x24
[  167.410486]  hci_cmd_sync_work+0xe8/0x150
[  167.410509]  process_one_work+0x140/0x4d0
[  167.410526]  worker_thread+0x134/0x2e4
[  167.410544]  kthread+0x148/0x160
[  167.410562]  ret_from_fork+0x10/0x30

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Zhengping Jiang <jiangzp@google.com>
Reviewed-by: Michael Sun <michaelfsun@google.com>

---

 net/bluetooth/hci_sync.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 3067d94e7a8e..8e843d34f7de 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1863,7 +1863,7 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev,
  */
 static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 {
-	struct hci_conn_params *params;
+	struct hci_conn_params *params, *tmp;
 	struct bdaddr_list *b, *t;
 	u8 num_entries = 0;
 	bool pend_conn, pend_report;
@@ -1930,7 +1930,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 	 * just abort and return filer policy value to not use the
 	 * accept list.
 	 */
-	list_for_each_entry(params, &hdev->pend_le_conns, action) {
+	list_for_each_entry_safe(params, tmp, &hdev->pend_le_conns, action) {
 		err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
 		if (err)
 			goto done;
@@ -1940,7 +1940,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 	 * the list of pending reports and also add these to the
 	 * accept list if there is still space. Abort if space runs out.
 	 */
-	list_for_each_entry(params, &hdev->pend_le_reports, action) {
+	list_for_each_entry_safe(params, tmp, &hdev->pend_le_reports, action) {
 		err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
 		if (err)
 			goto done;
-- 
2.37.1.359.gd136c6c3e2-goog

Re: [PATCH] Bluetooth: hci_sync: Use safe loop when adding accept list

[Eric Dumazet]

On Fri, Jul 22, 2022 at 12:23 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> When in the middle of adding accept list, the userspace can still
> remove devices, therefore causing crash if the removed device is
> the one being processed.
>
> Use a safe loop mechanism to guard against deletion while iterating
> the pending items.

 "the userspace can still remove devices" is a bit vague.

It seems that the issue at hand is that hci_le_add_accept_list_sync() can
move the current item from  pend_le_conns / pend_le_reports lists ?

Hopefully these lists can not be changed by other threads while
hci_update_accept_list_sync() is running ?


>
> Below is a sample btsnoop log when user enters wrong passkey when
> pairing a LE keyboard and the corresponding stacktrace.
> @ MGMT Event: Command Complete (0x0001) plen 10
>       Add Device (0x0033) plen 7
>         Status: Success (0x00)
>         LE Address: CA:CA:BD:78:37:F9 (Static)
> < HCI Command: LE Add Device To Accept List (0x08|0x0011) plen 7
>         Address type: Random (0x01)
>         Address: CA:CA:BD:78:37:F9 (Static)
> @ MGMT Event: Device Removed (0x001b) plen 7
>         LE Address: CA:CA:BD:78:37:F9 (Static)
> > HCI Event: Command Complete (0x0e) plen 4
>       LE Add Device To Accept List (0x08|0x0011) ncmd 1
>         Status: Success (0x00)
>
> [  167.409813] Call trace:
> [  167.409983]  hci_le_add_accept_list_sync+0x64/0x26c
> [  167.410150]  hci_update_passive_scan_sync+0x5f0/0x6dc
> [  167.410318]  add_device_sync+0x18/0x24
> [  167.410486]  hci_cmd_sync_work+0xe8/0x150
> [  167.410509]  process_one_work+0x140/0x4d0
> [  167.410526]  worker_thread+0x134/0x2e4
> [  167.410544]  kthread+0x148/0x160
> [  167.410562]  ret_from_fork+0x10/0x30
>
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>

Please add a Fixes: tag

> Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> Reviewed-by: Michael Sun <michaelfsun@google.com>
>
> ---
>
>  net/bluetooth/hci_sync.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 3067d94e7a8e..8e843d34f7de 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1863,7 +1863,7 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev,
>   */
>  static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
>  {
> -       struct hci_conn_params *params;
> +       struct hci_conn_params *params, *tmp;
>         struct bdaddr_list *b, *t;
>         u8 num_entries = 0;
>         bool pend_conn, pend_report;
> @@ -1930,7 +1930,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
>          * just abort and return filer policy value to not use the
>          * accept list.
>          */
> -       list_for_each_entry(params, &hdev->pend_le_conns, action) {
> +       list_for_each_entry_safe(params, tmp, &hdev->pend_le_conns, action) {
>                 err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
>                 if (err)
>                         goto done;
> @@ -1940,7 +1940,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
>          * the list of pending reports and also add these to the
>          * accept list if there is still space. Abort if space runs out.
>          */
> -       list_for_each_entry(params, &hdev->pend_le_reports, action) {
> +       list_for_each_entry_safe(params, tmp, &hdev->pend_le_reports, action) {
>                 err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
>                 if (err)
>                         goto done;
> --
> 2.37.1.359.gd136c6c3e2-goog
>

RE: Bluetooth: hci_sync: Use safe loop when adding accept list

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      1.40 seconds
GitLint                       PASS      0.53 seconds
SubjectPrefix                 PASS      0.32 seconds
BuildKernel                   PASS      42.52 seconds
BuildKernel32                 PASS      37.05 seconds
Incremental Build with patchesPASS      59.45 seconds
TestRunner: Setup             PASS      641.48 seconds
TestRunner: l2cap-tester      PASS      20.19 seconds
TestRunner: bnep-tester       PASS      6.83 seconds
TestRunner: mgmt-tester       PASS      113.16 seconds
TestRunner: rfcomm-tester     PASS      10.66 seconds
TestRunner: sco-tester        PASS      10.45 seconds
TestRunner: smp-tester        PASS      10.45 seconds
TestRunner: userchan-tester   PASS      7.07 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: hci_sync: Use safe loop when adding accept list

[Luiz Augusto von Dentz]

Hi Archie,

On Fri, Jul 22, 2022 at 3:23 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> When in the middle of adding accept list, the userspace can still
> remove devices, therefore causing crash if the removed device is
> the one being processed.
>
> Use a safe loop mechanism to guard against deletion while iterating
> the pending items.
>
> Below is a sample btsnoop log when user enters wrong passkey when
> pairing a LE keyboard and the corresponding stacktrace.
> @ MGMT Event: Command Complete (0x0001) plen 10
>       Add Device (0x0033) plen 7
>         Status: Success (0x00)
>         LE Address: CA:CA:BD:78:37:F9 (Static)
> < HCI Command: LE Add Device To Accept List (0x08|0x0011) plen 7
>         Address type: Random (0x01)
>         Address: CA:CA:BD:78:37:F9 (Static)
> @ MGMT Event: Device Removed (0x001b) plen 7
>         LE Address: CA:CA:BD:78:37:F9 (Static)
> > HCI Event: Command Complete (0x0e) plen 4
>       LE Add Device To Accept List (0x08|0x0011) ncmd 1
>         Status: Success (0x00)
>
> [  167.409813] Call trace:
> [  167.409983]  hci_le_add_accept_list_sync+0x64/0x26c
> [  167.410150]  hci_update_passive_scan_sync+0x5f0/0x6dc
> [  167.410318]  add_device_sync+0x18/0x24
> [  167.410486]  hci_cmd_sync_work+0xe8/0x150
> [  167.410509]  process_one_work+0x140/0x4d0
> [  167.410526]  worker_thread+0x134/0x2e4
> [  167.410544]  kthread+0x148/0x160
> [  167.410562]  ret_from_fork+0x10/0x30
>
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> Reviewed-by: Michael Sun <michaelfsun@google.com>
>
> ---
>
>  net/bluetooth/hci_sync.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 3067d94e7a8e..8e843d34f7de 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1863,7 +1863,7 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev,
>   */
>  static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
>  {
> -       struct hci_conn_params *params;
> +       struct hci_conn_params *params, *tmp;
>         struct bdaddr_list *b, *t;
>         u8 num_entries = 0;
>         bool pend_conn, pend_report;
> @@ -1930,7 +1930,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
>          * just abort and return filer policy value to not use the
>          * accept list.
>          */
> -       list_for_each_entry(params, &hdev->pend_le_conns, action) {
> +       list_for_each_entry_safe(params, tmp, &hdev->pend_le_conns, action) {
>                 err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
>                 if (err)
>                         goto done;
> @@ -1940,7 +1940,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
>          * the list of pending reports and also add these to the
>          * accept list if there is still space. Abort if space runs out.
>          */
> -       list_for_each_entry(params, &hdev->pend_le_reports, action) {
> +       list_for_each_entry_safe(params, tmp, &hdev->pend_le_reports, action) {
>                 err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
>                 if (err)
>                         goto done;

Hmm if this happens it means other threads are actually interfering
with cmd_sync queue which is something that is probably a bug since
the whole point of cmd_sync is to serialize the commands making it
easier to do more complex state updates (such accept+resolve list
updates), we could perhaps still apply this change as a workaround but
ultimately I think it would be better to add a mgmt-tester reproducing
the issue and have a proper fix of the code updating the list from a
different thread.

> --
> 2.37.1.359.gd136c6c3e2-goog
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: hci_sync: Use safe loop when adding accept list

[Archie Pusaka]

Hi Eric and Luiz,

>  "the userspace can still remove devices" is a bit vague.
I mean removing devices via MGMT command.

> It seems that the issue at hand is that hci_le_add_accept_list_sync() can
> move the current item from  pend_le_conns / pend_le_reports lists ?
The issue is, hci_le_add_accept_list_sync() is iterating the lists
when the content is being removed elsewhere.

> Hopefully these lists can not be changed by other threads while
> hci_update_accept_list_sync() is running ?
Probably. Looks like Luiz also thinks the same way.

> Please add a Fixes: tag
Unfortunately I don't know when this is introduced.

> Hmm if this happens it means other threads are actually interfering
> with cmd_sync queue which is something that is probably a bug since
> the whole point of cmd_sync is to serialize the commands making it
> easier to do more complex state updates (such accept+resolve list
> updates)
Thanks, I haven't fully grasped the intention of having hci_sync and
how to properly use it.

> we could perhaps still apply this change as a workaround but
> ultimately I think it would be better to add a mgmt-tester reproducing
> the issue and have a proper fix of the code updating the list from a
> different thread.
Agree. Having said that, I don't think currently I have the time to
invest in writing a test and a proper fix, so my apologies on this.

Best,
Archie