[v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting

Commit Message

Pauli Virtanen Sept. 30, 2023, 12:53 p.m. UTC

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 <[e-mail]>
---

Notes:
    v2: rebased

 net/bluetooth/hci_sync.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Comments

[e-mail] Sept. 30, 2023, 1:36 p.m. UTC | #1

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=789014

---Test result---

Test Summary:
CheckPatch                    PASS      1.53 seconds
GitLint                       FAIL      0.86 seconds
SubjectPrefix                 PASS      0.20 seconds
BuildKernel                   PASS      33.69 seconds
CheckAllWarning               PASS      36.16 seconds
CheckSparse                   PASS      41.46 seconds
CheckSmatch                   PASS      115.45 seconds
BuildKernel32                 PASS      31.93 seconds
TestRunnerSetup               PASS      489.12 seconds
TestRunner_l2cap-tester       PASS      30.32 seconds
TestRunner_iso-tester         PASS      49.69 seconds
TestRunner_bnep-tester        PASS      9.79 seconds
TestRunner_mgmt-tester        PASS      214.41 seconds
TestRunner_rfcomm-tester      PASS      14.96 seconds
TestRunner_sco-tester         PASS      18.53 seconds
TestRunner_ioctl-tester       PASS      16.93 seconds
TestRunner_mesh-tester        PASS      12.59 seconds
TestRunner_smp-tester         PASS      13.38 seconds
TestRunner_userchan-tester    PASS      10.31 seconds
IncrementalBuild              PASS      37.23 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting

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
1: T1 Title exceeds max length (81>80): "[v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting"
[v2,2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it

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
18: B1 Line exceeds max length (88>80): "Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/"
31: B3 Line contains hard tab characters (\t): "        2874	conn = hci_conn_hash_lookup_handle(hdev, handle);"
32: B3 Line contains hard tab characters (\t): "        2875	if (!conn || WARN_ON(!conn->abort_reason))"
33: B3 Line contains hard tab characters (\t): "        2876		return 0;"


---
Regards,
Linux Bluetooth

[e-mail] Oct. 3, 2023, 5:10 p.m. UTC | #2

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Sat, 30 Sep 2023 15:53:32 +0300 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting
    https://git.kernel.org/bluetooth/bluetooth-next/c/32f6776f0083
  - [v2,2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it
    (no matching commit)

You are awesome, thank you!

Patch

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);