ACPI: video: Fix use-after-free in acpi_video_switch_brightness()
Subsystems (2)
PATCH v2 1/1 ACPI: video: Fix use-after-free in acpi_video_switch_brightness()
The switch_brightness_work delayed work accesses device->brightness and device->backlight, which are freed by acpi_video_dev_unregister_backlight() during device removal.
If the work executes after acpi_video_bus_unregister_backlight() frees these resources, it causes a use-after-free when acpi_video_switch_brightness() dereferences device->brightness or device->backlight.
Fix this by calling cancel_delayed_work_sync() for each device's switch_brightness_work before unregistering its backlight resources. This ensures the work completes before the memory is freed.
---
Changes in v2:
- Move cancel_delayed_work_sync() to acpi_video_bus_unregister_backlight() instead of acpi_video_bus_put_devices() for better logic clarity and to prevent potential UAF of device->brightness
- Correct Fixes tag to point to 8ab58e8e7e097 which introduced the delayed work
- Link to v1: https://lore.kernel.org/all/[email protected]
--- drivers/acpi/acpi_video.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/acpi/acpi_video.c +3 -1
@@ -1869,8 +1869,10 @@ static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video) error =unregister_pm_notifier (&video->pm_nb);mutex_lock (&video->device_list_lock); -list_for_each_entry (dev, &video->video_device_list, entry) +list_for_each_entry (dev, &video->video_device_list, entry) { +cancel_delayed_work_sync (&dev->switch_brightness_work);acpi_video_dev_unregister_backlight (dev); + }mutex_unlock (&video->device_list_lock); video->backlight_registered = false;
Signature · 1 line
2.34.1
Hi Yuhao,
Quoted 9 lines · Yuhao Jiang
The
switch_brightness_workdelayed work accessesdevice->brightnessanddevice->backlight, which are freed byacpi_video_dev_unregister_backlight()during device removal.If the work executes after
acpi_video_bus_unregister_backlight()frees these resources, it causes a use-after-free whenacpi_video_switch_brightness()dereferencesdevice->brightnessordevice->backlight.
Quoted 7 lines
Fix this by calling
cancel_delayed_work_sync()for each device'sswitch_brightness_workbefore unregistering its backlight resources. This ensures the work completes before the memory is freed.Fixes 8ab58e8e7e097 ("ACPI / video: Fix backlight taking 2 steps on a brightness up/down keypress")Signed-off-by Yuhao Jiang <[email protected]>
Thank you for your patch, this is a good catch.
Quoted 17 lines
---
Changes in v2:
- Movecancel_delayed_work_sync()toacpi_video_bus_unregister_backlight()instead ofacpi_video_bus_put_devices()for better logic clarity and to prevent potential UAF ofdevice->brightness
- Correct Fixes tag to point to8ab58e8e7e097which introduced the delayed work
- Link to v1: https://lore.kernel.org/all/[email protected]--- drivers/acpi/acpi_video.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/acpi/acpi_video.c +0
-0@@ -1869,8 +1869,10 @@ static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video) error =unregister_pm_notifier (&video->pm_nb);
drivers/acpi/acpi_video.c static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video) · line 1878
Quoted 10 lines
drivers/acpi/acpi_video.c +3
-1mutex_lock (&video->device_list_lock); -list_for_each_entry (dev, &video->video_device_list, entry) +list_for_each_entry (dev, &video->video_device_list, entry) { +cancel_delayed_work_sync (&dev->switch_brightness_work);acpi_video_dev_unregister_backlight (dev); + }mutex_unlock (&video->device_list_lock); video->backlight_registered = false;
As you mention in your changelog, the cancel_delayed_work_sync() needs to happen before acpi_video_dev_unregister_backlight().
Since this needs to happen before unregistering things I think it would be more logical to put the cancel_delayed_work_sync(&dev->switch_brightness_work); call inside acpi_video_bus_remove_notify_handler().
So do the cancel in the loop there, directly after the acpi_video_dev_remove_notify_handler(dev) call which removes the handler which queues the work.
E.g. make the loop inside acpi_video_bus_remove_notify_handler() look like this:
mutex_lock(&video->device_list_lock);
list_for_each_entry(dev, &video->video_device_list, entry) {
acpi_video_dev_remove_notify_handler(dev);
cancel_delayed_work_sync(&dev->switch_brightness_work);
}
mutex_unlock(&video->device_list_lock);This cancels the work a bit earlier, but more importantly this feels like the more logical place to put the cancel call.
Regards,
Hans
Hi Hans,
Thanks for the feedback! I've submitted it in patch v3:
https://lore.kernel.org/all/[email protected]/.
Best regards,
Yuhao
Quoted 80 lines · Hans de Goede
Hi Yuhao,
Quoted 9 lines · Yuhao Jiang
The
switch_brightness_workdelayed work accessesdevice->brightnessanddevice->backlight, which are freed byacpi_video_dev_unregister_backlight()during device removal.If the work executes after
acpi_video_bus_unregister_backlight()frees these resources, it causes a use-after-free whenacpi_video_switch_brightness()dereferencesdevice->brightnessordevice->backlight.Quoted 7 lines
Fix this by calling
cancel_delayed_work_sync()for each device'sswitch_brightness_workbefore unregistering its backlight resources. This ensures the work completes before the memory is freed.Fixes 8ab58e8e7e097 ("ACPI / video: Fix backlight taking 2 steps on a brightness up/down keypress")Signed-off-by Yuhao Jiang <[email protected]>Thank you for your patch, this is a good catch.
Quoted 17 lines
---
Changes in v2:
- Movecancel_delayed_work_sync()toacpi_video_bus_unregister_backlight()instead ofacpi_video_bus_put_devices()for better logic clarity and to prevent potential UAF ofdevice->brightness
- Correct Fixes tag to point to8ab58e8e7e097which introduced the delayed work
- Link to v1: https://lore.kernel.org/all/[email protected]--- drivers/acpi/acpi_video.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/acpi/acpi_video.c +0
-0@@ -1869,8 +1869,10 @@ static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video) error =unregister_pm_notifier (&video->pm_nb);drivers/acpi/acpi_video.c static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video) · line 1878
Quoted 10 lines
drivers/acpi/acpi_video.c +3
-1mutex_lock (&video->device_list_lock); -list_for_each_entry (dev, &video->video_device_list, entry) +list_for_each_entry (dev, &video->video_device_list, entry) { +cancel_delayed_work_sync (&dev->switch_brightness_work);acpi_video_dev_unregister_backlight (dev); + }mutex_unlock (&video->device_list_lock); video->backlight_registered = false;As you mention in your changelog, the
cancel_delayed_work_sync()needs to happen beforeacpi_video_dev_unregister_backlight().Since this needs to happen before unregistering things I think it would be more logical to put the
cancel_delayed_work_sync(&dev->switch_brightness_work); call insideacpi_video_bus_remove_notify_handler().So do the cancel in the loop there, directly after the
acpi_video_dev_remove_notify_handler(dev)call which removes the handler which queues the work.E.g. make the loop inside
acpi_video_bus_remove_notify_handler()look like this:mutex_lock(&video->device_list_lock); list_for_each_entry(dev, &video->video_device_list, entry) { acpi_video_dev_remove_notify_handler(dev); cancel_delayed_work_sync(&dev->switch_brightness_work); } mutex_unlock(&video->device_list_lock);This cancels the work a bit earlier, but more importantly this feels like the more logical place to put the cancel call.
Regards,
Hans