Skip to contentSkip to conversation

Bluetooth: use hdev lock for accept_list and reject_list in conn req

PATCH Superseded Niels Dossche 3 participants 4 messages Apr 4, 2022 – Apr 6, 2022

Subsystems (2)
Bluetooth drivers Bluetooth

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Niels Dossche

author
#0 lore raw

PATCH 1/1 Bluetooth: use hdev lock for accept_list and reject_list in conn req

All accesses (both read and modifications) to hdev->{accept,reject}_list are protected by hdev lock, except the ones in hci_conn_request_evt. This can cause a race condition in the form of a list corruption.
The solution is to protect these lists in hci_conn_request_evt as well.

I was unable to find the exact commit that introduced the issue for the reject list, I was only able to find it for the accept list.

Note:
I am currently working on a static analyser to detect missing locks using type-based static analysis as my master's thesis
in order to obtain my master's degree.
If you would like to have more details, please let me know. This was a reported case. I manually verified the report by looking at the code, so that I do not send wrong information or patches. After concluding that this seems to be a true positive, I created this patch. I have both compile-tested this patch and runtime-tested this patch on x86_64. The effect on a running system could be a potential race condition in exceptional cases.
This issue was found on Linux v5.17.1.

Fixes a55bd29d5227 ("Bluetooth: Add white list lookup for incoming connection requests")
Signed-off-by Niels Dossche <[email protected]>
---
 net/bluetooth/hci_event.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
net/bluetooth/hci_event.c +4 -2
index abaabfae19cc..b9038f24f46f 100644
@@ -3222,8 +3222,11 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 		return;
 	}
 
+	hci_dev_lock(hdev);
+
 	if (hci_bdaddr_list_lookup(&hdev->reject_list, &ev->bdaddr,
 				   BDADDR_BREDR)) {
+		hci_dev_unlock(hdev);
 		hci_reject_conn(hdev, &ev->bdaddr);
 		return;
 	}

@@ -3236,14 +3239,13 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 	    !hci_dev_test_flag(hdev, HCI_CONNECTABLE) &&
 	    !hci_bdaddr_list_lookup_with_flags(&hdev->accept_list, &ev->bdaddr,
 					       BDADDR_BREDR)) {
+		hci_dev_unlock(hdev);
 		hci_reject_conn(hdev, &ev->bdaddr);
 		return;
 	}
 
 	/* Connection accepted */
 
-	hci_dev_lock(hdev);
-
 	ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
 	if (ie)
 		memcpy(ie->data.dev_class, ev->dev_class, 3);
Signature · 1 line
2.35.1
#1 lore raw

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

---Test result---

Test Summary:

CheckPatch                    PASS      1.54 seconds
GitLint                       PASS      1.01 seconds
SubjectPrefix                 PASS      0.89 seconds
BuildKernel                   PASS      30.05 seconds
BuildKernel32                 PASS      27.33 seconds
Incremental Build with patchesPASS      38.38 seconds
TestRunner: Setup             PASS      458.46 seconds
TestRunner: l2cap-tester      PASS      15.27 seconds
TestRunner: bnep-tester       PASS      6.04 seconds
TestRunner: mgmt-tester       PASS      99.50 seconds
TestRunner: rfcomm-tester     PASS      7.31 seconds
TestRunner: sco-tester        PASS      7.24 seconds
TestRunner: smp-tester        PASS      6.75 seconds
TestRunner: userchan-tester   PASS      5.87 seconds

---
Regards,
Linux Bluetooth

maintainer
#2 lore raw
In reply to Niels Dossche

Hi Niels,

Quoted 9 lines

All accesses (both read and modifications) to hdev->{accept,reject}_list are protected by hdev lock, except the ones in hci_conn_request_evt. This can cause a race condition in the form of a list corruption.
The solution is to protect these lists in hci_conn_request_evt as well.

I was unable to find the exact commit that introduced the issue for the reject list, I was only able to find it for the accept list.

Quoted 12 lines

Note:
I am currently working on a static analyser to detect missing locks using type-based static analysis as my master's thesis
in order to obtain my master's degree.
If you would like to have more details, please let me know. This was a reported case. I manually verified the report by looking at the code, so that I do not send wrong information or patches. After concluding that this seems to be a true positive, I created this patch. I have both compile-tested this patch and runtime-tested this patch on x86_64. The effect on a running system could be a potential race condition in exceptional cases.
This issue was found on Linux v5.17.1.

this doesn’t belong in the commit message.

Quoted 24 lines
Fixes a55bd29d5227 ("Bluetooth: Add white list lookup for incoming connection requests")
Signed-off-by Niels Dossche <[email protected]>

---
net/bluetooth/hci_event.c | 6 ++++--

