[PATCH 0/3] Bluetooth: ISO-related concurrency fixes

[PATCH 0/3] Bluetooth: ISO-related concurrency fixes

[Pauli Virtanen]

This series addresses some concurrency issues (NULL / GPF) in ISO
sockets or related.

These were found while testing patches that make hci_le_set_cig_params
check the validity of the configuration and return false if incorrect.
This causes dropping of hci_conn just created, which apparently makes
hitting race conditions easier.

The test setup was primitive

while true; do bluetoothctl power on; sleep 12; bluetoothctl power off; sleep 1.5; bluetoothctl power off; sleep 2.5; done;
while true; do sudo systemctl restart bluetooth; sleep 110; done
while true; do systemctl --user restart pipewire wireplumber pipewire-pulse; sleep 91; done
while true; do paplay sample.flac & sleep 2; kill %1; sleep 0.7; done

and equivalent operations manually, on VM + connect to TWS earbuds. This
eventually hit the NULL / GFP errors here, but they are hard to
reproduce aside from the first one that appears in iso-tester.

This also produces a few other types of crashes / KASAN errors, but not
addressed here.

Pauli Virtanen (3):
  Bluetooth: hci_sync: iterate over hci_conn_params safely
  Bluetooth: hci_event: call ISO disconnect callback before deleting
    conn
  Bluetooth: ISO: fix iso_conn related locking and validity issues

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_event.c        |  6 +++
 net/bluetooth/hci_sync.c         | 81 ++++++++++++++++++++++++++++----
 net/bluetooth/iso.c              | 53 ++++++++++++---------
 4 files changed, 111 insertions(+), 30 deletions(-)

-- 
2.40.1

