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

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

[Gui-Dong Han]

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>
---
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);
 }
 
 /*
-- 
2.43.0

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

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] temp2_crit_hyst_store() relies on uninitialized cached data if invoked before lm63_update_device()
--

commit 0ac9742e5a731c1457b701e413708de120dd3347
Author: Gui-Dong Han <hanguidong02@gmail.com>

hwmon: (lm63) Add locking to avoid TOCTOU

This patch adds locking to several show functions to prevent TOCTOU races
when accessing shared cached data, ensuring values remain stable during
calculations.

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

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 035176a98ce9c..b8b1de5fa90f1 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c

[ ... ]

> @@ -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);
>  }

This isn't a bug introduced by this patch, but looking at the corresponding
store function, is there a pre-existing initialization issue?

static ssize_t temp2_crit_hyst_store(...)
{
        struct lm63_data *data = dev_get_drvdata(dev);
        ...
        mutex_lock(&data->update_lock);
        hyst = temp8_from_reg(data, 2) + data->temp2_offset - val;

Unlike the show functions, temp2_crit_hyst_store() retrieves the context
via dev_get_drvdata(dev) rather than calling lm63_update_device(dev).

If it is invoked before any show function (for example, by an init script
restoring settings on boot), will data->temp8[2] still be 0 due to the
initial devm_kzalloc()?

Could this cause the relative hysteresis to be computed against 0 instead
of the actual hardware default critical temperature, resulting in the
hardware's hysteresis register being misprogrammed?

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