[PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[Gui-Dong Han]

The macro FAN_FROM_REG evaluates its argument multiple times. When used
in lockless code accessing shared driver data, this can cause a
Time-of-Check to Time-of-Use (TOCTOU) race and potentially a
divide-by-zero error.

Convert the macro to a static function so that the register value is
evaluated only once.

Check the other conversion macros in the driver as well. Keep them
unchanged because they either do not evaluate arguments multiple times
or are only used from locked code paths.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
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 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 035176a98ce9..da132b267c58 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -109,8 +109,14 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
  * adapted accordingly.
  */
 
-#define FAN_FROM_REG(reg)	((reg) == 0xFFFC || (reg) == 0 ? 0 : \
-				 5400000 / (reg))
+static int fan_from_reg(int reg)
+{
+	if (reg == 0xFFFC || reg == 0)
+		return 0;
+
+	return 5400000 / reg;
+}
+
 #define FAN_TO_REG(val)		((val) <= 82 ? 0xFFFC : \
 				 (5400000 / (val)) & 0xFFFC)
 #define TEMP8_FROM_REG(reg)	((reg) * 1000)
@@ -333,7 +339,7 @@ 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]));
+	return sprintf(buf, "%d\n", fan_from_reg(data->fan[attr->index]));
 }
 
 static ssize_t set_fan(struct device *dev, struct device_attribute *dummy,
-- 
2.43.0

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

[Gui-Dong Han]

The functions show_pwm1() and show_temp11() access shared cached data
multiple times 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(). For example,
show_pwm1() may read data->pwm1[nr] and data->pwm1_freq while a
concurrent lm63_update_device() refresh is updating them, resulting in a
mixed old/new snapshot.

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")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index da132b267c58..25b871695cb3 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -372,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);
 }
@@ -535,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.
@@ -550,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,
-- 
2.43.0