[PATCH 1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely

[Pauli Virtanen]

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, &params->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, &params->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, &params)) {
+		hci_dev_unlock(hdev);
+		err = hci_le_add_accept_list_sync(hdev, &params, &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, &params)) {
+		hci_dev_unlock(hdev);
+		err = hci_le_add_accept_list_sync(hdev, &params, &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

RE: Bluetooth: ISO-related concurrency fixes

[bluez.test.bot]

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

Re: [PATCH 1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely

[Luiz Augusto von Dentz]

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, &params->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, &params->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, &params)) {
> +               hci_dev_unlock(hdev);
> +               err = hci_le_add_accept_list_sync(hdev, &params, &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, &params)) {
> +               hci_dev_unlock(hdev);
> +               err = hci_le_add_accept_list_sync(hdev, &params, &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
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely

[Pauli Virtanen]

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, &params->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, &params->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, &params)) {
> > +               hci_dev_unlock(hdev);
> > +               err = hci_le_add_accept_list_sync(hdev, &params, &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, &params)) {
> > +               hci_dev_unlock(hdev);
> > +               err = hci_le_add_accept_list_sync(hdev, &params, &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
> > 
> 
> 

Re: [PATCH 1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely

[Luiz Augusto von Dentz]

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, &params->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, &params->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, &params)) {
> > > +               hci_dev_unlock(hdev);
> > > +               err = hci_le_add_accept_list_sync(hdev, &params, &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, &params)) {
> > > +               hci_dev_unlock(hdev);
> > > +               err = hci_le_add_accept_list_sync(hdev, &params, &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
> > >
> >
> >
>


-- 
Luiz Augusto von Dentz

[PATCH 2/3] Bluetooth: hci_event: call ISO disconnect callback before deleting conn

[Pauli Virtanen]

In hci_cs_disconnect, we do hci_conn_del even if disconnection failed.

When this occurs for ISO connections, which refer to the conn without
hci_conn_get, disconn_cfm must be called otherwise use-after-free
occurs.

Currently ISO socket Disconnect often fails because we try disconnect
them after disconnecting ACL when they are already implicitly
disconnected (Core v5.3 Vol 4 Part E Sec 7.1.6).

Trace from logs:
==========================================================
iso_sock_connect:880: sk 00000000eabd6557
iso_connect_cis:356: 70:1a:b8:98:ff:a2 -> 28:3d:c2:4a:7e:da
...
iso_conn_add:140: hcon 000000001696f1fd conn 00000000b6251073
hci_dev_put:1487: hci0 orig refcnt 17
__iso_chan_add:214: conn 00000000b6251073
iso_sock_clear_timer:117: sock 00000000eabd6557 state 3
...
hci_rx_work:4085: hci0 Event packet
hci_event_packet:7601: hci0: event 0x0f
hci_cmd_status_evt:4346: hci0: opcode 0x0406
hci_cs_disconnect:2760: hci0: status 0x0c
hci_sent_cmd_data:3107: hci0 opcode 0x0406
hci_conn_del:1151: hci0 hcon 000000001696f1fd handle 2560
hci_conn_unlink:1102: hci0: hcon 000000001696f1fd
hci_conn_drop:1451: hcon 00000000d8521aaf orig refcnt 2
hci_chan_list_flush:2780: hcon 000000001696f1fd
hci_dev_put:1487: hci0 orig refcnt 21
hci_dev_put:1487: hci0 orig refcnt 20
hci_req_cmd_complete:3978: opcode 0x0406 status 0x0c
... <no iso_* activity on sk/conn> ...
iso_sock_sendmsg:1098: sock 00000000dea5e2e0, sk 00000000eabd6557
BUG: kernel NULL pointer dereference, address: 0000000000000668
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP PTI
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
RIP: 0010:iso_sock_sendmsg (net/bluetooth/iso.c:1112) bluetooth
==========================================================

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    This might be necessary for all socket types, not sure about that yet.

 net/bluetooth/hci_event.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 7c199f7361f7..fb80923bf965 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2784,6 +2784,12 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
 			hci_enable_advertising(hdev);
 		}
 
+		/* Tell ISO sockets the conn went away, before we delete it,
+		 * because they do not hold reference.
+		 */
+		if (conn->type == ISO_LINK)
+			hci_disconn_cfm(conn, HCI_ERROR_LOCAL_HOST_TERM);
+
 		goto done;
 	}
 
-- 
2.40.1

[PATCH 3/3] Bluetooth: ISO: fix iso_conn related locking and validity issues

[Pauli Virtanen]

sk->sk_state indicates whether iso_pi(sk)->conn is valid. Operations
that check/update sk_state and access conn should hold lock_sock,
otherwise they can race.

The order of taking locks is hci_dev_lock > lock_sock > iso_conn_lock,
which is how it is in connect/disconnect_cfm -> iso_conn_del ->
iso_chan_del.

Fix locking in iso_connect_cis/bis and sendmsg/recvmsg to take lock_sock
around updating sk_state and conn.

iso_conn_del must not occur during iso_connect_cis/bis, as it frees the
iso_conn. Hold hdev->lock longer to prevent that.

This should not reintroduce the issue fixed in commit 241f51931c35
("Bluetooth: ISO: Avoid circular locking dependency"), since the we
acquire locks in order. We retain the fix in iso_sock_connect to release
lock_sock before iso_connect_* acquires hdev->lock.

Similarly for commit 6a5ad251b7cd ("Bluetooth: ISO: Fix possible
circular locking dependency"). We retain the fix in iso_conn_ready to
not acquire iso_conn_lock before lock_sock.

iso_conn_add shall return iso_conn with valid hcon. Make it so also when
reusing an old CIS connection waiting for disconnect timeout (see
__iso_sock_close where conn->hcon is set to NULL).

Trace with iso_conn_del after iso_chan_add in iso_connect_cis:
===============================================================
iso_sock_create:771: sock 00000000be9b69b7
iso_sock_init:693: sk 000000004dff667e
iso_sock_bind:827: sk 000000004dff667e 70:1a:b8:98:ff:a2 type 1
iso_sock_setsockopt:1289: sk 000000004dff667e
iso_sock_setsockopt:1289: sk 000000004dff667e
iso_sock_setsockopt:1289: sk 000000004dff667e
iso_sock_connect:875: sk 000000004dff667e
iso_connect_cis:353: 70:1a:b8:98:ff:a2 -> 28:3d:c2:4a:7e:da
hci_get_route:1199: 70:1a:b8:98:ff:a2 -> 28:3d:c2:4a:7e:da
hci_conn_add:1005: hci0 dst 28:3d:c2:4a:7e:da
iso_conn_add:140: hcon 000000007b65d182 conn 00000000daf8625e
__iso_chan_add:214: conn 00000000daf8625e
iso_connect_cfm:1700: hcon 000000007b65d182 bdaddr 28:3d:c2:4a:7e:da status 12
iso_conn_del:187: hcon 000000007b65d182 conn 00000000daf8625e, err 16
iso_sock_clear_timer:117: sock 000000004dff667e state 3
    <Note: sk_state is BT_BOUND (3), so iso_connect_cis is still
    running at this point>
iso_chan_del:153: sk 000000004dff667e, conn 00000000daf8625e, err 16
hci_conn_del:1151: hci0 hcon 000000007b65d182 handle 65535
hci_conn_unlink:1102: hci0: hcon 000000007b65d182
hci_chan_list_flush:2780: hcon 000000007b65d182
iso_sock_getsockopt:1376: sk 000000004dff667e
iso_sock_getname:1070: sock 00000000be9b69b7, sk 000000004dff667e
iso_sock_getname:1070: sock 00000000be9b69b7, sk 000000004dff667e
iso_sock_getsockopt:1376: sk 000000004dff667e
iso_sock_getname:1070: sock 00000000be9b69b7, sk 000000004dff667e
iso_sock_getname:1070: sock 00000000be9b69b7, sk 000000004dff667e
iso_sock_shutdown:1434: sock 00000000be9b69b7, sk 000000004dff667e, how 1
__iso_sock_close:632: sk 000000004dff667e state 5 socket 00000000be9b69b7
     <Note: sk_state is BT_CONNECT (5), even though iso_chan_del sets
     BT_CLOSED (6). Only iso_connect_cis sets it to BT_CONNECT, so it
     must be that iso_chan_del occurred between iso_chan_add and end of
     iso_connect_cis.>
BUG: kernel NULL pointer dereference, address: 0000000000000000
PGD 8000000006467067 P4D 8000000006467067 PUD 3f5f067 PMD 0
Oops: 0000 [#1] PREEMPT SMP PTI
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
RIP: 0010:__iso_sock_close (net/bluetooth/iso.c:664) bluetooth
===============================================================

Trace with iso_conn_del before iso_chan_add in iso_connect_cis:
===============================================================
iso_connect_cis:356: 70:1a:b8:98:ff:a2 -> 28:3d:c2:4a:7e:da
...
iso_conn_add:140: hcon 0000000093bc551f conn 00000000768ae504
hci_dev_put:1487: hci0 orig refcnt 21
hci_event_packet:7607: hci0: event 0x0e
hci_cmd_complete_evt:4231: hci0: opcode 0x2062
hci_cc_le_set_cig_params:3846: hci0: status 0x07
hci_sent_cmd_data:3107: hci0 opcode 0x2062
iso_connect_cfm:1703: hcon 0000000093bc551f bdaddr 28:3d:c2:4a:7e:da status 7
iso_conn_del:187: hcon 0000000093bc551f conn 00000000768ae504, err 12
hci_conn_del:1151: hci0 hcon 0000000093bc551f handle 65535
hci_conn_unlink:1102: hci0: hcon 0000000093bc551f
hci_chan_list_flush:2780: hcon 0000000093bc551f
__iso_chan_add:214: conn 00000000768ae504
    <Note: this conn was already freed in iso_conn_del above>
iso_sock_clear_timer:117: sock 0000000098323f95 state 3
general protection fault, probably for non-canonical address 0x30b29c630930aec8: 0000 [#1] PREEMPT SMP PTI
CPU: 1 PID: 1920 Comm: bluetoothd Tainted: G            E      6.3.0-rc7+ #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
RIP: 0010:detach_if_pending+0x28/0xd0
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 <>
RSP: 0018:ffffb90841a67d08 EFLAGS: 00010007
RAX: 0000000000000000 RBX: ffff9141bd5061b8 RCX: 0000000000000000
RDX: 30b29c630930aec8 RSI: ffff9141fdd21e80 RDI: ffff9141bd5061b8
RBP: 0000000000000001 R08: 0000000000000000 R09: ffffb90841a67b88
R10: 0000000000000003 R11: ffffffff8613f558 R12: ffff9141fdd21e80
R13: 0000000000000000 R14: ffff9141b5976010 R15: ffff914185755338
FS:  00007f45768bd840(0000) GS:ffff9141fdd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000619000424074 CR3: 0000000009f5e005 CR4: 0000000000170ee0
Call Trace:
 <TASK>
 timer_delete+0x48/0x80
 try_to_grab_pending+0xdf/0x170
 __cancel_work+0x37/0xb0
 iso_connect_cis+0x141/0x400 [bluetooth]
===============================================================

Trace with NULL conn->hcon in state BT_CONNECT:
===============================================================
__iso_sock_close:619: sk 00000000f7c71fc5 state 1 socket 00000000d90c5fe5
...
__iso_sock_close:619: sk 00000000f7c71fc5 state 8 socket 00000000d90c5fe5
iso_chan_del:153: sk 00000000f7c71fc5, conn 0000000022c03a7e, err 104
...
iso_sock_connect:862: sk 00000000129b56c3
iso_connect_cis:348: 70:1a:b8:98:ff:a2 -> 28:3d:c2:4a:7d:2a
hci_get_route:1199: 70:1a:b8:98:ff:a2 -> 28:3d:c2:4a:7d:2a
hci_dev_hold:1495: hci0 orig refcnt 19
__iso_chan_add:214: conn 0000000022c03a7e
    <Note: reusing old conn>
iso_sock_clear_timer:117: sock 00000000129b56c3 state 3
...
iso_sock_ready:1485: sk 00000000129b56c3
...
iso_sock_sendmsg:1077: sock 00000000e5013966, sk 00000000129b56c3
BUG: kernel NULL pointer dereference, address: 00000000000006a8
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP PTI
CPU: 1 PID: 1403 Comm: wireplumber Tainted: G            E      6.3.0-rc7+ #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc38 04/01/2014
RIP: 0010:iso_sock_sendmsg+0x63/0x2a0 [bluetooth]
===============================================================

Fixes: 241f51931c35 ("Bluetooth: ISO: Avoid circular locking dependency")
Fixes: 6a5ad251b7cd ("Bluetooth: ISO: Fix possible circular locking dependency")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    Don't have a clean reproducer, but these occur sometimes on
    bluetooth-next/master when sending data over ISO and disconnecting +
    reconnecting immediately, although the timing window required for these
    to occur is not so wide.
    
    These appear to be somewhat more easily reproduced if
    hci_le_set_cig_params is changed to return errors if CIG is busy.
    Enabling printks also seems to help.

 net/bluetooth/iso.c | 53 ++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index b9a008fd10b1..8f570ab66ae7 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -123,8 +123,11 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
 {
 	struct iso_conn *conn = hcon->iso_data;
 
-	if (conn)
+	if (conn) {
+		if (!conn->hcon)
+			conn->hcon = hcon;
 		return conn;
+	}
 
 	conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 	if (!conn)
@@ -311,14 +314,13 @@ static int iso_connect_bis(struct sock *sk)
 		goto unlock;
 	}
 
-	hci_dev_unlock(hdev);
-	hci_dev_put(hdev);
+	lock_sock(sk);
 
 	err = iso_chan_add(conn, sk, NULL);
-	if (err)
-		return err;
-
-	lock_sock(sk);
+	if (err) {
+		release_sock(sk);
+		goto unlock;
+	}
 
 	/* Update source addr of the socket */
 	bacpy(&iso_pi(sk)->src, &hcon->src);
@@ -335,7 +337,6 @@ static int iso_connect_bis(struct sock *sk)
 	}
 
 	release_sock(sk);
-	return err;
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -403,14 +404,13 @@ static int iso_connect_cis(struct sock *sk)
 		goto unlock;
 	}
 
-	hci_dev_unlock(hdev);
-	hci_dev_put(hdev);
+	lock_sock(sk);
 
 	err = iso_chan_add(conn, sk, NULL);
-	if (err)
-		return err;
-
-	lock_sock(sk);
+	if (err) {
+		release_sock(sk);
+		goto unlock;
+	}
 
 	/* Update source addr of the socket */
 	bacpy(&iso_pi(sk)->src, &hcon->src);
@@ -427,7 +427,6 @@ static int iso_connect_cis(struct sock *sk)
 	}
 
 	release_sock(sk);
-	return err;
 
 unlock:
 	hci_dev_unlock(hdev);
@@ -1078,8 +1077,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 			    size_t len)
 {
 	struct sock *sk = sock->sk;
-	struct iso_conn *conn = iso_pi(sk)->conn;
 	struct sk_buff *skb, **frag;
+	size_t mtu;
 	int err;
 
 	BT_DBG("sock %p, sk %p", sock, sk);
@@ -1091,11 +1090,18 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (msg->msg_flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
-	if (sk->sk_state != BT_CONNECTED)
+	lock_sock(sk);
+
+	if (sk->sk_state != BT_CONNECTED) {
+		release_sock(sk);
 		return -ENOTCONN;
+	}
+
+	mtu = iso_pi(sk)->conn->hcon->hdev->iso_mtu;
+
+	release_sock(sk);
 
-	skb = bt_skb_sendmsg(sk, msg, len, conn->hcon->hdev->iso_mtu,
-			     HCI_ISO_DATA_HDR_SIZE, 0);
+	skb = bt_skb_sendmsg(sk, msg, len, mtu, HCI_ISO_DATA_HDR_SIZE, 0);
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
 
@@ -1108,8 +1114,7 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	while (len) {
 		struct sk_buff *tmp;
 
-		tmp = bt_skb_sendmsg(sk, msg, len, conn->hcon->hdev->iso_mtu,
-				     0, 0);
+		tmp = bt_skb_sendmsg(sk, msg, len, mtu, 0, 0);
 		if (IS_ERR(tmp)) {
 			kfree_skb(skb);
 			return PTR_ERR(tmp);
@@ -1164,15 +1169,19 @@ static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	BT_DBG("sk %p", sk);
 
 	if (test_and_clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
+		lock_sock(sk);
 		switch (sk->sk_state) {
 		case BT_CONNECT2:
-			lock_sock(sk);
 			iso_conn_defer_accept(pi->conn->hcon);
 			sk->sk_state = BT_CONFIG;
 			release_sock(sk);
 			return 0;
 		case BT_CONNECT:
+			release_sock(sk);
 			return iso_connect_cis(sk);
+		default:
+			release_sock(sk);
+			break;
 		}
 	}
 
-- 
2.40.1