Re: [PATCH v1 2/2] Revert "thermal: hwmon: Register a hwmon device for each thermal zone"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The error handling path in `thermal_add_hwmon_sysfs()` unconditionally frees a shared `hwmon` device, causing a Use-After-Free vulnerability.
- [High] Lack of reference counting and inadequate lock scope for shared `hwmon` devices lead to TOCTOU races, duplicate device registration, and Use-After-Free vulnerabilities.
- [Medium] Unprotected concurrent modification of `hwmon->count` causes data races and sysfs file name collisions.
--

--- Patch [2]: [PATCH v1 2/2] Revert "thermal: hwmon: Register a hwmon device for each thermal zone" ---
commit 811b1574f43c922402d0fcdcdca33812b6bd90b8
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Revert "thermal: hwmon: Register a hwmon device for each thermal zone"

This commit reverts a previous change to thermal zone hwmon device names and
sysfs layout, restoring the behavior where thermal zone devices with the same
type share one hwmon device.

Link: https://lore.kernel.org/linux-pm/cafd8af9-c6e9-4bf2-b496-23e796fbc9a6@linux.dev/
Link: https://lore.kernel.org/linux-hwmon/ab8b093b-46e6-4738-afcf-4b97c9ad5af9@googlemail.com/

> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> index 223ae1571655b..597c33c8a5550 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c

[ ... ]

> @@ -91,6 +88,45 @@ temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
>  	return sysfs_emit(buf, "%d\n", temperature);
>  }
>  
> +
> +static struct thermal_hwmon_device *
> +thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
> +{
> +	struct thermal_hwmon_device *hwmon;
> +	char type[THERMAL_NAME_LENGTH];
> +
> +	mutex_lock(&thermal_hwmon_list_lock);
> +	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
> +		strscpy(type, tz->type);
> +		strreplace(type, '-', '_');
> +		if (!strcmp(hwmon->type, type)) {
> +			mutex_unlock(&thermal_hwmon_list_lock);
> +			return hwmon;

[Severity: High]
Does this introduce a race condition resulting in a use-after-free?

If a thread calls thermal_hwmon_lookup_by_type(), the lock is dropped before
returning the shared hwmon pointer. If another thread concurrently removes the
last thermal zone for this type via thermal_remove_hwmon_sysfs(), it could
free the hwmon device while the first thread is still executing
thermal_add_hwmon_sysfs().

Would it be safer to hold a reference count on the shared hwmon object before
dropping thermal_hwmon_list_lock?

> +		}
> +	}
> +	mutex_unlock(&thermal_hwmon_list_lock);
> +
> +	return NULL;
> +}
> +

[ ... ]

> @@ -101,39 +137,54 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)

[ ... ]

> + register_sys_interface:
> +	temp = kzalloc_obj(*temp);
> +	if (!temp) {
> +		result = -ENOMEM;
> +		goto unregister_name;
> +	}
> +
> +	temp->tz = tz;
> +	hwmon->count++;
> +
> +	snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
> +		 "temp%d_input", hwmon->count);

[Severity: Medium]
Is it possible for concurrent probes to race on hwmon->count?

If two threads execute thermal_add_hwmon_sysfs() for the same thermal zone
type simultaneously, they could increment hwmon->count outside of any lock.
Could this lead to both threads formatting identical sysfs attribute names
and causing device_create_file() to fail with -EEXIST?

> +	temp->temp_input.attr.attr.name = temp->temp_input.name;
> +	temp->temp_input.attr.attr.mode = 0444;

[ ... ]

> @@ -145,17 +196,21 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)

[ ... ]

>   unregister_input:
>  	device_remove_file(hwmon->device, &temp->temp_input.attr);
> + free_temp_mem:
> +	kfree(temp);
>   unregister_name:
> -	hwmon_device_unregister(hwmon->device);
> +	if (new_hwmon_device)
> +		hwmon_device_unregister(hwmon->device);
>   free_mem:
>  	kfree(hwmon);

[Severity: High]
Will this unconditionally free the shared hwmon object on an error path?

If new_hwmon_device is 0 (meaning the hwmon device was found via
thermal_hwmon_lookup_by_type and is shared), and an error occurs during sysfs
file creation, execution jumps to free_mem. Calling kfree(hwmon) here would
destroy the shared object while it is still in use by other thermal zones.

Should this kfree() be conditional on new_hwmon_device being true?

> -	return 0;
> +	return result;
>  }
>  EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysfs);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6319276.lOV4Wx5bFT@rafael.j.wysocki?part=2