Re: [PATCH v2 1/2] Revert "thermal/drivers/hwmon: Cleanup coding style a bit"

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:

Pre-existing issues:
- [High] Lack of synchronization during hwmon device lookup and creation re=
sults in Use-After-Free of `hwmon` and Time-of-Check to Time-of-Use (TOCTOU=
) list duplication.
- [High] The hwmon sysfs attribute callbacks do not use `hwmon_lock()` as r=
equired by the subsystem guidelines.
--

--- Patch [1]: [PATCH v2 1/2] Revert "thermal/drivers/hwmon: Cleanup coding=
 style a bit" ---
commit 85ae4605921a40cd95c1000cfc56b0df8bf8c9f1
Author: Rafael J. Wysocki <[email protected]>
Subject: Revert "thermal/drivers/hwmon: Cleanup coding style a bit"

Reverts a commit that introduced a use-after-free in the error path of
thermal_add_hwmon_sysfs() by removing a valid check.

Link: https://lore.kernel.org/linux-hwmon/20260803183915.4ED7D1F000E9@smtp.=
kernel.org/

> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hw=
mon.c
> index 597c33c8a5550..ec73d03a1e60a 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
> @@ -212,7 +212,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_devic=
e *tz)
>  	if (new_hwmon_device)
>  		hwmon_device_unregister(hwmon->device);
>   free_mem:
> -	kfree(hwmon);
> +	if (new_hwmon_device)
> +		kfree(hwmon);
> =20
>  	return result;
>  }

[Severity: High]
This is a pre-existing issue, but is there a use-after-free and TOCTOU race
when looking up and adding a new hwmon device?

If one thread calls thermal_add_hwmon_sysfs() and looks up the hwmon device
via thermal_hwmon_lookup_by_type(), the list lock is dropped and an
unrefcounted pointer is returned:

drivers/thermal/thermal_hwmon.c:thermal_hwmon_lookup_by_type() {
    ...
		if (!strcmp(hwmon->type, type)) {
			mutex_unlock(&thermal_hwmon_list_lock);
			return hwmon;
		}
    ...
}

Concurrently, another thread could call thermal_remove_hwmon_sysfs() for the
last thermal zone sharing that hwmon device. It removes the last entry,
drops the list lock, unregisters the device, and frees the hwmon pointer
without synchronizing with active readers:

drivers/thermal/thermal_hwmon.c:thermal_remove_hwmon_sysfs() {
    ...
	if (!list_empty(&hwmon->tz_list)) {
		mutex_unlock(&thermal_hwmon_list_lock);
		return;
	}
	list_del(&hwmon->node);
	mutex_unlock(&thermal_hwmon_list_lock);

	hwmon_device_unregister(hwmon->device);
	kfree(hwmon);
}

The first thread could then resume in thermal_add_hwmon_sysfs() and
dereference the freed pointer when incrementing the count:

drivers/thermal/thermal_hwmon.c:thermal_add_hwmon_sysfs() {
    ...
 register_sys_interface:
	temp =3D kzalloc_obj(*temp);
	if (!temp) {
		result =3D -ENOMEM;
		goto unregister_name;
	}

	temp->tz =3D tz;
	hwmon->count++;
    ...
}

Could this lead to memory corruption during concurrent device probing or
unbinding?

[Severity: High]
This is a pre-existing issue, but do the hwmon sysfs attribute callbacks
need to use hwmon_lock() and hwmon_unlock() wrappers?

The hwmon subsystem guidelines require drivers to implement locking for
attributes registered by other means than hwmon_device_register_with_info().

The temp_input_show() and temp_crit_show() callbacks read attributes direct=
ly
without the required lock:

drivers/thermal/thermal_hwmon.c:temp_input_show() {
    ...
	struct thermal_zone_device *tz =3D temp->tz;

	ret =3D thermal_zone_get_temp(tz, &temperature);
    ...
}

drivers/thermal/thermal_hwmon.c:temp_crit_show() {
    ...
	struct thermal_zone_device *tz =3D temp->tz;
	int temperature;
	int ret;

	guard(thermal_zone)(tz);

	ret =3D tz->ops.get_crit_temp(tz, &temperature);
    ...
}

Could this result in a race condition with interrupt handlers or driver
unregistration since it bypasses the core kernel locking rules for the hwmon
subsystem?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/12947893.O9o76ZdvQC=
@rafael.j.wysocki?part=3D1