All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation
@ 2024-03-27 14:24 Bastien Nocera
  2024-03-27 15:07 ` bluez.test.bot
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Bastien Nocera @ 2024-03-27 14:24 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Gui-Dong Han

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* RE: Bluetooth: Fix TOCTOU in HCI debugfs implementation
  2024-03-27 14:24 [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation Bastien Nocera
@ 2024-03-27 15:07 ` bluez.test.bot
  2024-03-27 15:59 ` [PATCH] " patchwork-bot+bluetooth
  2024-03-29  3:06 ` Gui-Dong Han
  2 siblings, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2024-03-27 15:07 UTC (permalink / raw)
  To: linux-bluetooth, hadess

[-- Attachment #1: Type: text/plain, Size: 2767 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=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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation
  2024-03-27 14:24 [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation Bastien Nocera
  2024-03-27 15:07 ` bluez.test.bot
@ 2024-03-27 15:59 ` patchwork-bot+bluetooth
  2024-03-29  3:06 ` Gui-Dong Han
  2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+bluetooth @ 2024-03-27 15:59 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: linux-bluetooth, 2045gemini

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation
  2024-03-27 14:24 [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation Bastien Nocera
  2024-03-27 15:07 ` bluez.test.bot
  2024-03-27 15:59 ` [PATCH] " patchwork-bot+bluetooth
@ 2024-03-29  3:06 ` Gui-Dong Han
  2 siblings, 0 replies; 4+ messages in thread
From: Gui-Dong Han @ 2024-03-29  3:06 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: linux-bluetooth

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
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-03-29  3:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-27 14:24 [PATCH] Bluetooth: Fix TOCTOU in HCI debugfs implementation Bastien Nocera
2024-03-27 15:07 ` bluez.test.bot
2024-03-27 15:59 ` [PATCH] " patchwork-bot+bluetooth
2024-03-29  3:06 ` Gui-Dong Han

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.