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 <[email protected]>
Signed-off-by: Gui-Dong Han <[email protected]>
Link: https://lore.kernel.org/linux-bluetooth/[email protected]/
Link: https://nvd.nist.gov/vuln/detail/CVE-2024-24858
Link: https://lore.kernel.org/linux-bluetooth/[email protected]/
Link: https://lore.kernel.org/linux-bluetooth/[email protected]/
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 <[email protected]>
---
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
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/[email protected]/"
51: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/[email protected]/"
52: B1 Line exceeds max length (89>80): "Link: https://lore.kernel.org/linux-bluetooth/[email protected]/"
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 <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
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 <[email protected]> 于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 <[email protected]>
> Signed-off-by: Gui-Dong Han <[email protected]>
> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/
> Link: https://nvd.nist.gov/vuln/detail/CVE-2024-24858
> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/
> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/
> 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 <[email protected]>
> ---
> 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
>