Bluetooth: hci_conn: fail SCO/ISO via hci_conn_failed if ACL gone early

Commit Message

Pauli Virtanen Aug. 19, 2023, 1:33 p.m. UTC

Not calling hci_(dis)connect_cfm before deleting conn referred to by a
socket generally results to use-after-free.

When cleaning up SCO connections when the parent ACL is deleted too
early, use hci_conn_failed to do the connection cleanup properly.

We also need to clean up ISO connections in a similar situation when
connecting has started but LE Create CIS is not yet sent, so do it too
here.

Fixes: ca1fd42e7dbf ("Bluetooth: Fix potential double free caused by hci_conn_unlink")
Reported-by: [e-mail]
Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
Signed-off-by: Pauli Virtanen <[e-mail]>
---

Notes:
    This makes BlueZ test cases pass (and should fix syzbot crash):

    ISO Connect ACL Disconnect - Failure                 Passed       1.004 seconds
    eSCO ACL Disconnect - Failure                        Passed       0.987 seconds

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git f0835e7404b7f6fd825fc1ad7a174253a54234cf

 net/bluetooth/hci_conn.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Comments

[e-mail] Aug. 19, 2023, 2:01 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=777636

---Test result---

Test Summary:
CheckPatch                    PASS      0.71 seconds
GitLint                       FAIL      0.55 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      32.33 seconds
CheckAllWarning               PASS      35.45 seconds
CheckSparse                   PASS      42.21 seconds
CheckSmatch                   PASS      113.36 seconds
BuildKernel32                 PASS      31.27 seconds
TestRunnerSetup               PASS      478.83 seconds
TestRunner_l2cap-tester       PASS      27.54 seconds
TestRunner_iso-tester         PASS      48.19 seconds
TestRunner_bnep-tester        PASS      10.63 seconds
TestRunner_mgmt-tester        PASS      216.22 seconds
TestRunner_rfcomm-tester      PASS      16.10 seconds
TestRunner_sco-tester         PASS      19.05 seconds
TestRunner_ioctl-tester       PASS      17.97 seconds
TestRunner_mesh-tester        PASS      13.37 seconds
TestRunner_smp-tester         PASS      14.33 seconds
TestRunner_userchan-tester    PASS      11.15 seconds
IncrementalBuild              PASS      29.72 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: hci_conn: fail SCO/ISO via hci_conn_failed if ACL gone early

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
14: B1 Line exceeds max length (88>80): "Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/"
21: B1 Line exceeds max length (83>80): "    ISO Connect ACL Disconnect - Failure                 Passed       1.004 seconds"
22: B1 Line exceeds max length (83>80): "    eSCO ACL Disconnect - Failure                        Passed       0.987 seconds"


---
Regards,
Linux Bluetooth

syzbot Aug. 19, 2023, 2:01 p.m. UTC | #2

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: [e-mail]

Tested on:

commit:         f0835e74 Bluetooth: ISO: Use defer setup to separate P..
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1479e265a80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4b3a4035b595be0
dashboard link: https://syzkaller.appspot.com/bug?extid=cf54c1da6574b6c1b049
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=13b9e265a80000

Note: testing is done by a robot and is best-effort only.

[e-mail] Aug. 21, 2023, 8:40 p.m. UTC | #3

Hello:

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

On Sat, 19 Aug 2023 16:33:36 +0300 you wrote:
> Not calling hci_(dis)connect_cfm before deleting conn referred to by a
> socket generally results to use-after-free.
> 
> When cleaning up SCO connections when the parent ACL is deleted too
> early, use hci_conn_failed to do the connection cleanup properly.
> 
> We also need to clean up ISO connections in a similar situation when
> connecting has started but LE Create CIS is not yet sent, so do it too
> here.
> 
> [...]

Here is the summary with links:
  - Bluetooth: hci_conn: fail SCO/ISO via hci_conn_failed if ACL gone early
    https://git.kernel.org/bluetooth/bluetooth-next/c/c452805643ff

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 8b0c8e631324..9d5057cef30a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1044,6 +1044,29 @@  struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	return conn;
 }
 
+static void hci_conn_cleanup_child(struct hci_conn *conn, u8 reason)
+{
+	if (!reason)
+		reason = HCI_ERROR_REMOTE_USER_TERM;
+
+	/* Due to race, SCO/ISO conn might be not established yet at this point,
+	 * and nothing else will clean it up. In other cases it is done via HCI
+	 * events.
+	 */
+	switch (conn->type) {
+	case SCO_LINK:
+	case ESCO_LINK:
+		if (HCI_CONN_HANDLE_UNSET(conn->handle))
+			hci_conn_failed(conn, reason);
+		break;
+	case ISO_LINK:
+		if (conn->state != BT_CONNECTED &&
+		    !test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
+			hci_conn_failed(conn, reason);
+		break;
+	}
+}
+
 static void hci_conn_unlink(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -1066,14 +1089,7 @@  static void hci_conn_unlink(struct hci_conn *conn)
 			if (!test_bit(HCI_UP, &hdev->flags))
 				continue;
 
-			/* Due to race, SCO connection might be not established
-			 * yet at this point. Delete it now, otherwise it is
-			 * possible for it to be stuck and can't be deleted.
-			 */
-			if ((child->type == SCO_LINK ||
-			     child->type == ESCO_LINK) &&
-			    HCI_CONN_HANDLE_UNSET(child->handle))
-				hci_conn_del(child);
+			hci_conn_cleanup_child(child, conn->abort_reason);
 		}
 
 		return;