Re: [PATCH v2] hwmon: (lm63) Add locking to avoid TOCTOU

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Apr 16, 2026 at 09:57:03PM +0800, Gui-Dong Han wrote:
> The functions show_fan(), show_pwm1(), show_temp11(),
> temp2_crit_hyst_show(), and show_lut_temp_hyst() access shared cached
> data without holding the update lock. This can cause TOCTOU races if
> the cached values change between the checks and the later calculations.
> 
> Those cached values are updated in lm63_update_device(). In the general
> case, the affected functions combine multiple cached values without
> locking and can therefore observe a mixed old/new snapshot. In
> addition, show_fan() reads data->fan[nr] locklessly while
> lm63_update_device() updates data->fan[0] in two steps, which can
> expose an intermediate torn value and potentially trigger a
> divide-by-zero error. This means that converting the macro to a
> function is not sufficient to fix show_fan().
> 
> Hold the update lock across the whole read and calculation sequence so
> that the values remain stable.
> 
> Check the other functions in the driver as well. Keep them unchanged
> because they either do not access shared cached values multiple times
> or already do so under lock.
> 
> Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Fixes: e872c91e726e ("hwmon: (lm63) Add support for unsigned upper temperature limits")
> Fixes: d216f6809eb6 ("hwmon: (lm63) Expose automatic fan speed control lookup table")
> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>

Applied.

Thanks,
Guenter

> ---
> v2:
> - Sashiko-bot pointed out that show_fan(), temp2_crit_hyst_show(), and
>   show_lut_temp_hyst() also need locking.
> 
> While learning the hwmon driver code, I found a few more potential
> TOCTOU problems in drivers still using the older non-_with_info() APIs.
> Fix them.
> ---
>  drivers/hwmon/lm63.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 035176a98ce9..b8b1de5fa90f 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -333,7 +333,13 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct lm63_data *data = lm63_update_device(dev);
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index]));
> +	int fan;
> +
> +	mutex_lock(&data->update_lock);
> +	fan = FAN_FROM_REG(data->fan[attr->index]);
> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%d\n", fan);
>  }
>  
>  static ssize_t set_fan(struct device *dev, struct device_attribute *dummy,
> @@ -366,12 +372,14 @@ static ssize_t show_pwm1(struct device *dev, struct device_attribute *devattr,
>  	int nr = attr->index;
>  	int pwm;
>  
> +	mutex_lock(&data->update_lock);
>  	if (data->pwm_highres)
>  		pwm = data->pwm1[nr];
>  	else
>  		pwm = data->pwm1[nr] >= 2 * data->pwm1_freq ?
>  		       255 : (data->pwm1[nr] * 255 + data->pwm1_freq) /
>  		       (2 * data->pwm1_freq);
> +	mutex_unlock(&data->update_lock);
>  
>  	return sprintf(buf, "%d\n", pwm);
>  }
> @@ -529,6 +537,7 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>  	int nr = attr->index;
>  	int temp;
>  
> +	mutex_lock(&data->update_lock);
>  	if (!nr) {
>  		/*
>  		 * Use unsigned temperature unless its value is zero.
> @@ -544,7 +553,10 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>  		else
>  			temp = TEMP11_FROM_REG(data->temp11[nr]);
>  	}
> -	return sprintf(buf, "%d\n", temp + data->temp2_offset);
> +	temp += data->temp2_offset;
> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%d\n", temp);
>  }
>  
>  static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> @@ -592,9 +604,14 @@ static ssize_t temp2_crit_hyst_show(struct device *dev,
>  				    struct device_attribute *dummy, char *buf)
>  {
>  	struct lm63_data *data = lm63_update_device(dev);
> -	return sprintf(buf, "%d\n", temp8_from_reg(data, 2)
> -		       + data->temp2_offset
> -		       - TEMP8_FROM_REG(data->temp2_crit_hyst));
> +	int temp;
> +
> +	mutex_lock(&data->update_lock);
> +	temp = temp8_from_reg(data, 2) + data->temp2_offset
> +	     - TEMP8_FROM_REG(data->temp2_crit_hyst);
> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%d\n", temp);
>  }
>  
>  static ssize_t show_lut_temp_hyst(struct device *dev,
> @@ -602,10 +619,14 @@ static ssize_t show_lut_temp_hyst(struct device *dev,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct lm63_data *data = lm63_update_device(dev);
> +	int temp;
>  
> -	return sprintf(buf, "%d\n", lut_temp_from_reg(data, attr->index)
> -		       + data->temp2_offset
> -		       - TEMP8_FROM_REG(data->lut_temp_hyst));
> +	mutex_lock(&data->update_lock);
> +	temp = lut_temp_from_reg(data, attr->index) + data->temp2_offset
> +	     - TEMP8_FROM_REG(data->lut_temp_hyst);
> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%d\n", temp);
>  }
>  
>  /*