Re: [PATCH] thermal: core: fix use-after-free due to init/cancel delayed_work race

[Mauricio Faria de Oliveira <mfo@igalia.com>]

Thanks for looking into this.

On 2026-03-25 09:47, Rafael J. Wysocki wrote:
> On Wed, Mar 25, 2026 at 1:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Wed, Mar 25, 2026 at 12:51 AM Mauricio Faria de Oliveira
>> <mfo@igalia.com> wrote:
>> >
>> > If INIT_DELAYED_WORK() is called for a currently running work item,
>> > cancel_delayed_work_sync() is unable to cancel/wait for it anymore,
>> > as the work item's data bits required for that are cleared.
>> >
>> > In the resume path, INIT_DELAYED_WORK() is called twice:
>> > 1) to replace the work function: thermal_zone_device_check/resume()
>> > 2) to restore it.
>> >
>> > Both cases might race with the unregister path and bypass the call to
>> > cancel_delayed_work_sync(),
>>
>> So this is the problem, isn't it?
>>
>> > after which struct thermal_zone_device *tz
>> > is freed, and the non-canceled/non-waited for work hits use-after-free.
>>
>> Which basically means that a TZ_STATE_FLAG_EXIT check is missing in
>> both thermal_zone_pm_complete() and thermal_zone_device_resume().
> 
> Actually, thermal_zone_pm_complete() runs under thermal_list_lock and
> thermal_zone_device_unregister() removes the zone from
> thermal_tz_list, also under thermal_list_lock, before calling
> cancel_delayed_work_sync().
> 
> So either thermal_zone_device_unregister() removes the zone from the
> list before thermal_zone_pm_complete() can run, in which case the
> latter won't run for the given zone at all because that zone is not
> there in thermal_tz_list, or the cancel_delayed_work_sync() will see
> the work item queued up by thermal_zone_pm_complete().

(I think this reply clarifies your previous points too; or tell me.)

Regarding the latter: yes, cancel_delayed_work_sync() will see the
work item queued up by thermal_zone_pm_complete(), but the issue is
it will not see the _previously queued up (and running)_ work item,
due to the INIT_DELAYED_WORK() in thermal_zone_pm_complete().

That work item, thermal_zone_device_check() might be past a check
for TZ_STATE_FLAG_EXIT by the time it is set. Please read on.

> So where's the race between thermal_zone_pm_complete() and
> thermal_zone_device_unregister()?

I'll append a more detailed view below, but the summary is:

thermal_pm_notify_complete() can acquire thermal_list_lock before
thermal_zone_device_unregister() -> thermal_zone_exit() does,
thus proceed to thermal_zone_pm_complete() which calls
INIT_DELAYED_WORK().

This makes future calls to cancel_delayed_work_sync() to wait only
for the newly initialized work function, not a previous one, which
might be running -- in this case, thermal_zone_device_check().

Then thermal_zone_pm_complete() and thermal_pm_notify_complete()
return, releasing thermal_list_lock.

Now, thermal_zone_device_unregister() -> thermal_zone_exit()
can set finally set TZ_STATE_FLAG_EXIT, but it might be too late.

The previously queued, currently running, work item might be past
any check for it (say, it acquired the lock to read it earlier,
or before thermal_zone_device_unregister() was even called).

So, thermal_zone_device_unregister() reaches cancel_delayed_work_sync(),
which waits for thermal_zone_device_resume() (new work function),
but _not_ for thermal_zone_device_check() (old work function).

That finishes, tz is freed, and now the old work funtion continues
to run and accesses tz -- hitting a use-after-free.

> I can see the one between thermal_zone_device_unregister() and
> thermal_zone_device_resume(), but that can be addressed by adding a
> TZ_STATE_FLAG_EXIT check to the latter AFAICS.

In the example describe above and detailed below, apparently that
is not sufficient, if I'm not missing anything. See, if _resume()
is reached with thermal_list_lock held, thermal_zone_device_exit()
is waiting for thermal_list_lock before setting TZ_STATE_FLAG_EXIT,
thus a check for it in _resume() would find it clear yet.

Hope this helps. Please let me know your thoughts. Thanks!

Detailed view)

Thread A:
  ...
  WORK: tz->poll_queue
    thermal_zone_device_check() // started running before
TZ_STATE_FLAG_EXIT could be set  
      ... // wait a bit
      
Thread B:

  thermal_pm_notify()
    thermal_pm_notify_complete()
      guard(mutex)(&thermal_list_lock)  // acquires the lock
      ...
      
Thread C:

  thermal_zone_device_unregister()
    thermal_zone_exit()
      guard(mutex)(&thermal_list_lock)  // blocks waiting for the lock
        ...
        tz->state |= TZ_STATE_FLAG_EXIT // not run yet
        list_del_init(&tz->node)        // not run yet

Thread B: // continues with tz in thermal_tz_list and without
TZ_STATE_FLAG_EXIT
      ...
      list_for_each_entry(tz, &thermal_tz_list, node)
        thermal_zone_pm_complete(tz)
          cancel_delayed_work(&tz->poll_queue) // does not wait for
thermal_zone_device_check() to finish
          INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_resume)
      ...
      returns, releases the thermal_list_lock.
      
From now on, cancel_delayed_work_sync(&tz->poll_queue)
will _not_ wait for thermal_zone_device_check() anymore 
(note it _is_ running in Thread A)
it will only wait for thermal_zone_device_resume().

Thread C:

        tz->state |= TZ_STATE_FLAG_EXIT
        list_del_init(&tz->node)

These are now set, but no longer effective for the already running
thermal_zone_device_check().

      cancel_delayed_work_sync(&tz->poll_queue) // as mentioned, waits
for the other function.
      kfree(tz)
      
Thread A: // not waited for
       ...
       thermal_zone_device_update()
         // access to tz (freed)

-- 
Mauricio