Re: [PATCH v1 07/14] hwmon:: Use non-OF thermal cooling device register function

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] This problem wasn't introduced by this patch, but in mlxreg-fan.c, the hwmon sysfs interfaces are exposed before the thermal cooling device pointer (pwm->cdev) is initialized, leading to a possible NULL pointer dereference.
- [High] This problem wasn't introduced by this patch, but in mlxreg-fan.c, the variables pwm->last_hwmon_state and pwm->last_thermal_state are read and modified concurrently without any locking, leading to inconsistent hardware state.
--

commit c24a43d962cbf165d7c767a5a51aa1a15586bd28
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>

hwmon:: Use non-OF thermal cooling device register function

Make HWMON drivers which are not based on device tree to use the
corresponding non-OF functions.

[ ... ]

> diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c
> index 137a90dd20752..860de6cfd8a4d 100644
> --- a/drivers/hwmon/mlxreg-fan.c
> +++ b/drivers/hwmon/mlxreg-fan.c
> @@ -583,8 +583,8 @@ static int mlxreg_fan_cooling_config(struct device *dev, struct mlxreg_fan *fan)
>  		pwm->fan = fan;
>  		/* Set minimal PWM speed. */
>  		pwm->last_hwmon_state = MLXREG_FAN_PWM_DUTY2STATE(MLXREG_FAN_MIN_DUTY);

This isn't a bug introduced by this commit, but are pwm->last_hwmon_state and
pwm->last_thermal_state read and modified concurrently without locking?

The function mlxreg_fan_write() (executed via sysfs) and
mlxreg_fan_set_cur_state() (executed by the thermal subsystem) concurrently
access and mutate these variables.

While the hwmon core serializes sysfs operations via
hwmon_device_register_with_info(), it does not serialize calls from the
thermal subsystem.

Because there is no lock protecting this multi-variable state, can a
Time-Of-Check to Time-Of-Use race occur? One thread may evaluate the maximum
of the two states using stale data and write an incorrectly low value to the
hardware.

Does this violate the hwmon subsystem guidelines requiring drivers to
implement locking for attributes registered by any other means?

> -		pwm->cdev = devm_thermal_of_cooling_device_register(dev, NULL, mlxreg_fan_name[i],
> -								    pwm, &mlxreg_fan_cooling_ops);
> +		pwm->cdev = devm_thermal_cooling_device_register(dev, mlxreg_fan_name[i],
> +								 pwm, &mlxreg_fan_cooling_ops);

This problem also wasn't introduced by this commit, but are the hwmon sysfs
interfaces exposed before the thermal cooling device pointer (pwm->cdev) is
initialized?

In mlxreg_fan_probe(), the hwmon device is registered via
devm_hwmon_device_register_with_info() before mlxreg_fan_cooling_config() is
called.  This exposes sysfs attributes to user-space immediately.

If a user writes to the PWM attribute in this narrow window, mlxreg_fan_write()
will be invoked and pass the currently uninitialized pwm->cdev to
_mlxreg_fan_set_cur_state().

Can this lead to a NULL pointer dereference when cdev->devdata is
unconditionally dereferenced?

>  		if (IS_ERR(pwm->cdev)) {
>  			dev_err(dev, "Failed to register cooling device\n");
>  			return PTR_ERR(pwm->cdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260419182203.4083985-1-daniel.lezcano@oss.qualcomm.com?part=7