| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5bd75fc2-56e9-4e58-a18a-29afd07fe0df@quicinc.com>
Date: Thu, 13 Feb 2025 02:39:06 +0530
From: Manaf Meethalavalappu Pallikunhi <quic_manafm@...cinc.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: Daniel Lezcano <daniel.lezcano@...aro.org>,
Zhang Rui
<rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] thermal: core: Fix race between zone registration and
userspace sysfs access
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@...cinc.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@...cinc.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
Powered by blists - more mailing lists