5.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Bastien Nocera hadess@hadess.net
commit 7835fcfd132eb88b87e8eb901f88436f63ab60f7 upstream.
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@gma... Link: https://nvd.nist.gov/vuln/detail/CVE-2024-24858 Link: https://lore.kernel.org/linux-bluetooth/20231222162931.6553-1-2045gemini@gma... Link: https://lore.kernel.org/linux-bluetooth/20231222162310.6461-1-2045gemini@gma... 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 Signed-off-by: Luiz Augusto von Dentz luiz.von.dentz@intel.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- net/bluetooth/hci_debugfs.c | 48 +++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 16 deletions(-)
--- a/net/bluetooth/hci_debugfs.c +++ b/net/bluetooth/hci_debugfs.c @@ -200,10 +200,12 @@ static int conn_info_min_age_set(void *d { 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);
@@ -228,10 +230,12 @@ static int conn_info_max_age_set(void *d { 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);
@@ -508,10 +512,12 @@ static int sniff_min_interval_set(void * { 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);
@@ -536,10 +542,12 @@ static int sniff_max_interval_set(void * { 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);
@@ -780,10 +788,12 @@ static int conn_min_interval_set(void *d { 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);
@@ -808,10 +818,12 @@ static int conn_max_interval_set(void *d { 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);
@@ -920,10 +932,12 @@ static int adv_min_interval_set(void *da { 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);
@@ -948,10 +962,12 @@ static int adv_max_interval_set(void *da { 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);