Bluetooth: SCO: fix sco_conn related locking and validity issues

Commit Message

Pauli Virtanen July 10, 2023, 4:48 p.m. UTC

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 > sco_conn_lock,
which is how it is in connect/disconnect_cfm -> sco_conn_del ->
sco_chan_del.

Fix locking in sco_connect to take lock_sock around updating sk_state
and conn.

sco_conn_del must not occur during sco_connect, as it frees the
sco_conn. Hold hdev->lock longer to prevent that.

sco_conn_add shall return sco_conn with valid hcon. Make it so also when
reusing an old SCO connection waiting for disconnect timeout (see
__sco_sock_close where conn->hcon is set to NULL).

This should not reintroduce the issue fixed in the earlier
commit 9a8ec9e8ebb5 ("Bluetooth: SCO: Fix possible circular locking
dependency on sco_connect_cfm"), the relevant fix of releasing lock_sock
in sco_sock_connect before acquiring hdev->lock is retained.

These changes mirror similar fixes earlier in ISO sockets.

Fixes: 9a8ec9e8ebb5 ("Bluetooth: SCO: Fix possible circular locking dependency on sco_connect_cfm")
Signed-off-by: Pauli Virtanen <[e-mail]>
---

Notes:
    Possibly related:
    
    https://lore.kernel.org/linux-bluetooth/[e-mail]/
    https://lore.kernel.org/linux-bluetooth/HwByr9Ffw_HVVE5pnMu6CTIrQjKwBJpJcSskM0xOP0S-4XiIdmGFTLYAcgKJsOkrxt62je88hFJ1vkHzYtCgwsEATe0KFq3E4YX8LeN5Fdo=@protonmail.com/

 net/bluetooth/sco.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

[e-mail] July 10, 2023, 5: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=764106

---Test result---

Test Summary:
CheckPatch                    PASS      0.72 seconds
GitLint                       FAIL      0.58 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      33.87 seconds
CheckAllWarning               PASS      37.12 seconds
CheckSparse                   WARNING   42.51 seconds
CheckSmatch                   WARNING   113.52 seconds
BuildKernel32                 PASS      32.46 seconds
TestRunnerSetup               PASS      500.07 seconds
TestRunner_l2cap-tester       PASS      23.85 seconds
TestRunner_iso-tester         PASS      42.86 seconds
TestRunner_bnep-tester        PASS      10.82 seconds
TestRunner_mgmt-tester        PASS      223.11 seconds
TestRunner_rfcomm-tester      PASS      16.22 seconds
TestRunner_sco-tester         PASS      17.16 seconds
TestRunner_ioctl-tester       PASS      18.11 seconds
TestRunner_mesh-tester        PASS      13.46 seconds
TestRunner_smp-tester         PASS      14.88 seconds
TestRunner_userchan-tester    PASS      11.35 seconds
IncrementalBuild              PASS      31.27 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: SCO: fix sco_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
32: B2 Line has trailing whitespace: "    "
33: B1 Line exceeds max length (84>80): "    https://lore.kernel.org/linux-bluetooth/[e-mail]/"
34: B1 Line exceeds max length (168>80): "    https://lore.kernel.org/linux-bluetooth/HwByr9Ffw_HVVE5pnMu6CTIrQjKwBJpJcSskM0xOP0S-4XiIdmGFTLYAcgKJsOkrxt62je88hFJ1vkHzYtCgwsEATe0KFq3E4YX8LeN5Fdo=@protonmail.com/"
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:150:35: warning: array of flexible structures
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/sco.c: note: in included file:./include/net/bluetooth/hci_core.h:150:35: warning: array of flexible structures


---
Regards,
Linux Bluetooth

[e-mail] July 12, 2023, 9:10 p.m. UTC | #2

Hello:

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

On Mon, 10 Jul 2023 19:48:19 +0300 you wrote:
> 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 > sco_conn_lock,
> which is how it is in connect/disconnect_cfm -> sco_conn_del ->
> sco_chan_del.
> 
> [...]

Here is the summary with links:
  - Bluetooth: SCO: fix sco_conn related locking and validity issues
    https://git.kernel.org/bluetooth/bluetooth-next/c/23ba0a1d51bd

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index bbf7072f5e80..ec6dce488a40 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -126,8 +126,11 @@  static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 	struct hci_dev *hdev = hcon->hdev;
 	struct sco_conn *conn = hcon->sco_data;
 
-	if (conn)
+	if (conn) {
+		if (!conn->hcon)
+			conn->hcon = hcon;
 		return conn;
+	}
 
 	conn = kzalloc(sizeof(struct sco_conn), GFP_KERNEL);
 	if (!conn)
@@ -268,21 +271,21 @@  static int sco_connect(struct sock *sk)
 		goto unlock;
 	}
 
-	hci_dev_unlock(hdev);
-	hci_dev_put(hdev);
-
 	conn = sco_conn_add(hcon);
 	if (!conn) {
 		hci_conn_drop(hcon);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto unlock;
 	}
 
-	err = sco_chan_add(conn, sk, NULL);
-	if (err)
-		return err;
-
 	lock_sock(sk);
 
+	err = sco_chan_add(conn, sk, NULL);
+	if (err) {
+		release_sock(sk);
+		goto unlock;
+	}
+
 	/* Update source addr of the socket */
 	bacpy(&sco_pi(sk)->src, &hcon->src);
 
@@ -296,8 +299,6 @@  static int sco_connect(struct sock *sk)
 
 	release_sock(sk);
 
-	return err;
-
 unlock:
 	hci_dev_unlock(hdev);
 	hci_dev_put(hdev);