[PATCH] Bluetooth: add timeout sanity check to hci_inquiry

Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
unreasonable huge inquiry timeout passed from userspace.
Fix it by adding sanity check for timeout value and add constant to
hsi_sock.h to inform userspace, that hci_inquiry_req::length field has
maximum possible value.

Since hci_inquiry() is the only user of hci_req_sync() with user
controlled timeout value, it makes sense to check timeout value in
hci_inquiry() and don't touch hci_req_sync().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
---

Hi, Bluetooth maintainers/reviewers!

I believe, 60 seconds will be more than enough for inquiry request. I've
searched for examples on the internet and maximum ir.length I found was 
8. Maybe, we have users, which need more than 60 seconds... I look forward
to receiving your views on this value.

---
 include/net/bluetooth/hci_sock.h | 1 +
 net/bluetooth/hci_core.c         | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/net/bluetooth/hci_sock.h b/include/net/bluetooth/hci_sock.h
index 9949870f7d78..1cd63d4da00b 100644
--- a/include/net/bluetooth/hci_sock.h
+++ b/include/net/bluetooth/hci_sock.h

@@ -168,6 +168,7 @@ struct hci_inquiry_req {
 	__u16 dev_id;
 	__u16 flags;
 	__u8  lap[3];
+#define HCI_INQUIRY_MAX_TIMEOUT		30
 	__u8  length;
 	__u8  num_rsp;
 };

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e1a545c8a69f..104babf67351 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c

@@ -1343,6 +1343,11 @@ int hci_inquiry(void __user *arg)
 		goto done;
 	}
 
+	if (ir.length > HCI_INQUIRY_MAX_TIMEOUT) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	hci_dev_lock(hdev);
 	if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
 	    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {

-- 
2.32.0

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

---Test result---

Test Summary:
CheckPatch                    FAIL      0.57 seconds
GitLint                       PASS      0.11 seconds
BuildKernel                   PASS      513.85 seconds
TestRunner: Setup             PASS      336.43 seconds
TestRunner: l2cap-tester      PASS      2.60 seconds
TestRunner: bnep-tester       PASS      1.84 seconds
TestRunner: mgmt-tester       PASS      30.71 seconds
TestRunner: rfcomm-tester     PASS      1.95 seconds
TestRunner: sco-tester        PASS      1.94 seconds
TestRunner: smp-tester        FAIL      1.99 seconds
TestRunner: userchan-tester   PASS      1.89 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.57 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: add timeout sanity check to hci_inquiry
WARNING: Unknown commit id '1da177e4c3f4', maybe rebased or not pulled?
#16: 
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17: 
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com

total: 0 errors, 2 warnings, 0 checks, 18 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: add timeout sanity check to hci_inquiry" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS - 0.11 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 513.85 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 336.43 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.60 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.84 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 30.71 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 1.95 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 1.94 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 1.99 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.020 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 1.89 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth

Hi Pavel,

	/* Restrict maximum inquiry length to 60 seconds */
	if (ir.length > 60) {
		..
	}

+	if (ir.length > HCI_INQUIRY_MAX_TIMEOUT) {
+		err = -EINVAL;
+		goto done;
+	}
+

I found this easier to read than adding anything define somewhere else. And since this is a legacy interface that is no longer used by bluetoothd, this should be fine. We will start to deprecate this eventually.

And I prefer 1 minute max time here. Just to be safe.

Regards

Marcel

On 8/19/21 6:05 PM, Marcel Holtmann wrote:

Hi Pavel,

quoted

	}

	/* Restrict maximum inquiry length to 60 seconds */
	if (ir.length > 60) {
		..
	}

quoted

+	if (ir.length > HCI_INQUIRY_MAX_TIMEOUT) {
+		err = -EINVAL;
+		goto done;
+	}
+

I found this easier to read than adding anything define somewhere else. And since this is a legacy interface that is no longer used by bluetoothd, this should be fine. We will start to deprecate this eventually.

And I prefer 1 minute max time here. Just to be safe.

I thought, that user-space should be aware of maximum value, that's why 
I decided to add this define :) I didn't know, that this interface is 
legacy.

Will fix in v2, thank you!


With regards,
Pavel Skripkin

Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
unreasonable huge inquiry timeout passed from userspace.
Fix it by adding sanity check for timeout value to hci_inquiry().

Since hci_inquiry() is the only user of hci_req_sync() with user
controlled timeout value, it makes sense to check timeout value in
hci_inquiry() and don't touch hci_req_sync().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
---

Changes in v2:
	Removed define + added comment suggested by Marcel

---
 net/bluetooth/hci_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e1a545c8a69f..170f513efa86 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c

@@ -1343,6 +1343,12 @@ int hci_inquiry(void __user *arg)
 		goto done;
 	}
 
+	/* Restrict maximum inquiry length to 60 seconds */
+	if (ir.length > 60) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	hci_dev_lock(hdev);
 	if (inquiry_cache_age(hdev) > INQUIRY_CACHE_AGE_MAX ||
 	    inquiry_cache_empty(hdev) || ir.flags & IREQ_CACHE_FLUSH) {

-- 
2.32.0

Hi Pavel,

Syzbot hit "task hung" bug in hci_req_sync(). The problem was in
unreasonable huge inquiry timeout passed from userspace.
Fix it by adding sanity check for timeout value to hci_inquiry().

Since hci_inquiry() is the only user of hci_req_sync() with user
controlled timeout value, it makes sense to check timeout value in
hci_inquiry() and don't touch hci_req_sync().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
---

Changes in v2:
	Removed define + added comment suggested by Marcel

---
net/bluetooth/hci_core.c | 6 ++++++
1 file changed, 6 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

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

---Test result---

Test Summary:
CheckPatch                    FAIL      0.45 seconds
GitLint                       PASS      0.10 seconds
BuildKernel                   PASS      500.56 seconds
TestRunner: Setup             PASS      326.35 seconds
TestRunner: l2cap-tester      PASS      2.53 seconds
TestRunner: bnep-tester       PASS      1.90 seconds
TestRunner: mgmt-tester       PASS      30.57 seconds
TestRunner: rfcomm-tester     PASS      2.00 seconds
TestRunner: sco-tester        PASS      2.01 seconds
TestRunner: smp-tester        FAIL      2.02 seconds
TestRunner: userchan-tester   PASS      1.92 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.45 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: add timeout sanity check to hci_inquiry
WARNING: Unknown commit id '1da177e4c3f4', maybe rebased or not pulled?
#14: 
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#15: 
Reported-and-tested-by: syzbot+be2baed593ea56c6a84c@syzkaller.appspotmail.com

total: 0 errors, 2 warnings, 0 checks, 12 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: add timeout sanity check to hci_inquiry" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS - 0.10 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 500.56 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 326.35 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.53 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.90 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 30.57 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.00 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.01 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.02 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.020 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 1.92 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth