Re: [PATCH 2/3] hwmon: (adm1031) Hold lock while reading cached data

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Store functions like fan_auto_channel_store() read uninitialized cached values, which can silently disable hardware monitoring.
- [High] auto_temp_max_store() corrupts the temp_max cache and causes register desynchronization.
--

commit 8f551aed0a746551e7fda3272d5c7e3fcaa2b090
Author: Gui-Dong Han <hanguidong02@gmail.com>

hwmon: (adm1031) Hold lock while reading cached data

The patch addresses TOCTOU races by holding data->update_lock while
accessing cached values in sysfs show and store functions.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/

> diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
> index 0551f815233d9..887fba9ea149f 100644
> --- a/drivers/hwmon/adm1031.c
> +++ b/drivers/hwmon/adm1031.c
> @@ -350,9 +350,8 @@ fan_auto_channel_store(struct device *dev, struct device_attribute *attr,
>  	if (ret)
>  		return ret;
>  
> -	old_fan_mode = data->conf1;
> -
>  	mutex_lock(&data->update_lock);
> +	old_fan_mode = data->conf1;

This is not a regression from this patch, but does this store function read
uninitialized cached values if it is called before adm1031_update_device()
has populated them?

The driver allocates its data structure with devm_kzalloc() during probe,
initializing the cache variables (like data->conf1) to zero, but
adm1031_update_device() is not called during probe to populate them.

If a user writes to this sysfs attribute before any reads, the store
function performs a read-modify-write using the zeroed data->conf1. When it
writes data->conf1 back to the ADM1031_REG_CONF1 register, it clears the
ADM1031_CONF1_MONITOR_ENABLE bit (0x01).

Could this silently disable the chip's hardware monitoring and thermal
protection?

[ ... ]

Additionally, there appears to be a separate pre-existing regression in
auto_temp_max_store():

drivers/hwmon/adm1031.c:auto_temp_max_store() {
    ...
	val = clamp_val(val, 0, 127000);
	mutex_lock(&data->update_lock);
	data->temp_max[nr] = AUTO_TEMP_MAX_TO_REG(val, data->auto_temp[nr],
						  data->pwm[nr]);
	adm1031_write_value(client, ADM1031_REG_AUTO_TEMP(nr),
			    data->temp_max[nr]);
	mutex_unlock(&data->update_lock);
    ...
}

Does this corrupt the temp_max cache and cause register desynchronization?

The calculated new automatic temperature limit is assigned to
data->temp_max[nr] instead of data->auto_temp[nr], corrupting the cache
used by temp_max_show().

Because data->auto_temp[nr] is left stale, if auto_temp_min_store() is
called before the cache expires, it will read the old data->auto_temp[nr]
value and write it back to ADM1031_REG_AUTO_TEMP.

Could this cause the hardware to silently revert the max temperature limit
just set by the user?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416091754.310-1-hanguidong02@gmail.com?part=2