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

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

On 2026-03-25 17:20, Rafael J. Wysocki wrote:
> On Wed, Mar 25, 2026 at 4:13 PM Mauricio Faria de Oliveira
> <mfo@igalia.com> wrote:
>>
>> On 2026-03-25 11:28, Mauricio Faria de Oliveira wrote:
>> > On 2026-03-25 11:17, Mauricio Faria de Oliveira wrote:
>> >> Thanks for looking into this.
>> >>
>> >> On 2026-03-25 09:47, Rafael J. Wysocki wrote:
>> >>> 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.
>> >>
>> >
>> > Please disregard this paragraph; I incorrectly read/wrote _resume()
>> > as thermal_zone_pm_complete() discussed above. The rest should be
>> > right. I'll review this and get back shortly.
>> >
>> >> 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.
>>
>> Ok, similarly:
>>
>> Say, thermal_pm_notify() -> thermal_pm_notify_complete() ->
>> thermal_zone_pm_complete()
>> run before thermal_zone_device_unregister() is called;
>> thermal_zone_device_resume()
>> starts, and by now thermal_zone_device_unregister() is called.
>>
>> If thermal_zone_device_resume() wins the race over thermal_zone_exit()
>> for guard(thermal_zone(tz) (tz->lock), it sees TZ_STATE_FLAG_EXIT clear;
>> note its callees (eg, thermal_zone_device_init()) run with tz->lock
>> held,
>> so they see it clear as well.
>>
>> So, thermal_zone_device_init() calls INIT_DELAYED_WORK(), everything
>> returns, tz->lock is released and the thermal_zone_device_unregister()
>> -> thermal_zone_exit() path can continue to run.
>>
>> Only now thermal_zone_exit() sets TZ_STATE_FLAG_EXIT (too late),
>> returns.
>> cancel_delayed_work_sync() does not wait for
>> thermal_zone_device_resume()
>> due to INIT_DELAYED_WORK() in thermal_zone_device_init(); and kfree(tz).
> 
> Wait.
> 
> I'm not sure why this matters because the zone lock is dropped at the
> end of thermal_zone_device_resume() at which point it has done
> everything it had to do.  Then, the cancel_delayed_work_sync() in
> thermal_zone_device_unregister() should properly wait for the work
> item queued up by thermal_zone_device_resume().

Oh, indeed, you're right.

>> Then, thermal_zone_device_resume() accesses tz and hits use-after-free.
> 
> Say thermal_zone_device_resume() has lost the race for tz->lock to
> thermal_zone_exit() though.  It runs after the latter and
> reinitializes the delayed work.  Now, thermal_zone_device_unregister()
> continues and cancel_delayed_work_sync() doesn't see the
> thermal_zone_device_resume() running in parallel with it any more, so
> it doesn't wait and if thermal_zone_device_unregister() continues to
> run, it may free tz prematurely.
> 
> I claim that this can be prevented by adding a tz->state check to
> thermal_zone_device_resume() before the reinitialization of the
> delayed work.  Namely, if it loses the race tz->lock to
> thermal_zone_exit(), the new check will trigger and the delayed work
> will not be reinitialized, so the cancel_delayed_work_sync() will wait
> for it to complete.

I see it in this case now, thanks for clarifying.

-- 
Mauricio