From patchwork Sat Sep 30 12:53:32 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pauli Virtanen X-Patchwork-Id: 13405105 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 01BAEE77371 for ; Sat, 30 Sep 2023 12:53:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234010AbjI3Mxq (ORCPT ); Sat, 30 Sep 2023 08:53:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40300 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233939AbjI3Mxp (ORCPT ); Sat, 30 Sep 2023 08:53:45 -0400 Received: from meesny.iki.fi (meesny.iki.fi [IPv6:2001:67c:2b0:1c1::201]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 92F96E6 for ; Sat, 30 Sep 2023 05:53:43 -0700 (PDT) Received: from monolith.lan (91-152-120-176.elisa-laajakaista.fi [91.152.120.176]) (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 4RyRwh2l9kzyXv; Sat, 30 Sep 2023 15:53:39 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1696078420; 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; bh=/AMsXO9IMyLMxzv4Xz4AhWb5QaTAdBihj6zISEM8lVk=; b=iionRygwcN/sp3O8e4Lpir+z3zyKdiNvSQyxku8ophR+kOGlQqiROCybC0SBp+GORHZSTD 5WwgUzDIr4WUycNpBzj+IUBcA3I+LN3IrlSKyaqe0lVCIaz0wUG/WFaj8dbmiu2bhDNT2o XCcfdSNhp6YAByNzDPaRlirO2/BtFBM= ARC-Seal: i=1; s=meesny; d=iki.fi; t=1696078420; a=rsa-sha256; cv=none; b=Ixbw3tXqKJOMR/XS2wG3R1i5HJxPb+EPCBXWh3DEz6C3F4DZEB/OwzZV7DcoQxGYjRSuZv 999+ok0y70sTnQ5uWZcS7lNbNO/5QcM5iiupaROHidY5ekqALzpqYa4N1kptu5BGUht9Wd VRLe/EOYcBPY/VrrSGRrqMXzTV/zqyo= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=pav smtp.mailfrom=pav@iki.fi ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1696078420; 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; bh=/AMsXO9IMyLMxzv4Xz4AhWb5QaTAdBihj6zISEM8lVk=; b=jsY263zxdi7NpCV/QRs6l3OwiKVANsa8dwvnGUxcnUVTix3x78nrbQurSAaSIO4rt1666X hEGMY723/Dlxqvnx0LUKDebgdiYSLwwOt0kgrz7yuZMQAmbPHTZMLIgNr6x/BHKyuZjN8B W+fCfhBjVP5R3qmsf9sZBLtq4xNFobc= From: Pauli Virtanen To: linux-bluetooth@vger.kernel.org Cc: Pauli Virtanen Subject: [PATCH v2 1/2] Bluetooth: hci_sync: always check if connection is alive before deleting Date: Sat, 30 Sep 2023 15:53:32 +0300 Message-ID: <53130b4a5fb21a15f2674c336282d25ef5d2232e.1696077070.git.pav@iki.fi> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org In hci_abort_conn_sync it is possible that conn is deleted concurrently by something else, also e.g. when waiting for hdev->lock. This causes double deletion of the conn, so UAF or conn_hash.list corruption. Fix by having all code paths check that the connection is still in conn_hash before deleting it, while holding hdev->lock which prevents any races. Log (when powering off while BAP streaming, occurs rarely): ======================================================================= kernel BUG at lib/list_debug.c:56! ... ? __list_del_entry_valid (lib/list_debug.c:56) hci_conn_del (net/bluetooth/hci_conn.c:154) bluetooth hci_abort_conn_sync (net/bluetooth/hci_sync.c:5415) bluetooth ? __pfx_hci_abort_conn_sync+0x10/0x10 [bluetooth] ? lock_release+0x1d5/0x3c0 ? hci_disconnect_all_sync.constprop.0+0xb2/0x230 [bluetooth] ? __pfx_lock_release+0x10/0x10 ? __kmem_cache_free+0x14d/0x2e0 hci_disconnect_all_sync.constprop.0+0xda/0x230 [bluetooth] ? __pfx_hci_disconnect_all_sync.constprop.0+0x10/0x10 [bluetooth] ? hci_clear_adv_sync+0x14f/0x170 [bluetooth] ? __pfx_set_powered_sync+0x10/0x10 [bluetooth] hci_set_powered_sync+0x293/0x450 [bluetooth] ======================================================================= Fixes: 94d9ba9f9888 ("Bluetooth: hci_sync: Fix UAF in hci_disconnect_all_sync") Signed-off-by: Pauli Virtanen --- Notes: v2: rebased net/bluetooth/hci_sync.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 3640d73f9595..c6f57af88bfd 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -5380,6 +5380,7 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) { int err = 0; u16 handle = conn->handle; + bool disconnect = false; struct hci_conn *c; switch (conn->state) { @@ -5395,24 +5396,15 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) break; case BT_OPEN: case BT_BOUND: - hci_dev_lock(hdev); - hci_conn_failed(conn, reason); - hci_dev_unlock(hdev); - return 0; + break; default: - hci_dev_lock(hdev); - conn->state = BT_CLOSED; - hci_disconn_cfm(conn, reason); - hci_conn_del(conn); - hci_dev_unlock(hdev); - return 0; + disconnect = true; + break; } hci_dev_lock(hdev); - /* Check if the connection hasn't been cleanup while waiting - * commands to complete. - */ + /* Check if the connection has been cleaned up concurrently */ c = hci_conn_hash_lookup_handle(hdev, handle); if (!c || c != conn) { err = 0; @@ -5424,7 +5416,13 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason) * or in case of LE it was still scanning so it can be cleanup * safely. */ - hci_conn_failed(conn, reason); + if (disconnect) { + conn->state = BT_CLOSED; + hci_disconn_cfm(conn, reason); + hci_conn_del(conn); + } else { + hci_conn_failed(conn, reason); + } unlock: hci_dev_unlock(hdev); From patchwork Sat Sep 30 12:53:33 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pauli Virtanen X-Patchwork-Id: 13405106 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 48672E77372 for ; Sat, 30 Sep 2023 12:53:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234049AbjI3Mxq (ORCPT ); Sat, 30 Sep 2023 08:53:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40302 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231448AbjI3Mxp (ORCPT ); Sat, 30 Sep 2023 08:53:45 -0400 Received: from meesny.iki.fi (meesny.iki.fi [IPv6:2001:67c:2b0:1c1::201]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 93727F1 for ; Sat, 30 Sep 2023 05:53:43 -0700 (PDT) Received: from monolith.lan (91-152-120-176.elisa-laajakaista.fi [91.152.120.176]) (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 4RyRwj0ZTYzydx; Sat, 30 Sep 2023 15:53:41 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1696078421; 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=m0wjHwVlU1if1y3UnlVFwCUKcy5xWVCk+uJEVHd1moE=; b=mi3yaSuU1hZHLRyF1nQaCrUfFyYIRc3BjGfqh+g3939tAtl7Sh5/jMYZLooPV9R48TW8vb lYF3vfLovsJpcC9h8+EZjyV5stXngTvzy77vncUU6fK7+ShLDsTom8cxwxsSZGIL9qcKwa 3diGvS8oA/iscgirfwLpgsmeLalxF6k= ARC-Seal: i=1; s=meesny; d=iki.fi; t=1696078421; a=rsa-sha256; cv=none; b=ReDjfgfnCzWdTBkKWfg9mmQZOPrgGPeyHA9ER0p5zj266D7iUbB7skb+YEGAeWcCEQc6zV J6DxRhLOsfRnb/LSXVpczl7gdw2pPr0qsBp8yMhvlfO5xIgFCxq9/Xpmn25KGqWfdX08Jm p4WwfPQq01Yllg3OPbSFUC0lg8OZQRk= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=pav smtp.mailfrom=pav@iki.fi ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1696078421; 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=m0wjHwVlU1if1y3UnlVFwCUKcy5xWVCk+uJEVHd1moE=; b=YrXN2sp0PU3k1AhWo8gxMiIC7JoCA6JRcReOAxNLCSBjM39xMlzZ+xqMZj/hnPUJS1esiQ SjQjupJaUnf2somGKGL/t/yrKGrkjq7LYqMdoN2FGkrCA7c1cu9TNqH0ITX5bArEI5VQZs KpxbFdvDWjrL518xcQ8RCX5a8XbIUJo= From: Pauli Virtanen To: linux-bluetooth@vger.kernel.org Cc: Pauli Virtanen , syzkaller-bugs@googlegroups.com, syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com Subject: [PATCH v2 2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it Date: Sat, 30 Sep 2023 15:53:33 +0300 Message-ID: X-Mailer: git-send-email 2.41.0 In-Reply-To: <53130b4a5fb21a15f2674c336282d25ef5d2232e.1696077070.git.pav@iki.fi> References: <53130b4a5fb21a15f2674c336282d25ef5d2232e.1696077070.git.pav@iki.fi> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org There is a race condition where a connection handle is reused, after hci_abort_conn but before abort_conn_sync is processed in hci_sync. In this case, hci_abort_conn_sync ends up working on the wrong connection, which can have abort_reason==0, in which case hci_connect_cfm is called with success status and then connection is deleted, which causes various use-after-free. Fix by checking abort_reason is nonzero before calling hci_abort_conn_sync. If it's zero, then the connection is probably a different one than we expected and should not be aborted. Also fix some theoretical UAF / races, where something frees the conn while hci_abort_conn_sync is working on it. Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections") Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@google.com/ Signed-off-by: Pauli Virtanen --- Notes: v2: drop one probably not necessary WARN_ON Not sure how you'd hit this condition in real controller, but syzbot does end up calling hci_abort_conn_sync with reason == 0 which then causes havoc. This can be verified: with a patch that changes abort_conn_sync to 2874 conn = hci_conn_hash_lookup_handle(hdev, handle); 2875 if (!conn || WARN_ON(!conn->abort_reason)) 2876 return 0; https://syzkaller.appspot.com/text?tag=Patch&x=16eff740680000 it hits that WARN_ON: https://syzkaller.appspot.com/x/log.txt?x=10affb97a80000 #syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master net/bluetooth/hci_conn.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index e62a5f368a51..22de82071e35 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -2901,12 +2901,25 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data) { struct hci_conn *conn; u16 handle = PTR_UINT(data); + u8 reason; + int err; + + rcu_read_lock(); conn = hci_conn_hash_lookup_handle(hdev, handle); + if (conn) { + reason = READ_ONCE(conn->abort_reason); + conn = reason ? hci_conn_get(conn) : NULL; + } + + rcu_read_unlock(); + if (!conn) return 0; - return hci_abort_conn_sync(hdev, conn, conn->abort_reason); + err = hci_abort_conn_sync(hdev, conn, reason); + hci_conn_put(conn); + return err; } int hci_abort_conn(struct hci_conn *conn, u8 reason) @@ -2918,6 +2931,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason) */ if (conn->abort_reason) return 0; + if (WARN_ON(!reason)) + reason = HCI_ERROR_UNSPECIFIED; bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);