Re: [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access

[Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>]

Hi Rafael,

Thank you for reviewing the patch.

On 2/13/2025 1:12 AM, Rafael J. Wysocki wrote:
> On Tue, Feb 11, 2025 at 1:45 PM Manaf Meethalavalappu Pallikunhi
> <quic_manafm@quicinc.com> wrote:
>> Currently, the thermal zone sysfs is created before setting the
>> governor for that thermal zone during registration. If a thermal
>> zone is being registered while a userspace module tries to access
>> the same thermal zone policy sysfs node, it can lead to a potential
>> NULL pointer dereference issue in the policy sysfs path.
>>
>> To avoid this race condition, set the thermal zone governor first
>> before enabling the thermal zone sysfs during registration.
>> This change fixes below issue,
>>
>> [ 20.964589]   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> [ 21.049645]   pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
>> [ 21.049647]   pc : policy_show+0x1c/0x3c
>> [ 21.049652]   lr : dev_attr_show+0x38/0x7c
>> [ 21.049655]   sp : ffffffc09a98bbf0
>> [ 21.049657]   x29: ffffffc09a98bbf0 x28: ffffff885b940000 x27: 0000000000000000
>> [ 21.049660]   x26: 0000000000000000 x25: 000000007ffff001 x24: 0000000000000001
>> [ 21.049664]   x23: ffffffdca6334c78 x22: ffffff88a2b2fe00 x21: ffffff881cee8000
>> [ 21.049667]   x20: ffffff8868318018 x19: ffffffdca7640d78 x18: ffffffdca74d94c0
>> [ 21.049670]   x17: 00000000ae84bcd4 x16: 00000000ae84bcd4 x15: 000000002df29963
>> [ 21.049673]   x14: 00000000cbef29c7 x13: 000000004e61db0a x12: ffffff885b940be0
>> [ 21.049677]   x11: ffffff881cee8000 x10: 0000000000000000 x9 : ffffffdca59f00b8
>> [ 21.049680]   x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
>> [ 21.049683]   x5 : 0000000000000040 x4 : 0000000000000000 x3 : 0000000000000004
>> [ 21.049686]   x2 : ffffff881cee8000 x1 : ffffffdca66e5bfb x0 : ffffff881cee8000
>> [ 21.049689]   Call trace:
>> [ 21.049690]    policy_show+0x1c/0x3c
>> [ 21.049692]    dev_attr_show+0x38/0x7c
>> [ 21.049695]    sysfs_kf_seq_show+0xd8/0x160
>> [ 21.049699]    kernfs_seq_show+0x44/0x54
>> [ 21.049701]    seq_read_iter+0x16c/0x4ec
>> [ 21.049705]    kernfs_fop_read_iter+0x64/0x1d8
>> [ 21.049709]    vfs_read+0x2d8/0x33c
>> [ 21.049711]    ksys_read+0x78/0xe8
>> [ 21.049714]    __arm64_sys_read+0x1c/0x2c
>> [ 21.049716]    invoke_syscall+0x58/0x10c
>> [ 21.049719]    el0_svc_common+0xa8/0xdc
>> [ 21.049722]    do_el0_svc+0x1c/0x28
>> [ 21.049724]    el0_svc+0x40/0x90
>> [ 21.049726]    el0t_64_sync_handler+0x70/0xbc
>> [ 21.049728]    el0t_64_sync+0x1a8/0x1ac
>> [ 21.049731]   Code: f9435008 aa0203e0 d00054e1 912fec21 (f9400108)
>>
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
>> ---
>>   drivers/thermal/thermal_core.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 2328ac0d8561..c6e6b229cc6e 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1589,6 +1589,11 @@ thermal_zone_device_register_with_trips(const char *type,
>>
>>          tz->state = TZ_STATE_FLAG_INIT;
>>
>> +       thermal_zone_device_init(tz);
>> +       result = thermal_zone_init_governor(tz);
>> +       if (result)
>> +               goto unregister;
>> +
>>          /* sys I/F */
>>          /* Add nodes that are always present via .groups */
>>          result = thermal_zone_create_device_groups(tz);
>> @@ -1600,19 +1605,14 @@ thermal_zone_device_register_with_trips(const char *type,
>>                  thermal_zone_destroy_device_groups(tz);
>>                  goto remove_id;
>>          }
>> -       thermal_zone_device_init(tz);
>>          result = device_register(&tz->device);
>>          if (result)
>>                  goto release_device;
>>
>> -       result = thermal_zone_init_governor(tz);
>> -       if (result)
>> -               goto unregister;
>> -
>>          if (!tz->tzp || !tz->tzp->no_hwmon) {
>>                  result = thermal_add_hwmon_sysfs(tz);
>>                  if (result)
>> -                       goto unregister;
>> +                       goto release_device;
>>          }
>>
>>          result = thermal_thresholds_init(tz);
>> @@ -1629,12 +1629,12 @@ thermal_zone_device_register_with_trips(const char *type,
>>
>>   remove_hwmon:
>>          thermal_remove_hwmon_sysfs(tz);
>> -unregister:
>> -       device_del(&tz->device);
>>   release_device:
>>          put_device(&tz->device);
>>   remove_id:
>>          ida_free(&thermal_tz_ida, id);
>> +unregister:
>> +       device_del(&tz->device);
>>   free_tzp:
>>          kfree(tz->tzp);
>>   free_tz:
>> --
> The catch is good, but the patch isn't AFAICS.  The changes in the
> error path don't look correct to me in particular.
I understood the issues in the error path. I’ll try to fix them in the 
v2 patch. Is there any concern with setting the governor before 
initializing sysfs ?
>
> I'd rather make the attached change, please let me know if it works for you.

Yes, It will work for current issue. But it could provide a incorrect 
policy information to userspace if a thermal zone is created with a 
governor other than the default one (def_governor), right ?

Thanks,

Manaf