1 file changed, 4 insertions(+), 2 deletions(-)
net/bluetooth/hci_event.c +3 -0
index abaabfae19cc..b9038f24f46f 100644
@@ -3222,8 +3222,11 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 		return;
 	}
 
+	hci_dev_lock(hdev);
+
 	if (hci_bdaddr_list_lookup(&hdev->reject_list, &ev->bdaddr,
 				   BDADDR_BREDR)) {
+		hci_dev_unlock(hdev);
 		hci_reject_conn(hdev, &ev->bdaddr);
 		return;
 	}

net/bluetooth/hci_event.c static void hci_conn_request_evt(struct hci_dev *hdev, void *data, · line 3245

Quoted 7 lines
net/bluetooth/hci_event.c +1 -0
@@ -3236,14 +3239,13 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 	    !hci_dev_test_flag(hdev, HCI_CONNECTABLE) &&
 	    !hci_bdaddr_list_lookup_with_flags(&hdev->accept_list, &ev->bdaddr,
 					       BDADDR_BREDR)) {
+		hci_dev_unlock(hdev);
 		hci_reject_conn(hdev, &ev->bdaddr);
 		return;
 	}

Can we use an exit goto-label instead. I am not a big fan of "unbalanced” _unlock statements.

net/bluetooth/hci_event.c static void hci_conn_request_evt(struct hci_dev *hdev, void *data, · line 3251

Quoted 7 lines
net/bluetooth/hci_event.c +0 -2
 	/* Connection accepted */
 
-	hci_dev_lock(hdev);
-
 	ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
 	if (ie)
 		memcpy(ie->data.dev_class, ev->dev_class, 3);

Regards

Marcel

author
#3 lore raw
Quoted 24 lines · Marcel Holtmann

Hi Niels,

Quoted 9 lines

All accesses (both read and modifications) to hdev->{accept,reject}_list are protected by hdev lock, except the ones in hci_conn_request_evt. This can cause a race condition in the form of a list corruption.
The solution is to protect these lists in hci_conn_request_evt as well.

I was unable to find the exact commit that introduced the issue for the reject list, I was only able to find it for the accept list.

Quoted 12 lines

Note:
I am currently working on a static analyser to detect missing locks using type-based static analysis as my master's thesis
in order to obtain my master's degree.
If you would like to have more details, please let me know. This was a reported case. I manually verified the report by looking at the code, so that I do not send wrong information or patches. After concluding that this seems to be a true positive, I created this patch. I have both compile-tested this patch and runtime-tested this patch on x86_64. The effect on a running system could be a potential race condition in exceptional cases.
This issue was found on Linux v5.17.1.

Quoted 1 line

this doesn’t belong in the commit message.

Hi Marcel
I'll remove it from the commit message.
I can write it in below the --- in the future such that it won't be included.

Quoted 26 lines
Quoted 26 lines
Fixes a55bd29d5227 ("Bluetooth: Add white list lookup for incoming connection requests")
Signed-off-by Niels Dossche <[email protected]>

---
net/bluetooth/hci_event.c | 6 ++++--

1 file changed, 4 insertions(+), 2 deletions(-)
net/bluetooth/hci_event.c +3 -0
index abaabfae19cc..b9038f24f46f 100644
@@ -3222,8 +3222,11 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 		return;
 	}
 
+	hci_dev_lock(hdev);
+
 	if (hci_bdaddr_list_lookup(&hdev->reject_list, &ev->bdaddr,
 				   BDADDR_BREDR)) {
+		hci_dev_unlock(hdev);
 		hci_reject_conn(hdev, &ev->bdaddr);
 		return;
 	}

@@ -3236,14 +3239,13 @@ static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 	    !hci_dev_test_flag(hdev, HCI_CONNECTABLE) &&
Quoted 8 lines

net/bluetooth/hci_event.c static void hci_conn_request_evt(struct hci_dev *hdev, void *data, · line 3245

Quoted 6 lines
net/bluetooth/hci_event.c +1 -0
 	    !hci_bdaddr_list_lookup_with_flags(&hdev->accept_list, &ev->bdaddr,
 					       BDADDR_BREDR)) {
+		hci_dev_unlock(hdev);
 		hci_reject_conn(hdev, &ev->bdaddr);
 		return;
 	}

Can we use an exit goto-label instead. I am not a big fan of "unbalanced” _unlock statements.

I'll make a v2 and send it.

Quoted 11 lines

net/bluetooth/hci_event.c static void hci_conn_request_evt(struct hci_dev *hdev, void *data, · line 3251

Quoted 8 lines
net/bluetooth/hci_event.c +0 -2
 	/* Connection accepted */
 
-	hci_dev_lock(hdev);
-
 	ie = hci_inquiry_cache_lookup(hdev, &ev->bdaddr);
 	if (ie)
 		memcpy(ie->data.dev_class, ev->dev_class, 3);

Regards

Quoted 2 lines

Marcel

Thanks
Niels