| Message ID | 20240327142526.332756-1-hadess@hadess.net |
|---|---|
| State | Accepted |
| Commit | 3b29aabb428808239f5773a0677303b6b1672bd0 |
| Headers | show |
| Series | Bluetooth: Fix TOCTOU in HCI debugfs implementation | expand |
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);