1. Patchew
  2. linux
  3. View series

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

Manaf Meethalavalappu Pallikunhi posted 1 patch 10 months, 1 week ago 
drivers/thermal/thermal_core.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
[PATCH] thermal: core: Fix race between zone registration and userspace sysfs access
Posted by Manaf Meethalavalappu Pallikunhi 10 months, 1 week ago 
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:
--
Re: [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access
Posted by Rafael J. Wysocki 10 months, 1 week ago 
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'd rather make the attached change, please let me know if it works for you.
Re: [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access
Posted by Manaf Meethalavalappu Pallikunhi 10 months, 1 week ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.