[PATCH v1 0/2] Bluetooth: Fix data-race on dst/src in connect paths

[PATCH v1 0/2] Bluetooth: Fix data-race on dst/src in connect paths

[SeungJu Cheon]

Two KCSAN-reported data races on socket address fields passed to
hci_get_route() without proper synchronization.

Patch 1/2 fixes ISO: iso_connect_bis(), iso_connect_cis(),
iso_listen_bis(), and iso_conn_big_sync() read iso_pi(sk)->dst/src
without lock_sock before calling hci_get_route().

Patch 2/2 fixes SCO: sco_connect() reads sco_pi(sk)->dst after
lock_sock has been released by the caller.

Both races were confirmed with KCSAN using VHCI-based reproducers.

SeungJu Cheon (2):
  Bluetooth: ISO: Fix data-race on iso_pi fields in hci_get_route calls
  Bluetooth: SCO: Fix data-race on dst in sco_connect

 net/bluetooth/iso.c | 51 ++++++++++++++++++++++++++++++++++-----------
 net/bluetooth/sco.c | 11 +++++++---
 2 files changed, 47 insertions(+), 15 deletions(-)

-- 
2.52.0

[PATCH v1 1/2] Bluetooth: ISO: Fix data-race on iso_pi fields in hci_get_route calls

[SeungJu Cheon]

iso_connect_bis(), iso_connect_cis(), iso_listen_bis(), and
iso_conn_big_sync() all call hci_get_route() reading iso_pi(sk)->dst,
iso_pi(sk)->src, and iso_pi(sk)->src_type without holding lock_sock.

These fields can be concurrently written by another thread calling
connect() or setsockopt() on the same socket, leading to torn reads
or TOCTOU mismatches.

Fix by snapshotting dst, src, and src_type into local variables under
lock_sock before calling hci_get_route() in all four functions.

BUG: KCSAN: data-race in memcmp+0x45/0xb0

race at unknown origin, with read to 0xffff8880122135cf of 1 bytes by task 333 on cpu 1:
 memcmp+0x45/0xb0
 hci_get_route+0x27e/0x490
 iso_connect_cis+0x4c/0xa10
 iso_sock_connect+0x60e/0xb30
 __sys_connect_file+0xbd/0xe0
 __sys_connect+0xe0/0x110
 __x64_sys_connect+0x40/0x50
 x64_sys_call+0xcad/0x1c60
 do_syscall_64+0x133/0x590
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fixes: 241f51931c35 ("Bluetooth: ISO: Avoid circular locking dependency")
Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
---
 net/bluetooth/iso.c | 51 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index d7af617cda45..58bb3a10d49f 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -337,12 +337,19 @@ static int iso_connect_bis(struct sock *sk)
 	struct iso_conn *conn;
 	struct hci_conn *hcon;
 	struct hci_dev  *hdev;
+	bdaddr_t src, dst;
+	u8 src_type;
 	int err;
 
-	BT_DBG("%pMR (SID 0x%2.2x)", &iso_pi(sk)->src, iso_pi(sk)->bc_sid);
+	lock_sock(sk);
+	bacpy(&dst, &iso_pi(sk)->dst);
+	bacpy(&src, &iso_pi(sk)->src);
+	src_type = iso_pi(sk)->src_type;
+	release_sock(sk);
+
+	BT_DBG("%pMR (SID 0x%2.2x)", &src, iso_pi(sk)->bc_sid);
 
-	hdev = hci_get_route(&iso_pi(sk)->dst, &iso_pi(sk)->src,
-			     iso_pi(sk)->src_type);
+	hdev = hci_get_route(&dst, &src, src_type);
 	if (!hdev)
 		return -EHOSTUNREACH;
 
@@ -430,12 +437,19 @@ static int iso_connect_cis(struct sock *sk)
 	struct iso_conn *conn;
 	struct hci_conn *hcon;
 	struct hci_dev  *hdev;
+	bdaddr_t src, dst;
+	u8 src_type;
 	int err;
 
-	BT_DBG("%pMR -> %pMR", &iso_pi(sk)->src, &iso_pi(sk)->dst);
+	lock_sock(sk);
+	bacpy(&dst, &iso_pi(sk)->dst);
+	bacpy(&src, &iso_pi(sk)->src);
+	src_type = iso_pi(sk)->src_type;
+	release_sock(sk);
+
+	BT_DBG("%pMR -> %pMR", &src, &dst);
 
-	hdev = hci_get_route(&iso_pi(sk)->dst, &iso_pi(sk)->src,
-			     iso_pi(sk)->src_type);
+	hdev = hci_get_route(&dst, &src, src_type);
 	if (!hdev)
 		return -EHOSTUNREACH;
 
