Re: [PATCH] hwmon: emc2305: Add missing device tree compatibles

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver fails to use `hwmon_lock()` to protect shared state and hardware registers during thermal cooling device operations, violating hwmon subsystem concurrency rules.
- [High] A division by zero vulnerability exists when reading the fan tachometer input if the hardware returns a small non-zero value between 1 and 7.
- [High] When thermal management is enabled, user inputs for PWM sysfs attributes bypass bounds checking, leading to massive integer underflow and unpredictable PWM duty cycles.
- [High] Device tree parsing hardcodes `pwm_separate = false` when multiple PWM children exist, breaking independent cooling zones.
--

--- Patch [1]: [PATCH] hwmon: emc2305: Add missing device tree compatibles ---
commit f16b37d8579458e0cc2f0b0ac6e79ec7dd8c69ac
Author: Peter Robinson <pbrobinson@gmail.com>

hwmon: emc2305: Add missing device tree compatibles

Add the missing device tree compatibles for all the variants
so they can be specified. There's at least the use of emc2301
already.

> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index eef3b021671b0..38034d7a30901 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c
> @@ -762,6 +762,9 @@ static void emc2305_shutdown(struct i2c_client *client)
>  }
>  
>  static const struct of_device_id of_emc2305_match_table[] = {
> +	{ .compatible = "microchip,emc2301", },
> +	{ .compatible = "microchip,emc2302", },
> +	{ .compatible = "microchip,emc2303", },
>  	{ .compatible = "microchip,emc2305", },
>  	{},
>  };

[Severity: High]
This isn't a bug introduced by this patch, but could the lack of locking here
cause race conditions?

The hwmon subsystem core serializes sysfs operations using its internal lock,
which protects emc2305_write(). However, the thermal subsystem callback can
execute concurrently:

drivers/hwmon/emc2305.c:emc2305_set_cur_state() {
	/* Save thermal state. */
	data->cdev_data[cdev_idx].last_thermal_state = state;
	ret = __emc2305_set_cur_state(data, cdev_idx, state);
	if (ret < 0)
		return ret;
}

Should emc2305_set_cur_state() use hwmon_lock() and hwmon_unlock() to prevent
corrupting logical fan states and interleaving I2C transactions with
emc2305_write()?

[Severity: High]
This is a pre-existing issue, but is it possible for this code to trigger a
division by zero?

If the hardware returns a small non-zero value between 1 and 7, the shift
operation evaluates to 0:

drivers/hwmon/emc2305.c:emc2305_show_fan() {
...
	ret = i2c_smbus_read_word_swapped(client, EMC2305_REG_FAN_TACH(channel));
	if (ret <= 0)
		return ret;

	ret = ret >> EMC2305_TACH_REGS_UNUSE_BITS;
	ret = EMC2305_RPM_FACTOR / ret;
...
}

Might we need an additional check to ensure ret is not zero after the shift
before performing the division?

[Severity: High]
This isn't a bug introduced by this patch, but does emc2305_write() lack
bounds checking for val when thermal management is active?

When a user writes to the PWM sysfs attribute, the input val is used directly
in EMC2305_PWM_DUTY2STATE:

drivers/hwmon/emc2305.c:emc2305_write() {
...
			if (IS_REACHABLE(CONFIG_THERMAL)) {
...
				data->cdev_data[cdev_idx].last_hwmon_state =
					EMC2305_PWM_DUTY2STATE(val, data->max_state,
							       EMC2305_FAN_MAX);
...
}

If val is negative, could the macro EMC2305_PWM_DUTY2STATE evaluate to a
negative integer that underflows the unsigned long field last_hwmon_state?

This appears to bypass the validation naturally present in emc2305_set_pwm().

[Severity: High]
This is a pre-existing issue, but does device tree parsing correctly support
independent cooling zones?

When pwm_childs is greater than 0, the driver hardcodes pwm_separate to false:

drivers/hwmon/emc2305.c:emc2305_probe() {
...
	pwm_childs = emc2305_probe_childs_from_dt(dev);
...
	if (!pwm_childs) {
...
	} else {
		data->max_state = EMC2305_FAN_MAX_STATE;
		data->pwm_separate = false;
...
	}
...
}

Because pwm_separate is forced to false, it looks like emc2305_get_cdev_idx()
bypasses channel indexing:

drivers/hwmon/emc2305.c:emc2305_get_cdev_idx() {
...
	if (!data->pwm_separate)
		return 0;
...
}

Will independent cooling devices mapped to separate fans incorrectly write
their thermal states to channel 0 and broadcast their PWM values to all fans?

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