-
Notifications
You must be signed in to change notification settings - Fork 12
[FIPS 9.2 Compliant] CVE-2024-58002 #603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fips-9-compliant/5.14.0-284.30.1
Are you sure you want to change the base?
[FIPS 9.2 Compliant] CVE-2024-58002 #603
Conversation
jira VULN-53466 cve-pre CVE-2024-58002 commit-author Ricardo Ribalda <[email protected]> commit 64627da Avoid using the iterators after the list_for_each() constructs. This patch should be a NOP, but makes cocci, happier: drivers/media/usb/uvc/uvc_ctrl.c:1861:44-50: ERROR: invalid reference to the index variable of the iterator on line 1850 drivers/media/usb/uvc/uvc_ctrl.c:2195:17-23: ERROR: invalid reference to the index variable of the iterator on line 2179 Reviewed-by: Sergey Senozhatsky <[email protected]> Reviewed-by: Laurent Pinchart <[email protected]> Signed-off-by: Ricardo Ribalda <[email protected]> Signed-off-by: Hans Verkuil <[email protected]> (cherry picked from commit 64627da) Signed-off-by: Jonathan Maple <[email protected]>
jira VULN-53466 cve-pre CVE-2024-58002 commit-author Ricardo Ribalda <[email protected]> commit d9fecd0 Now we keep a reference to the active fh for any call to uvc_ctrl_set, regardless if it is an actual set or if it is a just a try or if the device refused the operation. We should only keep the file handle if the device actually accepted applying the operation. Cc: [email protected] Fixes: e5225c8 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Suggested-by: Hans de Goede <[email protected]> Reviewed-by: Hans de Goede <[email protected]> Reviewed-by: Laurent Pinchart <[email protected]> Signed-off-by: Ricardo Ribalda <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Laurent Pinchart <[email protected]> Signed-off-by: Mauro Carvalho Chehab <[email protected]> (cherry picked from commit d9fecd0) Signed-off-by: Jonathan Maple <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses CVE-2024-58002 by implementing a comprehensive fix for dangling pointer issues in the UVC video driver. The changes introduce proper reference counting and cleanup mechanisms for asynchronous control handles to prevent use-after-free vulnerabilities.
Key changes:
- Added reference counting for pending asynchronous controls per file handle
- Implemented proper cleanup of dangling pointers when file handles are released
- Refactored control handle management with thread-safe operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
drivers/media/usb/uvc/uvcvideo.h | Added pending_async_ctrls field and uvc_ctrl_cleanup_fh declaration |
drivers/media/usb/uvc/uvc_v4l2.c | Added cleanup call in file release handler |
drivers/media/usb/uvc/uvc_ctrl.c | Implemented reference counting and cleanup logic for control handles |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a6c2456
to
7775b45
Compare
I would say this was a test to catch Co-Pilot and reviewers but in reality it was me moving too fast with too many spinning plates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥌
drivers/media/usb/uvc/uvc_ctrl.c
Outdated
if (!handle->pending_async_ctrls){ | ||
mutex_unlock(&handle->chain->ctrl_mutex); | ||
return; | ||
} | ||
|
||
list_for_each_entry(entity, &handle->chain->dev->entities, list) { | ||
for (unsigned int i = 0; i < entity->ncontrols; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are differences here compared to the 5.15 version of this commit:
$ interdiffbkpt 7775b458062116b21c5a65c0366cfa3ae2d98161 117f7a2975ba
diff -u b/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
--- b/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2614,13 +2614,14 @@
mutex_lock(&handle->chain->ctrl_mutex);
- if (!handle->pending_async_ctrls){
+ if (!handle->pending_async_ctrls) {
mutex_unlock(&handle->chain->ctrl_mutex);
return;
}
list_for_each_entry(entity, &handle->chain->dev->entities, list) {
- for (unsigned int i = 0; i < entity->ncontrols; ++i) {
+ unsigned int i;
+ for (i = 0; i < entity->ncontrols; ++i) {
if (entity->controls[i].handle != handle)
continue;
uvc_ctrl_set_handle(handle, &entity->controls[i], NULL);
Apply that interdiff to make this patch the same as the 5.15 version, or just re-pick using the 5.15 SHA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats correct this kernel contains the -std=gnu11
commit
c1bb5aa
commit c1bb5aaa977654cc83839a6269a97302db05e6e4
Author: Waiman Long <[email protected]>
Date: Tue Jun 7 17:44:36 2022 -0400
Kbuild: move to -std=gnu11
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2074118
Conflicts: The hunk for zh_TW/process/programming-language.rst is dropped
as the file isn't in RHEL9.
commit e8c07082a810fbb9db303a2b66b66b8d7e588b53
Author: Arnd Bergmann <[email protected]>
Date: Tue, 8 Mar 2022 22:56:14 +0100
Kbuild: move to -std=gnu11
[jmaple@devbox kernel-src-tree]$ git describe --contains c1bb5aaa977654cc83839a6269a97302db05e6e4
kernel-5.14.0-121.el9~7^2~1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its clear though that my `upstream-diff is insufficient though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upstream-diff - Refenced kernel-lt 5.15 commit 117f7a2975baa4b7d702d3f4830d5a4ebd0c6d50
This is due to missing:
- 54da6a092431 - locking: Introduce __cleanup() based infrastructure
Leaving the loop as the mainline since this kernel branch contains:
- e8c07082a810 - Kbuild: move to -std=gnu11 at c1bb5aaa9776
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the {
issue on the if statement I'm not changing the for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats correct this kernel contains the -std=gnu11 commit
It does, but the code doesn't perfectly align with anything upstream anymore: it's a hybrid between the 5.15 patch and the mainline patch. IMO, for situations like these, it is better to choose one or the other in its entirety instead of diverging from both.
7775b45
to
c48b137
Compare
jira VULN-53466 cve CVE-2024-58002 commit-author Ricardo Ribalda <[email protected]> commit 221cd51 upstream-diff - Refenced kernel-lt 5.15 commit 117f7a2 This is due to missing: - 54da6a0 - locking: Introduce __cleanup() based infrastructure Leaving the loop as the mainline since this kernel branch contains: - e8c0708 - Kbuild: move to -std=gnu11 at c1bb5aa When an async control is written, we copy a pointer to the file handle that started the operation. That pointer will be used when the device is done. Which could be anytime in the future. If the user closes that file descriptor, its structure will be freed, and there will be one dangling pointer per pending async control, that the driver will try to use. Clean all the dangling pointers during release(). To avoid adding a performance penalty in the most common case (no async operation), a counter has been introduced with some logic to make sure that it is properly handled. Cc: [email protected] Fixes: e5225c8 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Reviewed-by: Hans de Goede <[email protected]> Signed-off-by: Ricardo Ribalda <[email protected]> Reviewed-by: Laurent Pinchart <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Laurent Pinchart <[email protected]> Signed-off-by: Mauro Carvalho Chehab <[email protected]> (cherry picked from commit 221cd51) Signed-off-by: Jonathan Maple <[email protected]>
c48b137
to
98202de
Compare
BUILD
KselfTest
KselfTest Diff Experimental