From patchwork Thu Jun 8 21:06:42 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pauli Virtanen X-Patchwork-Id: 690628 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 789FFC7EE29 for ; Thu, 8 Jun 2023 21:06:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236986AbjFHVGz (ORCPT ); Thu, 8 Jun 2023 17:06:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57424 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236968AbjFHVGy (ORCPT ); Thu, 8 Jun 2023 17:06:54 -0400 Received: from meesny.iki.fi (meesny.iki.fi [IPv6:2001:67c:2b0:1c1::201]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0505930C8 for ; Thu, 8 Jun 2023 14:06:50 -0700 (PDT) Received: from monolith.lan (91-152-120-101.elisa-laajakaista.fi [91.152.120.101]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: pav) by meesny.iki.fi (Postfix) with ESMTPSA id 4QccGH65Gdzykm; Fri, 9 Jun 2023 00:06:47 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1686258407; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AxQu0jI+AcZazHydptOdbBREFjCcYuiV/09/2+1LXbY=; b=L1gRwKr0VubmvoRGTbqsApDy+nXwMKoSKcS+8gbgCMtsOTGBaAOJs8Er0Ng+tSCy+NdF2L RStYEYKhHfH1oNi92ZhRk6epq72YANTZU+rMi0cQ6okgwarx4HaMeDzIXgb4eXNjFjHLju 2hEnU8EZmfsWRO3uffyRs/f0nzpvLAg= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1686258407; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AxQu0jI+AcZazHydptOdbBREFjCcYuiV/09/2+1LXbY=; b=dL2jPyaut99T0Mi+yyGB0xGW8jxf1OnCRYW4NxDyxqH024MdIX3hep6zEX/gGpXy4q6Upk n+qk6Ir2M9J/8lw0oDH4RuscWHFIv/lji+DK71WMStxTit41s5tOtCWQ5TpBgTQ8pLzZ9Y IWTCbBclQZWPR6qZfo9f4KSoZ+2ib+Y= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=pav smtp.mailfrom=pav@iki.fi ARC-Seal: i=1; s=meesny; d=iki.fi; t=1686258407; a=rsa-sha256; cv=none; b=Lpk1daYRhV6li85NFrHpFRdewmwvuP7UpdPUCeWcJ2y4OiTkRRgZ6xZPyrZQGEmz264cFE ch0JSxfvsa+GmKtQoERk0IVau3+TtdX9LDhayVW4PqOowG7Ls3ZZHd26RVgmUZstk+l92E LvcH4HSP5w0DseZDQwW5UmAdeWYklBc= From: Pauli Virtanen To: linux-bluetooth@vger.kernel.org Cc: Pauli Virtanen Subject: [PATCH 1/3] Bluetooth: hci_sync: iterate over hci_conn_params safely Date: Fri, 9 Jun 2023 00:06:42 +0300 Message-Id: X-Mailer: git-send-email 2.40.1 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org 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: 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) 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 --- 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; +} + /* 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 From patchwork Thu Jun 8 21:06:43 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pauli Virtanen X-Patchwork-Id: 690987 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF37CC7EE43 for ; Thu, 8 Jun 2023 21:06:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236987AbjFHVG5 (ORCPT ); Thu, 8 Jun 2023 17:06:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57422 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236909AbjFHVGy (ORCPT ); Thu, 8 Jun 2023 17:06:54 -0400 Received: from meesny.iki.fi (meesny.iki.fi [IPv6:2001:67c:2b0:1c1::201]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 046F62D7E for ; Thu, 8 Jun 2023 14:06:50 -0700 (PDT) Received: from monolith.lan (91-152-120-101.elisa-laajakaista.fi [91.152.120.101]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: pav) by meesny.iki.fi (Postfix) with ESMTPSA id 4QccGJ2nYGz10LN; Fri, 9 Jun 2023 00:06:48 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1686258408; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UAF2j62o62g4jxJcr8tw9/1yrhub+sdCrQ4qdE9Qg6g=; b=b0N5+gdnE/AOBwAzrAutUA5HvVI64AlbIXaxeRqcN6bbZQ5m10Z+I/1qGMOHs1Me+H/Jx0 GhbJ9gJhvJkppSdnFPTnX8JB1r28/Uc+t2nc6+fpWIgjm0xek8ZRNidHAw0D1My14JE28i Vg+0Z+dgTTw+pT7+w0f1N64+g0v2RHg= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1686258408; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UAF2j62o62g4jxJcr8tw9/1yrhub+sdCrQ4qdE9Qg6g=; b=S0lEIwkCIE4Y2z0NBqFMr9rJzcjanw04a5pdc0U1gQT5cEDGsgcAXiyVFKmzkim9Pm7+oC HclxgDxn2/1uWt8ZVFEGWkqZLQCEWt5gOj7H7Z4RVkhf7iks/NqRVjy4CK2VoNxJDOG2rt 2086b3zBV6LLkQ1VRJlPrC8q2jd0BPc= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=pav smtp.mailfrom=pav@iki.fi ARC-Seal: i=1; s=meesny; d=iki.fi; t=1686258408; a=rsa-sha256; cv=none; b=UfKzAKnTA98c6TUYEpczt4/vCnaGc3ZaPCPNJUlxgcHMb/VxsS7pjJ5BrY7ukEmi4QvGuv UFXjPZftWmbpmeYVu+SnLVXlaubF5uVQL0Nw+faWxT/RMsNDOz93MGiJkNJHsUznKL/pAK oyybWfgz4rDEL/imKyo/cYD37up6q8M= From: Pauli Virtanen To: linux-bluetooth@vger.kernel.org Cc: Pauli Virtanen Subject: [PATCH 2/3] Bluetooth: hci_event: call ISO disconnect callback before deleting conn Date: Fri, 9 Jun 2023 00:06:43 +0300 Message-Id: <7dd7bff24c3f6765728aec12b2e96be792d8f6d7.1686252492.git.pav@iki.fi> X-Mailer: git-send-email 2.40.1 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org 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 ... ... 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 --- 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; } From patchwork Thu Jun 8 21:06:44 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pauli Virtanen X-Patchwork-Id: 690988 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 08A98C7EE25 for ; Thu, 8 Jun 2023 21:06:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236982AbjFHVG4 (ORCPT ); Thu, 8 Jun 2023 17:06:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57420 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236935AbjFHVGy (ORCPT ); Thu, 8 Jun 2023 17:06:54 -0400 Received: from meesny.iki.fi (meesny.iki.fi [IPv6:2001:67c:2b0:1c1::201]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 04F7F30C1 for ; Thu, 8 Jun 2023 14:06:50 -0700 (PDT) Received: from monolith.lan (91-152-120-101.elisa-laajakaista.fi [91.152.120.101]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: pav) by meesny.iki.fi (Postfix) with ESMTPSA id 4QccGJ3Mh2z10MD; Fri, 9 Jun 2023 00:06:48 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1686258408; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qrs4q5qgVBu4pnZE4HY+ySu9EXbDTYqE7CgsUDti+e4=; b=mRhwnqXUC3Tar6kpPoNOuMpinC4G+zZQy1Q2hm1E2YswBLhC7H/IWu4kJ287G2jzCWsCpf NcpUKZUjmp+Roq2Fl12MHrREyJ6AYgz4dHSuDpwRsagNrAvp6gvNkgcNb4GhBEj4gUxR9q w/mOWAk6Q2aZoZVhViKU3/cE2wQtvvc= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1686258408; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qrs4q5qgVBu4pnZE4HY+ySu9EXbDTYqE7CgsUDti+e4=; b=kTCDo8nl4PuoBlZsErC4VikbV7GM26oPOer9vrHOtQQ67rl0/myS2AtsiAwl/MKeq4L0Uw oHSqmFjn7bSMySwBgNRP7FYeH4iwhSKxa2LjxrlcJYs770n+1qp2hK2VB0YdrQWML6GeZ0 64+DvEzidgU3uQYAKGSd26KND2SUZ88= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=pav smtp.mailfrom=pav@iki.fi ARC-Seal: i=1; s=meesny; d=iki.fi; t=1686258408; a=rsa-sha256; cv=none; b=MLmTqeilg+ZI/8hCpHk0ekZI1yt/sI8rlfRTJ1c1Hp1Cu0rKfPKXboXOO77htsI0Owy90P 6yaaoyi+vC9PHeiXGuLNgG6x6wPRo7W0f7CzfH/2q2fBYWkK1KNpa/tY8K5OO9VOU0spns g8pQlGN7L50jXPrvn/NLB8HdNY4I8Gk= From: Pauli Virtanen To: linux-bluetooth@vger.kernel.org Cc: Pauli Virtanen Subject: [PATCH 3/3] Bluetooth: ISO: fix iso_conn related locking and validity issues Date: Fri, 9 Jun 2023 00:06:44 +0300 Message-Id: X-Mailer: git-send-email 2.40.1 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org 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 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 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 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: 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 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 --- 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; } }