@@ -1210,11 +1224,18 @@ static int iso_listen_bis(struct sock *sk)
 {
 	struct hci_dev *hdev;
 	int err = 0;
+	bdaddr_t src, dst;
+	u8 src_type;
 	struct iso_conn *conn;
 	struct hci_conn *hcon;
 
-	BT_DBG("%pMR -> %pMR (SID 0x%2.2x)", &iso_pi(sk)->src,
-	       &iso_pi(sk)->dst, iso_pi(sk)->bc_sid);
+	lock_sock(sk);
+	bacpy(&dst, &iso_pi(sk)->dst);
+	bacpy(&src, &iso_pi(sk)->src);
+	src_type = iso_pi(sk)->src_type;
+	release_sock(sk);
+
+	BT_DBG("%pMR -> %pMR (SID 0x%2.2x)", &src, &dst, iso_pi(sk)->bc_sid);
 
 	write_lock(&iso_sk_list.lock);
 
@@ -1227,8 +1248,7 @@ static int iso_listen_bis(struct sock *sk)
 	if (err)
 		return err;
 
-	hdev = hci_get_route(&iso_pi(sk)->dst, &iso_pi(sk)->src,
-			     iso_pi(sk)->src_type);
+	hdev = hci_get_route(&dst, &src, src_type);
 	if (!hdev)
 		return -EHOSTUNREACH;
 
@@ -1564,9 +1584,16 @@ static void iso_conn_big_sync(struct sock *sk)
 {
 	int err;
 	struct hci_dev *hdev;
+	bdaddr_t src, dst;
+	u8 src_type;
+
+	lock_sock(sk);
+	bacpy(&dst, &iso_pi(sk)->dst);
+	bacpy(&src, &iso_pi(sk)->src);
+	src_type = iso_pi(sk)->src_type;
+	release_sock(sk);
 
-	hdev = hci_get_route(&iso_pi(sk)->dst, &iso_pi(sk)->src,
-			     iso_pi(sk)->src_type);
+	hdev = hci_get_route(&dst, &src, src_type);
 
 	if (!hdev)
 		return;
-- 
2.52.0

[PATCH v1 2/2] Bluetooth: SCO: Fix data-race on dst in sco_connect

[SeungJu Cheon]

sco_sock_connect() copies the destination address into
sco_pi(sk)->dst under lock_sock, then releases the lock and calls
sco_connect(), which reads dst back without holding any lock in
hci_get_route() and hci_connect_sco().

If two threads call connect() on the same socket concurrently with
different addresses, one thread can overwrite dst before the other
thread's sco_connect() reads it.

Fix by snapshotting dst into a local variable under lock_sock at
the start of sco_connect(), matching the approach used for ISO in
the previous patch.

BUG: KCSAN: data-race in memcmp+0x45/0xb0

race at unknown origin, with read to 0xffff88800e6b0dd0 of 1 bytes by task 315 on cpu 0:
 memcmp+0x45/0xb0
 hci_connect_acl+0x1b7/0x6b0
 hci_connect_sco+0x4d/0xb30
 sco_sock_connect+0x27b/0xd60
 __sys_connect_file+0xbd/0xe0
 __sys_connect+0xe0/0x110
 __x64_sys_connect+0x40/0x50
 x64_sys_call+0xcad/0x1c60
 do_syscall_64+0x133/0x590
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fixes: 9a8ec9e8ebb5 ("Bluetooth: Fix three socket race condition bugs in sco.c")
Signed-off-by: SeungJu Cheon <suunj1331@gmail.com>
---
 net/bluetooth/sco.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index f1799c6a6f87..c9f6a8aaee57 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -312,11 +312,16 @@ static int sco_connect(struct sock *sk)
 	struct sco_conn *conn;
 	struct hci_conn *hcon;
 	struct hci_dev  *hdev;
+	bdaddr_t dst;
 	int err, type;
 
-	BT_DBG("%pMR -> %pMR", &sco_pi(sk)->src, &sco_pi(sk)->dst);
+	lock_sock(sk);
+	bacpy(&dst, &sco_pi(sk)->dst);
+	release_sock(sk);
+
+	BT_DBG("%pMR -> %pMR", &sco_pi(sk)->src, &dst);
 
-	hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src, BDADDR_BREDR);
+	hdev = hci_get_route(&dst, &sco_pi(sk)->src, BDADDR_BREDR);
 	if (!hdev)
 		return -EHOSTUNREACH;
 
@@ -336,7 +341,7 @@ static int sco_connect(struct sock *sk)
 		break;
 	}
 
-	hcon = hci_connect_sco(hdev, type, &sco_pi(sk)->dst,
+	hcon = hci_connect_sco(hdev, type, &dst,
 			       sco_pi(sk)->setting, &sco_pi(sk)->codec,
 			       READ_ONCE(sk->sk_sndtimeo));
 	if (IS_ERR(hcon)) {
-- 
2.52.0