Re: [PATCH v2] ACPI: video: Fix use-after-free in acpi_video_switch_brightness()

Hi Hans,

Thanks for the feedback! I've submitted it in patch v3: https://lore.kernel.org/all/20251022200704.2655507-1-danisjiang@gmail.com/.

Best regards, Yuhao

On Wed, Oct 22, 2025 at 4:28 AM Hans de Goede hansg@kernel.org wrote:

Hi Yuhao,

On 22-Oct-25 6:25 AM, Yuhao Jiang wrote:

...

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.

Fixes: 8ab58e8e7e097 ("ACPI / video: Fix backlight taking 2 steps on a brightness up/down keypress") Cc: stable@vger.kernel.org Signed-off-by: Yuhao Jiang danisjiang@gmail.com

Thank you for your patch, this is a good catch.

...

---

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/20251022040859.2102914-1-danisjiang@gmail.com

---

drivers/acpi/acpi_video.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 103f29661576..64709658bdc4 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -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;
  

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