* [PATCH] Bluetooth: add timeout sanity check to hci_inquiry
@ 2021-08-17 10:31 Pavel Skripkin
2021-08-17 11:07 ` bluez.test.bot
2021-08-19 15:05 ` [PATCH] " Marcel Holtmann
0 siblings, 2 replies; 7+ messages in thread
From: Pavel Skripkin @ 2021-08-17 10:31 UTC (permalink / raw)
To: marcel, johan.hedberg, luiz.dentz, davem
Cc: linux-bluetooth, netdev, linux-kernel, Pavel Skripkin,
syzbot+be2baed593ea56c6a84c
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 <paskripkin@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: Bluetooth: add timeout sanity check to hci_inquiry
2021-08-17 10:31 [PATCH] Bluetooth: add timeout sanity check to hci_inquiry Pavel Skripkin
@ 2021-08-17 11:07 ` bluez.test.bot
2021-08-19 15:05 ` [PATCH] " Marcel Holtmann
1 sibling, 0 replies; 7+ messages in thread
From: bluez.test.bot @ 2021-08-17 11:07 UTC (permalink / raw)
To: linux-bluetooth, paskripkin
[-- Attachment #1: Type: text/plain, Size: 3489 bytes --]
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
[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44386 bytes --]
[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3593 bytes --]
[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616863 bytes --]
[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11713 bytes --]
[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9948 bytes --]
[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11741 bytes --]
[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5490 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: add timeout sanity check to hci_inquiry
2021-08-17 10:31 [PATCH] Bluetooth: add timeout sanity check to hci_inquiry Pavel Skripkin
2021-08-17 11:07 ` bluez.test.bot
@ 2021-08-19 15:05 ` Marcel Holtmann
2021-08-19 15:09 ` Pavel Skripkin
2021-08-19 15:15 ` [PATCH v2] " Pavel Skripkin
1 sibling, 2 replies; 7+ messages in thread
From: Marcel Holtmann @ 2021-08-19 15:05 UTC (permalink / raw)
To: Pavel Skripkin
Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller,
linux-bluetooth, netdev, linux-kernel,
syzbot+be2baed593ea56c6a84c
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 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 <paskripkin@gmail.com>
> ---
>
> 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;
> }
>
/* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Bluetooth: add timeout sanity check to hci_inquiry
2021-08-19 15:05 ` [PATCH] " Marcel Holtmann
@ 2021-08-19 15:09 ` Pavel Skripkin
2021-08-19 15:15 ` [PATCH v2] " Pavel Skripkin
1 sibling, 0 replies; 7+ messages in thread
From: Pavel Skripkin @ 2021-08-19 15:09 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller,
linux-bluetooth, netdev, linux-kernel,
syzbot+be2baed593ea56c6a84c
On 8/19/21 6:05 PM, Marcel Holtmann wrote:
> 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.
>
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] Bluetooth: add timeout sanity check to hci_inquiry
2021-08-19 15:05 ` [PATCH] " Marcel Holtmann
2021-08-19 15:09 ` Pavel Skripkin
@ 2021-08-19 15:15 ` Pavel Skripkin
2021-08-19 15:28 ` Marcel Holtmann
2021-08-19 16:10 ` [v2] " bluez.test.bot
1 sibling, 2 replies; 7+ messages in thread
From: Pavel Skripkin @ 2021-08-19 15:15 UTC (permalink / raw)
To: marcel, johan.hedberg, luiz.dentz, davem
Cc: linux-bluetooth, netdev, linux-kernel, Pavel Skripkin,
syzbot+be2baed593ea56c6a84c
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 <paskripkin@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Bluetooth: add timeout sanity check to hci_inquiry
2021-08-19 15:15 ` [PATCH v2] " Pavel Skripkin
@ 2021-08-19 15:28 ` Marcel Holtmann
2021-08-19 16:10 ` [v2] " bluez.test.bot
1 sibling, 0 replies; 7+ messages in thread
From: Marcel Holtmann @ 2021-08-19 15:28 UTC (permalink / raw)
To: Pavel Skripkin
Cc: Johan Hedberg, Luiz Augusto von Dentz, David S. Miller,
linux-bluetooth, netdev, linux-kernel,
syzbot+be2baed593ea56c6a84c
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 <paskripkin@gmail.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [v2] Bluetooth: add timeout sanity check to hci_inquiry
2021-08-19 15:15 ` [PATCH v2] " Pavel Skripkin
2021-08-19 15:28 ` Marcel Holtmann
@ 2021-08-19 16:10 ` bluez.test.bot
1 sibling, 0 replies; 7+ messages in thread
From: bluez.test.bot @ 2021-08-19 16:10 UTC (permalink / raw)
To: linux-bluetooth, paskripkin
[-- Attachment #1: Type: text/plain, Size: 3489 bytes --]
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
[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44385 bytes --]
[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3593 bytes --]
[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616863 bytes --]
[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11713 bytes --]
[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9948 bytes --]
[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11741 bytes --]
[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5489 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-19 16:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-17 10:31 [PATCH] Bluetooth: add timeout sanity check to hci_inquiry Pavel Skripkin
2021-08-17 11:07 ` bluez.test.bot
2021-08-19 15:05 ` [PATCH] " Marcel Holtmann
2021-08-19 15:09 ` Pavel Skripkin
2021-08-19 15:15 ` [PATCH v2] " Pavel Skripkin
2021-08-19 15:28 ` Marcel Holtmann
2021-08-19 16:10 ` [v2] " bluez.test.bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).