Re: [hwmon PATCH v2 7/8] hwmon: (adt7470) Fix divide-by-zero TOCTOU crash in fan speed read

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

Pre-existing issues:
- [High] Pre-existing TOCTOU vulnerabilities in multiple hwmon drivers due to unsafe macro evaluation of unlocked array values, leading to divide-by-zero crashes.
- [High] The driver violates hwmon subsystem guidelines by failing to use hwmon_lock() for extra sysfs attributes and the background kthread, leaving lockless accesses exposed to data races.
- [High] Transient zeroing of `data->alarm` state causes missing alarms due to lockless sysfs reads.
--

--- Patch [7]: [hwmon PATCH v2 7/8] hwmon: (adt7470) Fix divide-by-zero TOCTOU crash in fan speed read ---
commit c42c0e5abb409dc36e59b29c77b757552ecfb7d9
Author: Luiz Angelo Daros de Luca <[email protected]>

hwmon: (adt7470) Fix divide-by-zero TOCTOU crash in fan speed read
    
This commit fixes a time-of-check to time-of-use (TOCTOU) bug in the adt7470
driver by reading the fan data only once. Previously, a concurrent update
could zero out the fan data between the validation check and the RPM
conversion, causing a divide-by-zero.

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 1fbca4869b7b6..772d2a409bb5c 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -660,36 +660,33 @@ static ssize_t alarm_mask_store(struct device *dev,
>  static int adt7470_fan_read(struct device *dev, u32 attr, int channel, long *val)
>  {
>  	struct adt7470_data *data = adt7470_update_device(dev);

[Severity: High]
This is a pre-existing issue, but does this driver fail to serialize sysfs
attributes and the background kthread properly?

The hwmon subsystem guidelines mandate that drivers use hwmon_lock() and
hwmon_unlock() to serialize attributes registered by any other means than
the core info template.

This driver registers several extra attributes, such as alarm_mask_show(),
via the extra_groups parameter in adt7470_probe() and runs a background
auto_update kthread. None of these paths acquire hwmon_lock(), and instead
invoke adt7470_update_device(), which uses a private internal data->lock.

Meanwhile, standard hwmon core operations read from the cache arrays
locklessly, assuming the core holds hwmon_lock(). Can this architectural
locking mismatch create unprotected data races against concurrent updates?

> +	u16 fan_data;
>  
>  	if (IS_ERR(data))
>  		return PTR_ERR(data);
>  
>  	switch (attr) {

[ ... ]

>  	case hwmon_fan_alarm:
>  		*val = !!(data->alarm & FAN_ALARM_BIT(channel));

[Severity: High]
This isn't a bug introduced by this patch, but could this lockless read of
data->alarm evaluate to 0 incorrectly and miss an active alarm?

In adt7470_update_sensors(), data->alarm is first assigned the low byte:

	err = regmap_read(data->regmap, ADT7470_REG_ALARM1, &val);
	...
	data->alarm = val;
	if (data->alarm & ADT7470_OOL_ALARM) {
		...
		data->alarm |= ALARM2(val);
	}

This temporarily zeroes out the upper 8 bits. Because adt7470_fan_read()
accesses data->alarm locklessly after adt7470_update_device() releases
data->lock, could a concurrent sysfs read happen exactly during this transient
state and completely miss an active fan or high-channel temperature alarm?

> -		break;
> +		return 0;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (FAN_DATA_VALID(fan_data))
> +		*val = FAN_PERIOD_TO_RPM(fan_data);
> +	else
> +		*val = 0;
> +

[Severity: High]
This isn't a bug introduced by this patch, but do other hwmon drivers suffer
from the same TOCTOU bug that this patch fixes for adt7470?

In drivers/hwmon/adt7462.c:fan_show(), the shared array element is passed
directly to the FAN_PERIOD_TO_RPM macro without holding a lock:

	if (!fan_enabled(data, attr->index) ||
	    !FAN_DATA_VALID(data->fan[attr->index]))
		return sprintf(buf, "0\n");

	return sprintf(buf, "%d\n",
		       FAN_PERIOD_TO_RPM(data->fan[attr->index]));

Because the macro expands the argument multiple times, the shared state is read
once for the zero-check and again for the division. Could a concurrent update
modify the array element to zero immediately after the check, resulting in a
divide-by-zero?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7