| Message ID | 20240327142526.332756-1-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | db4597cc88b2621706dc39a7363e61f4e7734425 |
| Headers | show |
| Series | Bluetooth: Fix TOCTOU in HCI debugfs implementation | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | 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 20: B3 Line contains hard tab characters (\t): " if (val == 0 || val > hdev->conn_info_max_age)" 21: B3 Line contains hard tab characters (\t): " return -EINVAL;" 25: B3 Line contains hard tab characters (\t): " if (val == 0 || val > hdev->conn_info_max_age)" 26: B3 Line contains hard tab characters (\t): " return -EINVAL;" 49: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/20231222161317.6255-1-2045gemini@gmail.com/" 51: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/20231222162931.6553-1-2045gemini@gmail.com/" 52: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/20231222162310.6461-1-2045gemini@gmail.com/" 58: B1 Line exceeds max length (81>80): "I've made changes to the patches that you submitted in December 2023 and that are" |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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=838901 ---Test result--- Test Summary: CheckPatch PASS 0.54 seconds GitLint FAIL 0.54 seconds SubjectPrefix PASS 0.07 seconds BuildKernel PASS 27.85 seconds CheckAllWarning PASS 30.40 seconds CheckSparse PASS 36.02 seconds CheckSmatch PASS 98.47 seconds BuildKernel32 PASS 26.97 seconds TestRunnerSetup PASS 519.49 seconds TestRunner_l2cap-tester PASS 20.17 seconds TestRunner_iso-tester PASS 32.68 seconds TestRunner_bnep-tester PASS 4.81 seconds TestRunner_mgmt-tester PASS 113.85 seconds TestRunner_rfcomm-tester PASS 7.31 seconds TestRunner_sco-tester PASS 14.94 seconds TestRunner_ioctl-tester PASS 7.76 seconds TestRunner_mesh-tester PASS 5.80 seconds TestRunner_smp-tester PASS 6.89 seconds TestRunner_userchan-tester PASS 4.92 seconds IncrementalBuild PASS 26.45 seconds Details ############################## Test: GitLint - FAIL Desc: Run gitlint Output: Bluetooth: Fix TOCTOU in HCI debugfs implementation 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 20: B3 Line contains hard tab characters (\t): " if (val == 0 || val > hdev->conn_info_max_age)" 21: B3 Line contains hard tab characters (\t): " return -EINVAL;" 25: B3 Line contains hard tab characters (\t): " if (val == 0 || val > hdev->conn_info_max_age)" 26: B3 Line contains hard tab characters (\t): " return -EINVAL;" 49: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/20231222161317.6255-1-2045gemini@gmail.com/" 51: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/20231222162931.6553-1-2045gemini@gmail.com/" 52: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/20231222162310.6461-1-2045gemini@gmail.com/" 58: B1 Line exceeds max length (81>80): "I've made changes to the patches that you submitted in December 2023 and that are" --- Regards, Linux Bluetooth
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Wed, 27 Mar 2024 15:24:56 +0100 you wrote: > struct hci_dev members conn_info_max_age, conn_info_min_age, > le_conn_max_interval, le_conn_min_interval, le_adv_max_interval, > and le_adv_min_interval can be modified from the HCI core code, as well > through debugfs. > > The debugfs implementation, that's only available to privileged users, > will check for boundaries, making sure that the minimum value being set > is strictly above the maximum value that already exists, and vice-versa. > > [...] Here is the summary with links: - Bluetooth: Fix TOCTOU in HCI debugfs implementation https://git.kernel.org/bluetooth/bluetooth-next/c/db4597cc88b2 You are awesome, thank you!
Hi Bastien, Thanks for the updates to the December 2023 patches, including the shift to identifying the issue as TOCTOU and the corrections made to the commit messages and CVE references. I see the merit in distinguishing between TOCTOU and atomicity violation as you've outlined. Both terms provide valid frameworks for categorizing concurrency issues—atomicity violation from a broad program analysis standpoint and TOCTOU focusing on the timing discrepancies. I'm on board with this classification and the changes you've implemented. I'm good with the changes. Best regards, Han Bastien Nocera <hadess@hadess.net> 于2024年3月27日周三 22:25写道： > > struct hci_dev members conn_info_max_age, conn_info_min_age, > le_conn_max_interval, le_conn_min_interval, le_adv_max_interval, > and le_adv_min_interval can be modified from the HCI core code, as well > through debugfs. > > The debugfs implementation, that's only available to privileged users, > will check for boundaries, making sure that the minimum value being set > is strictly above the maximum value that already exists, and vice-versa. > > However, as both minimum and maximum values can be changed concurrently > to us modifying them, we need to make sure that the value we check is > the value we end up using. > > For example, with ->conn_info_max_age set to 10, conn_info_min_age_set() > gets called from vfs handlers to set conn_info_min_age to 8. > > In conn_info_min_age_set(), this goes through: > if (val == 0 || val > hdev->conn_info_max_age) > return -EINVAL; > > Concurrently, conn_info_max_age_set() gets called to set to set the > conn_info_max_age to 7: > if (val == 0 || val > hdev->conn_info_max_age) > return -EINVAL; > That check will also pass because we used the old value (10) for > conn_info_max_age. > > After those checks that both passed, the struct hci_dev access > is mutex-locked, disabling concurrent access, but that does not matter > because the invalid value checks both passed, and we'll end up with > conn_info_min_age = 8 and conn_info_max_age = 7 > > To fix this problem, we need to lock the structure access before so the > check and assignment are not interrupted. > > This fix was originally devised by the BassCheck[1] team, and > considered the problem to be an atomicity one. This isn't the case as > there aren't any concerns about the variable changing while we check it, > but rather after we check it parallel to another change. > > This patch fixes CVE-2024-24858 and CVE-2024-24857. > > [1] https://sites.google.com/view/basscheck/ > > Co-developed-by: Gui-Dong Han <2045gemini@gmail.com> > Signed-off-by: Gui-Dong Han <2045gemini@gmail.com> > Link: https://lore.kernel.org/linux-bluetooth/20231222161317.6255-1-2045gemini@gmail.com/ > Link: https://nvd.nist.gov/vuln/detail/CVE-2024-24858 > Link: https://lore.kernel.org/linux-bluetooth/20231222162931.6553-1-2045gemini@gmail.com/ > Link: https://lore.kernel.org/linux-bluetooth/20231222162310.6461-1-2045gemini@gmail.com/ > Link: https://nvd.nist.gov/vuln/detail/CVE-2024-24857 > Fixes: 31ad169148df ("Bluetooth: Add conn info lifetime parameters to debugfs") > Fixes: 729a1051da6f ("Bluetooth: Expose default LE advertising interval via debugfs") > Fixes: 71c3b60ec6d2 ("Bluetooth: Move BR/EDR debugfs file creation into hci_debugfs.c") > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > Hello Gui-Dong Han, > > I've made changes to the patches that you submitted in December 2023 and that are > linked above to: > - correct the commit message and description, this isn't an atomicity > problem, but a TOCTOU problem > - corrected the "fixes" references to be of the original code > - added CVE references for the changes that warranted it > > I've kept you as the co-author of this patch and kept the references to > BassCheck as well. > > Let me know what you think. > > Regards > > net/bluetooth/hci_debugfs.c | 48 ++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 16 deletions(-) > > diff --git a/net/bluetooth/hci_debugfs.c b/net/bluetooth/hci_debugfs.c > index 233453807b50..ce3ff2fa72e5 100644 > --- a/net/bluetooth/hci_debugfs.c > +++ b/net/bluetooth/hci_debugfs.c > @@ -218,10 +218,12 @@ static int conn_info_min_age_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val == 0 || val > hdev->conn_info_max_age) > + hci_dev_lock(hdev); > + if (val == 0 || val > hdev->conn_info_max_age) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->conn_info_min_age = val; > hci_dev_unlock(hdev); > > @@ -246,10 +248,12 @@ static int conn_info_max_age_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val == 0 || val < hdev->conn_info_min_age) > + hci_dev_lock(hdev); > + if (val == 0 || val < hdev->conn_info_min_age) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->conn_info_max_age = val; > hci_dev_unlock(hdev); > > @@ -567,10 +571,12 @@ static int sniff_min_interval_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val == 0 || val % 2 || val > hdev->sniff_max_interval) > + hci_dev_lock(hdev); > + if (val == 0 || val % 2 || val > hdev->sniff_max_interval) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->sniff_min_interval = val; > hci_dev_unlock(hdev); > > @@ -595,10 +601,12 @@ static int sniff_max_interval_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val == 0 || val % 2 || val < hdev->sniff_min_interval) > + hci_dev_lock(hdev); > + if (val == 0 || val % 2 || val < hdev->sniff_min_interval) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->sniff_max_interval = val; > hci_dev_unlock(hdev); > > @@ -850,10 +858,12 @@ static int conn_min_interval_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val < 0x0006 || val > 0x0c80 || val > hdev->le_conn_max_interval) > + hci_dev_lock(hdev); > + if (val < 0x0006 || val > 0x0c80 || val > hdev->le_conn_max_interval) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->le_conn_min_interval = val; > hci_dev_unlock(hdev); > > @@ -878,10 +888,12 @@ static int conn_max_interval_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val < 0x0006 || val > 0x0c80 || val < hdev->le_conn_min_interval) > + hci_dev_lock(hdev); > + if (val < 0x0006 || val > 0x0c80 || val < hdev->le_conn_min_interval) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->le_conn_max_interval = val; > hci_dev_unlock(hdev); > > @@ -990,10 +1002,12 @@ static int adv_min_interval_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val < 0x0020 || val > 0x4000 || val > hdev->le_adv_max_interval) > + hci_dev_lock(hdev); > + if (val < 0x0020 || val > 0x4000 || val > hdev->le_adv_max_interval) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->le_adv_min_interval = val; > hci_dev_unlock(hdev); > > @@ -1018,10 +1032,12 @@ static int adv_max_interval_set(void *data, u64 val) > { > struct hci_dev *hdev = data; > > - if (val < 0x0020 || val > 0x4000 || val < hdev->le_adv_min_interval) > + hci_dev_lock(hdev); > + if (val < 0x0020 || val > 0x4000 || val < hdev->le_adv_min_interval) { > + hci_dev_unlock(hdev); > return -EINVAL; > + } > > - hci_dev_lock(hdev); > hdev->le_adv_max_interval = val; > hci_dev_unlock(hdev); > > -- > 2.44.0 >
diff --git a/net/bluetooth/hci_debugfs.c b/net/bluetooth/hci_debugfs.c index 233453807b50..ce3ff2fa72e5 100644 --- a/net/bluetooth/hci_debugfs.c +++ b/net/bluetooth/hci_debugfs.c @@ -218,10 +218,12 @@ static int conn_info_min_age_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val == 0 || val > hdev->conn_info_max_age) + hci_dev_lock(hdev); + if (val == 0 || val > hdev->conn_info_max_age) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->conn_info_min_age = val; hci_dev_unlock(hdev); @@ -246,10 +248,12 @@ static int conn_info_max_age_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val == 0 || val < hdev->conn_info_min_age) + hci_dev_lock(hdev); + if (val == 0 || val < hdev->conn_info_min_age) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->conn_info_max_age = val; hci_dev_unlock(hdev); @@ -567,10 +571,12 @@ static int sniff_min_interval_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val == 0 || val % 2 || val > hdev->sniff_max_interval) + hci_dev_lock(hdev); + if (val == 0 || val % 2 || val > hdev->sniff_max_interval) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->sniff_min_interval = val; hci_dev_unlock(hdev); @@ -595,10 +601,12 @@ static int sniff_max_interval_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val == 0 || val % 2 || val < hdev->sniff_min_interval) + hci_dev_lock(hdev); + if (val == 0 || val % 2 || val < hdev->sniff_min_interval) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->sniff_max_interval = val; hci_dev_unlock(hdev); @@ -850,10 +858,12 @@ static int conn_min_interval_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val < 0x0006 || val > 0x0c80 || val > hdev->le_conn_max_interval) + hci_dev_lock(hdev); + if (val < 0x0006 || val > 0x0c80 || val > hdev->le_conn_max_interval) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->le_conn_min_interval = val; hci_dev_unlock(hdev); @@ -878,10 +888,12 @@ static int conn_max_interval_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val < 0x0006 || val > 0x0c80 || val < hdev->le_conn_min_interval) + hci_dev_lock(hdev); + if (val < 0x0006 || val > 0x0c80 || val < hdev->le_conn_min_interval) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->le_conn_max_interval = val; hci_dev_unlock(hdev); @@ -990,10 +1002,12 @@ static int adv_min_interval_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val < 0x0020 || val > 0x4000 || val > hdev->le_adv_max_interval) + hci_dev_lock(hdev); + if (val < 0x0020 || val > 0x4000 || val > hdev->le_adv_max_interval) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->le_adv_min_interval = val; hci_dev_unlock(hdev); @@ -1018,10 +1032,12 @@ static int adv_max_interval_set(void *data, u64 val) { struct hci_dev *hdev = data; - if (val < 0x0020 || val > 0x4000 || val < hdev->le_adv_min_interval) + hci_dev_lock(hdev); + if (val < 0x0020 || val > 0x4000 || val < hdev->le_adv_min_interval) { + hci_dev_unlock(hdev); return -EINVAL; + } - hci_dev_lock(hdev); hdev->le_adv_max_interval = val; hci_dev_unlock(hdev);