| Message ID | b723c232c6fd1f90e49b147180420d1856fb907d.1686252492.git.pav@iki.fi |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: ISO-related concurrency fixes | expand |
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=755455
---Test result---
Test Summary:
CheckPatch FAIL 2.84 seconds
GitLint FAIL 1.32 seconds
SubjectPrefix PASS 0.41 seconds
BuildKernel PASS 32.59 seconds
CheckAllWarning PASS 35.36 seconds
CheckSparse WARNING 40.70 seconds
CheckSmatch WARNING 110.07 seconds
BuildKernel32 PASS 32.20 seconds
TestRunnerSetup PASS 448.29 seconds
TestRunner_l2cap-tester PASS 16.94 seconds
TestRunner_iso-tester PASS 23.46 seconds
TestRunner_bnep-tester PASS 5.73 seconds
TestRunner_mgmt-tester PASS 117.30 seconds
TestRunner_rfcomm-tester PASS 9.10 seconds
TestRunner_sco-tester PASS 8.48 seconds
TestRunner_ioctl-tester PASS 9.91 seconds
TestRunner_mesh-tester PASS 7.27 seconds
TestRunner_smp-tester PASS 8.35 seconds
TestRunner_userchan-tester PASS 6.00 seconds
IncrementalBuild PASS 41.08 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#104:
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
total: 0 errors, 1 warnings, 0 checks, 165 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13272928.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
[3/3] Bluetooth: ISO: fix iso_conn related locking and validity issues
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#120:
iso_connect_cfm:1700: hcon 000000007b65d182 bdaddr 28:3d:c2:4a:7e:da status 12
total: 0 errors, 1 warnings, 0 checks, 123 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13272927.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
26: B1 Line exceeds max length (155>80): "BUG: KASAN: slab-use-after-free in hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841)"
29: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
35: B1 Line exceeds max length (107>80): "? __virt_addr_valid (./include/linux/mmzone.h:1915 ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65)"
36: B1 Line exceeds max length (122>80): "? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841)"
38: B1 Line exceeds max length (122>80): "? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841)"
39: B1 Line exceeds max length (120>80): "hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841)"
58: B1 Line exceeds max length (105>80): "hci_conn_params_add (./include/linux/slab.h:580 ./include/linux/slab.h:720 net/bluetooth/hci_core.c:2277)"
59: B1 Line exceeds max length (81>80): "hci_connect_le_scan (net/bluetooth/hci_conn.c:1419 net/bluetooth/hci_conn.c:1589)"
72: B1 Line exceeds max length (85>80): "__kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244)"
[2/3] Bluetooth: hci_event: call ISO disconnect callback before deleting conn
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
40: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
[3/3] Bluetooth: ISO: fix iso_conn related locking and validity issues
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
68: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
90: B1 Line exceeds max length (106>80): "general protection fault, probably for non-canonical address 0x30b29c630930aec8: 0000 [#1] PREEMPT SMP PTI"
92: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
94: B1 Line exceeds max length (134>80): "Code: 90 90 0f 1f 44 00 00 48 8b 47 08 48 85 c0 0f 84 ad 00 00 00 55 89 d5 53 48 83 3f 00 48 89 fb 74 7d 66 90 48 8b 03 48 8b 53 08 <>"
134: B1 Line exceeds max length (81>80): "Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014"
146: B2 Line has trailing whitespace: " "
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
---
Regards,
Linux Bluetooth
Hi Pauli. On Thu, Jun 8, 2023 at 2:10 PM Pauli Virtanen <pav@iki.fi> wrote: > > hci_update_accept_list_sync iterates over hdev->pend_le_conns and > hdev->pend_le_reports, and waits for controller events in the loop body, > without holding hdev lock. > > Meanwhile, these lists and the items may be modified e.g. by > le_scan_cleanup. This can invalidate the list cursor in the ongoing > iterations, resulting to invalid behavior (eg use-after-free). > > hdev lock should be held when operating on these lists, but it cannot be > held in the loop bodies here as they block. The loops are in hci_sync, > so at most one loop runs at a time (per hdev). > > Fix this by doing the iteration in a safe way vs. list mutation, and > copy data to avoid locking. Add hci_conn_params.add_pending flag to > track which items are left. > > Lock also around hci_pend_le_action_lookup there. > > This fixes the following, which can be triggered at least by changing > hci_le_set_cig_params to always return false, and running BlueZ > iso-tester, which causes connections to be created and dropped fast: > > ================================================================== > BUG: KASAN: slab-use-after-free in hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > Read of size 8 at addr ffff888001265018 by task kworker/u3:0/32 > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014 > Workqueue: hci0 hci_cmd_sync_work > Call Trace: > <TASK> > dump_stack_lvl (./arch/x86/include/asm/irqflags.h:134 lib/dump_stack.c:107) > print_report (mm/kasan/report.c:320 mm/kasan/report.c:430) > ? __virt_addr_valid (./include/linux/mmzone.h:1915 ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65) > ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > kasan_report (mm/kasan/report.c:538) > ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > ? __pfx_hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2780) > ? mutex_lock (kernel/locking/mutex.c:282) > ? __pfx_mutex_lock (kernel/locking/mutex.c:282) > ? __pfx_mutex_unlock (kernel/locking/mutex.c:538) > ? __pfx_update_passive_scan_sync (net/bluetooth/hci_sync.c:2861) > hci_cmd_sync_work (net/bluetooth/hci_sync.c:306) > process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) > ? __pfx_worker_thread (kernel/workqueue.c:2480) > kthread (kernel/kthread.c:376) > ? __pfx_kthread (kernel/kthread.c:331) > ret_from_fork (arch/x86/entry/entry_64.S:314) > </TASK> > > Allocated by task 31: > kasan_save_stack (mm/kasan/common.c:46) > kasan_set_track (mm/kasan/common.c:52) > __kasan_kmalloc (mm/kasan/common.c:374 mm/kasan/common.c:383) > hci_conn_params_add (./include/linux/slab.h:580 ./include/linux/slab.h:720 net/bluetooth/hci_core.c:2277) > hci_connect_le_scan (net/bluetooth/hci_conn.c:1419 net/bluetooth/hci_conn.c:1589) > hci_connect_cis (net/bluetooth/hci_conn.c:2266) > iso_connect_cis (net/bluetooth/iso.c:390) > iso_sock_connect (net/bluetooth/iso.c:899) > __sys_connect (net/socket.c:2003 net/socket.c:2020) > __x64_sys_connect (net/socket.c:2027) > do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) > > Freed by task 15: > kasan_save_stack (mm/kasan/common.c:46) > kasan_set_track (mm/kasan/common.c:52) > kasan_save_free_info (mm/kasan/generic.c:523) > __kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244) > __kmem_cache_free (mm/slub.c:1807 mm/slub.c:3787 mm/slub.c:3800) > hci_conn_params_del (net/bluetooth/hci_core.c:2323) > le_scan_cleanup (net/bluetooth/hci_conn.c:202) > process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) > kthread (kernel/kthread.c:376) > ret_from_fork (arch/x86/entry/entry_64.S:314) > ================================================================== > > Fixes: e8907f76544f ("Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 3") > Signed-off-by: Pauli Virtanen <pav@iki.fi> > --- > > Notes: > It might be that even more of hci_passive_scan_sync and the routines it > calls should hold hdev->lock, but don't know that right now. > > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_sync.c | 81 ++++++++++++++++++++++++++++---- > 2 files changed, 74 insertions(+), 8 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 683666ea210c..e79b3831fcf3 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -822,6 +822,7 @@ struct hci_conn_params { > > struct hci_conn *conn; > bool explicit_connect; > + bool add_pending; > hci_conn_flags_t flags; > u8 privacy_mode; > }; > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 97da5bcaa904..e6fde15dc9ca 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -2160,15 +2160,23 @@ static int hci_le_del_accept_list_sync(struct hci_dev *hdev, > return 0; > } > > +struct conn_params { > + bdaddr_t addr; > + u8 addr_type; > + hci_conn_flags_t flags; > + u8 privacy_mode; > +}; > + > /* Adds connection to resolve list if needed. > * Setting params to NULL programs local hdev->irk > */ > static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > - struct hci_conn_params *params) > + struct conn_params *params) > { > struct hci_cp_le_add_to_resolv_list cp; > struct smp_irk *irk; > struct bdaddr_list_with_irk *entry; > + struct hci_conn_params *hparams; > > if (!use_ll_privacy(hdev)) > return 0; > @@ -2203,6 +2211,13 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > /* Default privacy mode is always Network */ > params->privacy_mode = HCI_NETWORK_PRIVACY; > > + hci_dev_lock(hdev); > + hparams = hci_conn_params_lookup(hdev, ¶ms->addr, > + params->addr_type); > + if (hparams) > + hparams->privacy_mode = HCI_NETWORK_PRIVACY; > + hci_dev_unlock(hdev); > + > done: > if (hci_dev_test_flag(hdev, HCI_PRIVACY)) > memcpy(cp.local_irk, hdev->irk, 16); > @@ -2215,7 +2230,7 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > /* Set Device Privacy Mode. */ > static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, > - struct hci_conn_params *params) > + struct conn_params *params) > { > struct hci_cp_le_set_privacy_mode cp; > struct smp_irk *irk; > @@ -2249,7 +2264,7 @@ static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, > * properly set the privacy mode. > */ > static int hci_le_add_accept_list_sync(struct hci_dev *hdev, > - struct hci_conn_params *params, > + struct conn_params *params, > u8 *num_entries) > { > struct hci_cp_le_add_to_accept_list cp; > @@ -2447,6 +2462,37 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev, > return __hci_cmd_sync_sk(hdev, opcode, 0, NULL, 0, HCI_CMD_TIMEOUT, sk); > } > > +static void conn_params_iter_init(struct list_head *list) > +{ > + struct hci_conn_params *params; > + > + list_for_each_entry(params, list, action) > + params->add_pending = true; > +} > + > +static bool conn_params_iter_next(struct list_head *list, > + struct conn_params *item) > +{ > + struct hci_conn_params *params; > + > + /* Must hold hdev lock. Not reentrant. Mutating list is allowed. */ > + > + list_for_each_entry(params, list, action) { > + if (!params->add_pending) > + continue; > + > + memcpy(&item->addr, ¶ms->addr, sizeof(params->addr)); > + item->addr_type = params->addr_type; > + item->privacy_mode = params->privacy_mode; > + item->flags = params->flags; > + > + params->add_pending = false; > + return true; > + } > + > + return false; > +} I wonder if we shouldn't take the approach of hci_lookup_connect_le though, it uses rcu_read_lock + list_for_each_entry_rcu which seems to be more efficient than having a dedicated lock to protect the list. > /* Device must not be scanning when updating the accept list. > * > * Update is done using the following sequence: > @@ -2466,7 +2512,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 conn_params params; > struct bdaddr_list *b, *t; > u8 num_entries = 0; > bool pend_conn, pend_report; > @@ -2494,6 +2540,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > goto done; > } > > + hci_dev_lock(hdev); > + > /* Go through the current accept list programmed into the > * controller one by one and check if that address is connected or is > * still in the list of pending connections or list of devices to > @@ -2515,8 +2563,10 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > * remove it from the acceptlist. > */ > if (!pend_conn && !pend_report) { > + hci_dev_unlock(hdev); > hci_le_del_accept_list_sync(hdev, &b->bdaddr, > b->bdaddr_type); > + hci_dev_lock(hdev); > continue; > } > > @@ -2532,23 +2582,38 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > * available accept list entries in the controller, then > * just abort and return filer policy value to not use the > * accept list. > + * > + * The list may be mutated at any time outside hdev lock, > + * so use special iteration with copies. > */ > - list_for_each_entry(params, &hdev->pend_le_conns, action) { > - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); > + > + conn_params_iter_init(&hdev->pend_le_conns); > + > + while (conn_params_iter_next(&hdev->pend_le_conns, ¶ms)) { > + hci_dev_unlock(hdev); > + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); > if (err) > goto done; > + hci_dev_lock(hdev); > } > > /* After adding all new pending connections, walk through > * 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) { > - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); > + > + conn_params_iter_init(&hdev->pend_le_reports); > + > + while (conn_params_iter_next(&hdev->pend_le_reports, ¶ms)) { > + hci_dev_unlock(hdev); > + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); > if (err) > goto done; > + hci_dev_lock(hdev); > } > > + hci_dev_unlock(hdev); > + > /* Use the allowlist unless the following conditions are all true: > * - We are not currently suspending > * - There are 1 or more ADV monitors registered and it's not offloaded > -- > 2.40.1 >
Hi Luiz, pe, 2023-06-09 kello 10:19 -0700, Luiz Augusto von Dentz kirjoitti: > On Thu, Jun 8, 2023 at 2:10 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > hci_update_accept_list_sync iterates over hdev->pend_le_conns and > > hdev->pend_le_reports, and waits for controller events in the loop body, > > without holding hdev lock. > > > > Meanwhile, these lists and the items may be modified e.g. by > > le_scan_cleanup. This can invalidate the list cursor in the ongoing > > iterations, resulting to invalid behavior (eg use-after-free). > > > > hdev lock should be held when operating on these lists, but it cannot be > > held in the loop bodies here as they block. The loops are in hci_sync, > > so at most one loop runs at a time (per hdev). > > > > Fix this by doing the iteration in a safe way vs. list mutation, and > > copy data to avoid locking. Add hci_conn_params.add_pending flag to > > track which items are left. > > > > Lock also around hci_pend_le_action_lookup there. > > > > This fixes the following, which can be triggered at least by changing > > hci_le_set_cig_params to always return false, and running BlueZ > > iso-tester, which causes connections to be created and dropped fast: > > > > ================================================================== > > BUG: KASAN: slab-use-after-free in hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > Read of size 8 at addr ffff888001265018 by task kworker/u3:0/32 > > > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014 > > Workqueue: hci0 hci_cmd_sync_work > > Call Trace: > > <TASK> > > dump_stack_lvl (./arch/x86/include/asm/irqflags.h:134 lib/dump_stack.c:107) > > print_report (mm/kasan/report.c:320 mm/kasan/report.c:430) > > ? __virt_addr_valid (./include/linux/mmzone.h:1915 ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65) > > ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > kasan_report (mm/kasan/report.c:538) > > ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > ? __pfx_hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2780) > > ? mutex_lock (kernel/locking/mutex.c:282) > > ? __pfx_mutex_lock (kernel/locking/mutex.c:282) > > ? __pfx_mutex_unlock (kernel/locking/mutex.c:538) > > ? __pfx_update_passive_scan_sync (net/bluetooth/hci_sync.c:2861) > > hci_cmd_sync_work (net/bluetooth/hci_sync.c:306) > > process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) > > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) > > ? __pfx_worker_thread (kernel/workqueue.c:2480) > > kthread (kernel/kthread.c:376) > > ? __pfx_kthread (kernel/kthread.c:331) > > ret_from_fork (arch/x86/entry/entry_64.S:314) > > </TASK> > > > > Allocated by task 31: > > kasan_save_stack (mm/kasan/common.c:46) > > kasan_set_track (mm/kasan/common.c:52) > > __kasan_kmalloc (mm/kasan/common.c:374 mm/kasan/common.c:383) > > hci_conn_params_add (./include/linux/slab.h:580 ./include/linux/slab.h:720 net/bluetooth/hci_core.c:2277) > > hci_connect_le_scan (net/bluetooth/hci_conn.c:1419 net/bluetooth/hci_conn.c:1589) > > hci_connect_cis (net/bluetooth/hci_conn.c:2266) > > iso_connect_cis (net/bluetooth/iso.c:390) > > iso_sock_connect (net/bluetooth/iso.c:899) > > __sys_connect (net/socket.c:2003 net/socket.c:2020) > > __x64_sys_connect (net/socket.c:2027) > > do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) > > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) > > > > Freed by task 15: > > kasan_save_stack (mm/kasan/common.c:46) > > kasan_set_track (mm/kasan/common.c:52) > > kasan_save_free_info (mm/kasan/generic.c:523) > > __kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244) > > __kmem_cache_free (mm/slub.c:1807 mm/slub.c:3787 mm/slub.c:3800) > > hci_conn_params_del (net/bluetooth/hci_core.c:2323) > > le_scan_cleanup (net/bluetooth/hci_conn.c:202) > > process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) > > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) > > kthread (kernel/kthread.c:376) > > ret_from_fork (arch/x86/entry/entry_64.S:314) > > ================================================================== > > > > Fixes: e8907f76544f ("Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 3") > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > --- > > > > Notes: > > It might be that even more of hci_passive_scan_sync and the routines it > > calls should hold hdev->lock, but don't know that right now. > > > > include/net/bluetooth/hci_core.h | 1 + > > net/bluetooth/hci_sync.c | 81 ++++++++++++++++++++++++++++---- > > 2 files changed, 74 insertions(+), 8 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index 683666ea210c..e79b3831fcf3 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -822,6 +822,7 @@ struct hci_conn_params { > > > > struct hci_conn *conn; > > bool explicit_connect; > > + bool add_pending; > > hci_conn_flags_t flags; > > u8 privacy_mode; > > }; > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > index 97da5bcaa904..e6fde15dc9ca 100644 > > --- a/net/bluetooth/hci_sync.c > > +++ b/net/bluetooth/hci_sync.c > > @@ -2160,15 +2160,23 @@ static int hci_le_del_accept_list_sync(struct hci_dev *hdev, > > return 0; > > } > > > > +struct conn_params { > > + bdaddr_t addr; > > + u8 addr_type; > > + hci_conn_flags_t flags; > > + u8 privacy_mode; > > +}; > > + > > /* Adds connection to resolve list if needed. > > * Setting params to NULL programs local hdev->irk > > */ > > static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > - struct hci_conn_params *params) > > + struct conn_params *params) > > { > > struct hci_cp_le_add_to_resolv_list cp; > > struct smp_irk *irk; > > struct bdaddr_list_with_irk *entry; > > + struct hci_conn_params *hparams; > > > > if (!use_ll_privacy(hdev)) > > return 0; > > @@ -2203,6 +2211,13 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > /* Default privacy mode is always Network */ > > params->privacy_mode = HCI_NETWORK_PRIVACY; > > > > + hci_dev_lock(hdev); > > + hparams = hci_conn_params_lookup(hdev, ¶ms->addr, > > + params->addr_type); > > + if (hparams) > > + hparams->privacy_mode = HCI_NETWORK_PRIVACY; > > + hci_dev_unlock(hdev); > > + > > done: > > if (hci_dev_test_flag(hdev, HCI_PRIVACY)) > > memcpy(cp.local_irk, hdev->irk, 16); > > @@ -2215,7 +2230,7 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > > > /* Set Device Privacy Mode. */ > > static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, > > - struct hci_conn_params *params) > > + struct conn_params *params) > > { > > struct hci_cp_le_set_privacy_mode cp; > > struct smp_irk *irk; > > @@ -2249,7 +2264,7 @@ static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, > > * properly set the privacy mode. > > */ > > static int hci_le_add_accept_list_sync(struct hci_dev *hdev, > > - struct hci_conn_params *params, > > + struct conn_params *params, > > u8 *num_entries) > > { > > struct hci_cp_le_add_to_accept_list cp; > > @@ -2447,6 +2462,37 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev, > > return __hci_cmd_sync_sk(hdev, opcode, 0, NULL, 0, HCI_CMD_TIMEOUT, sk); > > } > > > > +static void conn_params_iter_init(struct list_head *list) > > +{ > > + struct hci_conn_params *params; > > + > > + list_for_each_entry(params, list, action) > > + params->add_pending = true; > > +} > > + > > +static bool conn_params_iter_next(struct list_head *list, > > + struct conn_params *item) > > +{ > > + struct hci_conn_params *params; > > + > > + /* Must hold hdev lock. Not reentrant. Mutating list is allowed. */ > > + > > + list_for_each_entry(params, list, action) { > > + if (!params->add_pending) > > + continue; > > + > > + memcpy(&item->addr, ¶ms->addr, sizeof(params->addr)); > > + item->addr_type = params->addr_type; > > + item->privacy_mode = params->privacy_mode; > > + item->flags = params->flags; > > + > > + params->add_pending = false; > > + return true; > > + } > > + > > + return false; > > +} > > I wonder if we shouldn't take the approach of hci_lookup_connect_le > though, it uses rcu_read_lock + list_for_each_entry_rcu which seems to > be more efficient than having a dedicated lock to protect the list. It could use RCU in this function instead of hdev->lock, we'd need to change the several places where the list is modified though. I can change that if it's better that way (I don't have a good idea which is more efficient, or if this list is "mostly-read" one). I'm not sure RCU would help getting rid of the add_pending flag or copying the params data though: IIUC, RCU does not guarantee items or the list cursor stay alive once you leave the read side critical section (Documentation/RCU/checklist.rst point 2). If it's like this, it wouldn't be allowed to rcu_read_unlock in the loop body, and then rcu_read_lock again and continue the iteration, and we'd need a lock or data copy to prevent params from being freed. > > /* Device must not be scanning when updating the accept list. > > * > > * Update is done using the following sequence: > > @@ -2466,7 +2512,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 conn_params params; > > struct bdaddr_list *b, *t; > > u8 num_entries = 0; > > bool pend_conn, pend_report; > > @@ -2494,6 +2540,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > > goto done; > > } > > > > + hci_dev_lock(hdev); > > + > > /* Go through the current accept list programmed into the > > * controller one by one and check if that address is connected or is > > * still in the list of pending connections or list of devices to > > @@ -2515,8 +2563,10 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > > * remove it from the acceptlist. > > */ > > if (!pend_conn && !pend_report) { > > + hci_dev_unlock(hdev); > > hci_le_del_accept_list_sync(hdev, &b->bdaddr, > > b->bdaddr_type); > > + hci_dev_lock(hdev); > > continue; > > } > > > > @@ -2532,23 +2582,38 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > > * available accept list entries in the controller, then > > * just abort and return filer policy value to not use the > > * accept list. > > + * > > + * The list may be mutated at any time outside hdev lock, > > + * so use special iteration with copies. > > */ > > - list_for_each_entry(params, &hdev->pend_le_conns, action) { > > - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); > > + > > + conn_params_iter_init(&hdev->pend_le_conns); > > + > > + while (conn_params_iter_next(&hdev->pend_le_conns, ¶ms)) { > > + hci_dev_unlock(hdev); > > + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); > > if (err) > > goto done; > > + hci_dev_lock(hdev); > > } > > > > /* After adding all new pending connections, walk through > > * 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) { > > - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); > > + > > + conn_params_iter_init(&hdev->pend_le_reports); > > + > > + while (conn_params_iter_next(&hdev->pend_le_reports, ¶ms)) { > > + hci_dev_unlock(hdev); > > + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); > > if (err) > > goto done; > > + hci_dev_lock(hdev); > > } > > > > + hci_dev_unlock(hdev); > > + > > /* Use the allowlist unless the following conditions are all true: > > * - We are not currently suspending > > * - There are 1 or more ADV monitors registered and it's not offloaded > > -- > > 2.40.1 > > > >
Hi Pauli, On Fri, Jun 9, 2023 at 11:22 AM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > pe, 2023-06-09 kello 10:19 -0700, Luiz Augusto von Dentz kirjoitti: > > On Thu, Jun 8, 2023 at 2:10 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > > > hci_update_accept_list_sync iterates over hdev->pend_le_conns and > > > hdev->pend_le_reports, and waits for controller events in the loop body, > > > without holding hdev lock. > > > > > > Meanwhile, these lists and the items may be modified e.g. by > > > le_scan_cleanup. This can invalidate the list cursor in the ongoing > > > iterations, resulting to invalid behavior (eg use-after-free). > > > > > > hdev lock should be held when operating on these lists, but it cannot be > > > held in the loop bodies here as they block. The loops are in hci_sync, > > > so at most one loop runs at a time (per hdev). > > > > > > Fix this by doing the iteration in a safe way vs. list mutation, and > > > copy data to avoid locking. Add hci_conn_params.add_pending flag to > > > track which items are left. > > > > > > Lock also around hci_pend_le_action_lookup there. > > > > > > This fixes the following, which can be triggered at least by changing > > > hci_le_set_cig_params to always return false, and running BlueZ > > > iso-tester, which causes connections to be created and dropped fast: > > > > > > ================================================================== > > > BUG: KASAN: slab-use-after-free in hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > > Read of size 8 at addr ffff888001265018 by task kworker/u3:0/32 > > > > > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014 > > > Workqueue: hci0 hci_cmd_sync_work > > > Call Trace: > > > <TASK> > > > dump_stack_lvl (./arch/x86/include/asm/irqflags.h:134 lib/dump_stack.c:107) > > > print_report (mm/kasan/report.c:320 mm/kasan/report.c:430) > > > ? __virt_addr_valid (./include/linux/mmzone.h:1915 ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65) > > > ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > > kasan_report (mm/kasan/report.c:538) > > > ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > > hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) > > > ? __pfx_hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2780) > > > ? mutex_lock (kernel/locking/mutex.c:282) > > > ? __pfx_mutex_lock (kernel/locking/mutex.c:282) > > > ? __pfx_mutex_unlock (kernel/locking/mutex.c:538) > > > ? __pfx_update_passive_scan_sync (net/bluetooth/hci_sync.c:2861) > > > hci_cmd_sync_work (net/bluetooth/hci_sync.c:306) > > > process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) > > > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) > > > ? __pfx_worker_thread (kernel/workqueue.c:2480) > > > kthread (kernel/kthread.c:376) > > > ? __pfx_kthread (kernel/kthread.c:331) > > > ret_from_fork (arch/x86/entry/entry_64.S:314) > > > </TASK> > > > > > > Allocated by task 31: > > > kasan_save_stack (mm/kasan/common.c:46) > > > kasan_set_track (mm/kasan/common.c:52) > > > __kasan_kmalloc (mm/kasan/common.c:374 mm/kasan/common.c:383) > > > hci_conn_params_add (./include/linux/slab.h:580 ./include/linux/slab.h:720 net/bluetooth/hci_core.c:2277) > > > hci_connect_le_scan (net/bluetooth/hci_conn.c:1419 net/bluetooth/hci_conn.c:1589) > > > hci_connect_cis (net/bluetooth/hci_conn.c:2266) > > > iso_connect_cis (net/bluetooth/iso.c:390) > > > iso_sock_connect (net/bluetooth/iso.c:899) > > > __sys_connect (net/socket.c:2003 net/socket.c:2020) > > > __x64_sys_connect (net/socket.c:2027) > > > do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) > > > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) > > > > > > Freed by task 15: > > > kasan_save_stack (mm/kasan/common.c:46) > > > kasan_set_track (mm/kasan/common.c:52) > > > kasan_save_free_info (mm/kasan/generic.c:523) > > > __kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244) > > > __kmem_cache_free (mm/slub.c:1807 mm/slub.c:3787 mm/slub.c:3800) > > > hci_conn_params_del (net/bluetooth/hci_core.c:2323) > > > le_scan_cleanup (net/bluetooth/hci_conn.c:202) > > > process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) > > > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) > > > kthread (kernel/kthread.c:376) > > > ret_from_fork (arch/x86/entry/entry_64.S:314) > > > ================================================================== > > > > > > Fixes: e8907f76544f ("Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 3") > > > Signed-off-by: Pauli Virtanen <pav@iki.fi> > > > --- > > > > > > Notes: > > > It might be that even more of hci_passive_scan_sync and the routines it > > > calls should hold hdev->lock, but don't know that right now. > > > > > > include/net/bluetooth/hci_core.h | 1 + > > > net/bluetooth/hci_sync.c | 81 ++++++++++++++++++++++++++++---- > > > 2 files changed, 74 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > index 683666ea210c..e79b3831fcf3 100644 > > > --- a/include/net/bluetooth/hci_core.h > > > +++ b/include/net/bluetooth/hci_core.h > > > @@ -822,6 +822,7 @@ struct hci_conn_params { > > > > > > struct hci_conn *conn; > > > bool explicit_connect; > > > + bool add_pending; > > > hci_conn_flags_t flags; > > > u8 privacy_mode; > > > }; > > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > > index 97da5bcaa904..e6fde15dc9ca 100644 > > > --- a/net/bluetooth/hci_sync.c > > > +++ b/net/bluetooth/hci_sync.c > > > @@ -2160,15 +2160,23 @@ static int hci_le_del_accept_list_sync(struct hci_dev *hdev, > > > return 0; > > > } > > > > > > +struct conn_params { > > > + bdaddr_t addr; > > > + u8 addr_type; > > > + hci_conn_flags_t flags; > > > + u8 privacy_mode; > > > +}; > > > + > > > /* Adds connection to resolve list if needed. > > > * Setting params to NULL programs local hdev->irk > > > */ > > > static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > > - struct hci_conn_params *params) > > > + struct conn_params *params) > > > { > > > struct hci_cp_le_add_to_resolv_list cp; > > > struct smp_irk *irk; > > > struct bdaddr_list_with_irk *entry; > > > + struct hci_conn_params *hparams; > > > > > > if (!use_ll_privacy(hdev)) > > > return 0; > > > @@ -2203,6 +2211,13 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > > /* Default privacy mode is always Network */ > > > params->privacy_mode = HCI_NETWORK_PRIVACY; > > > > > > + hci_dev_lock(hdev); > > > + hparams = hci_conn_params_lookup(hdev, ¶ms->addr, > > > + params->addr_type); > > > + if (hparams) > > > + hparams->privacy_mode = HCI_NETWORK_PRIVACY; > > > + hci_dev_unlock(hdev); > > > + > > > done: > > > if (hci_dev_test_flag(hdev, HCI_PRIVACY)) > > > memcpy(cp.local_irk, hdev->irk, 16); > > > @@ -2215,7 +2230,7 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, > > > > > > /* Set Device Privacy Mode. */ > > > static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, > > > - struct hci_conn_params *params) > > > + struct conn_params *params) > > > { > > > struct hci_cp_le_set_privacy_mode cp; > > > struct smp_irk *irk; > > > @@ -2249,7 +2264,7 @@ static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, > > > * properly set the privacy mode. > > > */ > > > static int hci_le_add_accept_list_sync(struct hci_dev *hdev, > > > - struct hci_conn_params *params, > > > + struct conn_params *params, > > > u8 *num_entries) > > > { > > > struct hci_cp_le_add_to_accept_list cp; > > > @@ -2447,6 +2462,37 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev, > > > return __hci_cmd_sync_sk(hdev, opcode, 0, NULL, 0, HCI_CMD_TIMEOUT, sk); > > > } > > > > > > +static void conn_params_iter_init(struct list_head *list) > > > +{ > > > + struct hci_conn_params *params; > > > + > > > + list_for_each_entry(params, list, action) > > > + params->add_pending = true; > > > +} > > > + > > > +static bool conn_params_iter_next(struct list_head *list, > > > + struct conn_params *item) > > > +{ > > > + struct hci_conn_params *params; > > > + > > > + /* Must hold hdev lock. Not reentrant. Mutating list is allowed. */ > > > + > > > + list_for_each_entry(params, list, action) { > > > + if (!params->add_pending) > > > + continue; > > > + > > > + memcpy(&item->addr, ¶ms->addr, sizeof(params->addr)); > > > + item->addr_type = params->addr_type; > > > + item->privacy_mode = params->privacy_mode; > > > + item->flags = params->flags; > > > + > > > + params->add_pending = false; > > > + return true; > > > + } > > > + > > > + return false; > > > +} > > > > I wonder if we shouldn't take the approach of hci_lookup_connect_le > > though, it uses rcu_read_lock + list_for_each_entry_rcu which seems to > > be more efficient than having a dedicated lock to protect the list. > > It could use RCU in this function instead of hdev->lock, we'd need to > change the several places where the list is modified though. I can > change that if it's better that way (I don't have a good idea which is > more efficient, or if this list is "mostly-read" one). We have been extensively using it for hci_conn lists though so I'd give it a try. > > I'm not sure RCU would help getting rid of the add_pending flag or > copying the params data though: IIUC, RCU does not guarantee items or > the list cursor stay alive once you leave the read side critical > section (Documentation/RCU/checklist.rst point 2). If it's like this, > it wouldn't be allowed to rcu_read_unlock in the loop body, and then > rcu_read_lock again and continue the iteration, and we'd need a lock or > data copy to prevent params from being freed. Yeah, I got that doing RCU unlock+lock pattern on a loop is not going to be safe, but it is not that different than using a dedicated lock either since once you unlock other threats can modify the data as well, so RCU only really help us not introducing more and more locks to protect lists for example. > > > > /* Device must not be scanning when updating the accept list. > > > * > > > * Update is done using the following sequence: > > > @@ -2466,7 +2512,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 conn_params params; > > > struct bdaddr_list *b, *t; > > > u8 num_entries = 0; > > > bool pend_conn, pend_report; > > > @@ -2494,6 +2540,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > > > goto done; > > > } > > > > > > + hci_dev_lock(hdev); > > > + > > > /* Go through the current accept list programmed into the > > > * controller one by one and check if that address is connected or is > > > * still in the list of pending connections or list of devices to > > > @@ -2515,8 +2563,10 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > > > * remove it from the acceptlist. > > > */ > > > if (!pend_conn && !pend_report) { > > > + hci_dev_unlock(hdev); > > > hci_le_del_accept_list_sync(hdev, &b->bdaddr, > > > b->bdaddr_type); > > > + hci_dev_lock(hdev); > > > continue; > > > } > > > > > > @@ -2532,23 +2582,38 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) > > > * available accept list entries in the controller, then > > > * just abort and return filer policy value to not use the > > > * accept list. > > > + * > > > + * The list may be mutated at any time outside hdev lock, > > > + * so use special iteration with copies. > > > */ > > > - list_for_each_entry(params, &hdev->pend_le_conns, action) { > > > - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); > > > + > > > + conn_params_iter_init(&hdev->pend_le_conns); > > > + > > > + while (conn_params_iter_next(&hdev->pend_le_conns, ¶ms)) { > > > + hci_dev_unlock(hdev); > > > + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); > > > if (err) > > > goto done; > > > + hci_dev_lock(hdev); > > > } > > > > > > /* After adding all new pending connections, walk through > > > * 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) { > > > - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); > > > + > > > + conn_params_iter_init(&hdev->pend_le_reports); > > > + > > > + while (conn_params_iter_next(&hdev->pend_le_reports, ¶ms)) { > > > + hci_dev_unlock(hdev); > > > + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); > > > if (err) > > > goto done; > > > + hci_dev_lock(hdev); > > > } > > > > > > + hci_dev_unlock(hdev); > > > + > > > /* Use the allowlist unless the following conditions are all true: > > > * - We are not currently suspending > > > * - There are 1 or more ADV monitors registered and it's not offloaded > > > -- > > > 2.40.1 > > > > > > > >
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 683666ea210c..e79b3831fcf3 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -822,6 +822,7 @@ struct hci_conn_params { struct hci_conn *conn; bool explicit_connect; + bool add_pending; hci_conn_flags_t flags; u8 privacy_mode; }; diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 97da5bcaa904..e6fde15dc9ca 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -2160,15 +2160,23 @@ static int hci_le_del_accept_list_sync(struct hci_dev *hdev, return 0; } +struct conn_params { + bdaddr_t addr; + u8 addr_type; + hci_conn_flags_t flags; + u8 privacy_mode; +}; + /* Adds connection to resolve list if needed. * Setting params to NULL programs local hdev->irk */ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, - struct hci_conn_params *params) + struct conn_params *params) { struct hci_cp_le_add_to_resolv_list cp; struct smp_irk *irk; struct bdaddr_list_with_irk *entry; + struct hci_conn_params *hparams; if (!use_ll_privacy(hdev)) return 0; @@ -2203,6 +2211,13 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, /* Default privacy mode is always Network */ params->privacy_mode = HCI_NETWORK_PRIVACY; + hci_dev_lock(hdev); + hparams = hci_conn_params_lookup(hdev, ¶ms->addr, + params->addr_type); + if (hparams) + hparams->privacy_mode = HCI_NETWORK_PRIVACY; + hci_dev_unlock(hdev); + done: if (hci_dev_test_flag(hdev, HCI_PRIVACY)) memcpy(cp.local_irk, hdev->irk, 16); @@ -2215,7 +2230,7 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev, /* Set Device Privacy Mode. */ static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, - struct hci_conn_params *params) + struct conn_params *params) { struct hci_cp_le_set_privacy_mode cp; struct smp_irk *irk; @@ -2249,7 +2264,7 @@ static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev, * properly set the privacy mode. */ static int hci_le_add_accept_list_sync(struct hci_dev *hdev, - struct hci_conn_params *params, + struct conn_params *params, u8 *num_entries) { struct hci_cp_le_add_to_accept_list cp; @@ -2447,6 +2462,37 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev, return __hci_cmd_sync_sk(hdev, opcode, 0, NULL, 0, HCI_CMD_TIMEOUT, sk); } +static void conn_params_iter_init(struct list_head *list) +{ + struct hci_conn_params *params; + + list_for_each_entry(params, list, action) + params->add_pending = true; +} + +static bool conn_params_iter_next(struct list_head *list, + struct conn_params *item) +{ + struct hci_conn_params *params; + + /* Must hold hdev lock. Not reentrant. Mutating list is allowed. */ + + list_for_each_entry(params, list, action) { + if (!params->add_pending) + continue; + + memcpy(&item->addr, ¶ms->addr, sizeof(params->addr)); + item->addr_type = params->addr_type; + item->privacy_mode = params->privacy_mode; + item->flags = params->flags; + + params->add_pending = false; + return true; + } + + return false; +} + /* Device must not be scanning when updating the accept list. * * Update is done using the following sequence: @@ -2466,7 +2512,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 conn_params params; struct bdaddr_list *b, *t; u8 num_entries = 0; bool pend_conn, pend_report; @@ -2494,6 +2540,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) goto done; } + hci_dev_lock(hdev); + /* Go through the current accept list programmed into the * controller one by one and check if that address is connected or is * still in the list of pending connections or list of devices to @@ -2515,8 +2563,10 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) * remove it from the acceptlist. */ if (!pend_conn && !pend_report) { + hci_dev_unlock(hdev); hci_le_del_accept_list_sync(hdev, &b->bdaddr, b->bdaddr_type); + hci_dev_lock(hdev); continue; } @@ -2532,23 +2582,38 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev) * available accept list entries in the controller, then * just abort and return filer policy value to not use the * accept list. + * + * The list may be mutated at any time outside hdev lock, + * so use special iteration with copies. */ - list_for_each_entry(params, &hdev->pend_le_conns, action) { - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); + + conn_params_iter_init(&hdev->pend_le_conns); + + while (conn_params_iter_next(&hdev->pend_le_conns, ¶ms)) { + hci_dev_unlock(hdev); + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); if (err) goto done; + hci_dev_lock(hdev); } /* After adding all new pending connections, walk through * 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) { - err = hci_le_add_accept_list_sync(hdev, params, &num_entries); + + conn_params_iter_init(&hdev->pend_le_reports); + + while (conn_params_iter_next(&hdev->pend_le_reports, ¶ms)) { + hci_dev_unlock(hdev); + err = hci_le_add_accept_list_sync(hdev, ¶ms, &num_entries); if (err) goto done; + hci_dev_lock(hdev); } + hci_dev_unlock(hdev); + /* Use the allowlist unless the following conditions are all true: * - We are not currently suspending * - There are 1 or more ADV monitors registered and it's not offloaded
hci_update_accept_list_sync iterates over hdev->pend_le_conns and hdev->pend_le_reports, and waits for controller events in the loop body, without holding hdev lock. Meanwhile, these lists and the items may be modified e.g. by le_scan_cleanup. This can invalidate the list cursor in the ongoing iterations, resulting to invalid behavior (eg use-after-free). hdev lock should be held when operating on these lists, but it cannot be held in the loop bodies here as they block. The loops are in hci_sync, so at most one loop runs at a time (per hdev). Fix this by doing the iteration in a safe way vs. list mutation, and copy data to avoid locking. Add hci_conn_params.add_pending flag to track which items are left. Lock also around hci_pend_le_action_lookup there. This fixes the following, which can be triggered at least by changing hci_le_set_cig_params to always return false, and running BlueZ iso-tester, which causes connections to be created and dropped fast: ================================================================== BUG: KASAN: slab-use-after-free in hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) Read of size 8 at addr ffff888001265018 by task kworker/u3:0/32 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014 Workqueue: hci0 hci_cmd_sync_work Call Trace: <TASK> dump_stack_lvl (./arch/x86/include/asm/irqflags.h:134 lib/dump_stack.c:107) print_report (mm/kasan/report.c:320 mm/kasan/report.c:430) ? __virt_addr_valid (./include/linux/mmzone.h:1915 ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65) ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) kasan_report (mm/kasan/report.c:538) ? hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2536 net/bluetooth/hci_sync.c:2723 net/bluetooth/hci_sync.c:2841) ? __pfx_hci_update_passive_scan_sync (net/bluetooth/hci_sync.c:2780) ? mutex_lock (kernel/locking/mutex.c:282) ? __pfx_mutex_lock (kernel/locking/mutex.c:282) ? __pfx_mutex_unlock (kernel/locking/mutex.c:538) ? __pfx_update_passive_scan_sync (net/bluetooth/hci_sync.c:2861) hci_cmd_sync_work (net/bluetooth/hci_sync.c:306) process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) ? __pfx_worker_thread (kernel/workqueue.c:2480) kthread (kernel/kthread.c:376) ? __pfx_kthread (kernel/kthread.c:331) ret_from_fork (arch/x86/entry/entry_64.S:314) </TASK> Allocated by task 31: kasan_save_stack (mm/kasan/common.c:46) kasan_set_track (mm/kasan/common.c:52) __kasan_kmalloc (mm/kasan/common.c:374 mm/kasan/common.c:383) hci_conn_params_add (./include/linux/slab.h:580 ./include/linux/slab.h:720 net/bluetooth/hci_core.c:2277) hci_connect_le_scan (net/bluetooth/hci_conn.c:1419 net/bluetooth/hci_conn.c:1589) hci_connect_cis (net/bluetooth/hci_conn.c:2266) iso_connect_cis (net/bluetooth/iso.c:390) iso_sock_connect (net/bluetooth/iso.c:899) __sys_connect (net/socket.c:2003 net/socket.c:2020) __x64_sys_connect (net/socket.c:2027) do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) Freed by task 15: kasan_save_stack (mm/kasan/common.c:46) kasan_set_track (mm/kasan/common.c:52) kasan_save_free_info (mm/kasan/generic.c:523) __kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244) __kmem_cache_free (mm/slub.c:1807 mm/slub.c:3787 mm/slub.c:3800) hci_conn_params_del (net/bluetooth/hci_core.c:2323) le_scan_cleanup (net/bluetooth/hci_conn.c:202) process_one_work (./arch/x86/include/asm/preempt.h:27 kernel/workqueue.c:2399) worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2538) kthread (kernel/kthread.c:376) ret_from_fork (arch/x86/entry/entry_64.S:314) ================================================================== Fixes: e8907f76544f ("Bluetooth: hci_sync: Make use of hci_cmd_sync_queue set 3") Signed-off-by: Pauli Virtanen <pav@iki.fi> --- Notes: It might be that even more of hci_passive_scan_sync and the routines it calls should hold hdev->lock, but don't know that right now. include/net/bluetooth/hci_core.h | 1 + net/bluetooth/hci_sync.c | 81 ++++++++++++++++++++++++++++---- 2 files changed, 74 insertions(+), 8 deletions(-)