[PATCH 1/3] hwmon: (adm1031) Convert macros to functions to avoid TOCTOU

[PATCH 1/3] hwmon: (adm1031) Convert macros to functions to avoid TOCTOU

[Gui-Dong Han]

The macros TEMP_OFFSET_FROM_REG, FAN_FROM_REG, and
AUTO_TEMP_MAX_FROM_REG evaluate their arguments multiple times. When
used in lockless code accessing shared driver data, this can cause
Time-of-Check to Time-of-Use (TOCTOU) races. In the case of
FAN_FROM_REG, it can also result in a divide-by-zero error.

Convert those macros to static functions so that their arguments are
always evaluated only once.

Check the remaining 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")
Fixes: 49dc9efed05a ("hwmon: (adm1031) Add sysfs files for temperature offsets")
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/adm1031.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 343118532cdb..0551f815233d 100644
--- a/drivers/hwmon/adm1031.c
+++ b/drivers/hwmon/adm1031.c
@@ -203,16 +203,24 @@ static struct adm1031_data *adm1031_update_device(struct device *dev)
 #define TEMP_FROM_REG_EXT(val, ext)	(TEMP_FROM_REG(val) + (ext) * 125)
 
 #define TEMP_OFFSET_TO_REG(val)		(TEMP_TO_REG(val) & 0x8f)
-#define TEMP_OFFSET_FROM_REG(val)	TEMP_FROM_REG((val) < 0 ? \
-						      (val) | 0x70 : (val))
 
-#define FAN_FROM_REG(reg, div)		((reg) ? \
-					 (11250 * 60) / ((reg) * (div)) : 0)
+static int temp_offset_from_reg(int val)
+{
+	return TEMP_FROM_REG(val < 0 ? val | 0x70 : val);
+}
+
+static int fan_from_reg(int reg, int div)
+{
+	if (!reg)
+		return 0;
+
+	return (11250 * 60) / (reg * div);
+}
 
 static int FAN_TO_REG(int reg, int div)
 {
 	int tmp;
-	tmp = FAN_FROM_REG(clamp_val(reg, 0, 65535), div);
+	tmp = fan_from_reg(clamp_val(reg, 0, 65535), div);
 	return tmp > 255 ? 255 : tmp;
 }
 
@@ -235,9 +243,10 @@ static int FAN_TO_REG(int reg, int div)
 #define AUTO_TEMP_OFF_FROM_REG(reg)		\
 	(AUTO_TEMP_MIN_FROM_REG(reg) - 5000)
 
-#define AUTO_TEMP_MAX_FROM_REG(reg)		\
-	(AUTO_TEMP_RANGE_FROM_REG(reg) +	\
-	AUTO_TEMP_MIN_FROM_REG(reg))
+static int auto_temp_max_from_reg(int reg)
+{
+	return AUTO_TEMP_RANGE_FROM_REG(reg) + AUTO_TEMP_MIN_FROM_REG(reg);
+}
 
 static int AUTO_TEMP_MAX_TO_REG(int val, int reg, int pwm)
 {
@@ -426,7 +435,7 @@ static ssize_t auto_temp_max_show(struct device *dev,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       AUTO_TEMP_MAX_FROM_REG(data->auto_temp[nr]));
+		       auto_temp_max_from_reg(data->auto_temp[nr]));
 }
 static ssize_t
 auto_temp_max_store(struct device *dev, struct device_attribute *attr,
@@ -559,7 +568,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *attr,
 	struct adm1031_data *data = adm1031_update_device(dev);
 	int value;
 
-	value = trust_fan_readings(data, nr) ? FAN_FROM_REG(data->fan[nr],
+	value = trust_fan_readings(data, nr) ? fan_from_reg(data->fan[nr],
 				 FAN_DIV_FROM_REG(data->fan_div[nr])) : 0;
 	return sprintf(buf, "%d\n", value);
 }
@@ -577,7 +586,7 @@ static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       FAN_FROM_REG(data->fan_min[nr],
+		       fan_from_reg(data->fan_min[nr],
 				    FAN_DIV_FROM_REG(data->fan_div[nr])));
 }
 static ssize_t fan_min_store(struct device *dev,
@@ -679,7 +688,7 @@ static ssize_t temp_offset_show(struct device *dev,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       TEMP_OFFSET_FROM_REG(data->temp_offset[nr]));
+		       temp_offset_from_reg(data->temp_offset[nr]));
 }
 static ssize_t temp_min_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
-- 
2.43.0

[PATCH 2/3] hwmon: (adm1031) Hold lock while reading cached data

[Gui-Dong Han]

The functions fan_show(), fan_min_show(), and temp_show() read shared
cached values multiple times without holding data->update_lock.
fan_auto_channel_store() also reads data->conf1 before taking the lock.
Those cached values can change in adm1031_update_device(), resulting in
inconsistent snapshots and TOCTOU races.

Hold data->update_lock across those reads so that the cached values stay
stable while the results are calculated.

Check the remaining 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")
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/adm1031.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 0551f815233d..887fba9ea149 100644
--- a/drivers/hwmon/adm1031.c
+++ b/drivers/hwmon/adm1031.c
@@ -350,9 +350,8 @@ fan_auto_channel_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	old_fan_mode = data->conf1;
-
 	mutex_lock(&data->update_lock);
+	old_fan_mode = data->conf1;
 
 	ret = get_fan_auto_nearest(data, nr, val, data->conf1);
 	if (ret < 0) {
@@ -568,8 +567,10 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *attr,
 	struct adm1031_data *data = adm1031_update_device(dev);
 	int value;
 
+	mutex_lock(&data->update_lock);
 	value = trust_fan_readings(data, nr) ? fan_from_reg(data->fan[nr],
 				 FAN_DIV_FROM_REG(data->fan_div[nr])) : 0;
+	mutex_unlock(&data->update_lock);
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -585,9 +586,13 @@ static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
 {
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
-	return sprintf(buf, "%d\n",
-		       fan_from_reg(data->fan_min[nr],
-				    FAN_DIV_FROM_REG(data->fan_div[nr])));
+	int value;
+
+	mutex_lock(&data->update_lock);
+	value = fan_from_reg(data->fan_min[nr],
+			     FAN_DIV_FROM_REG(data->fan_div[nr]));
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", value);
 }
 static ssize_t fan_min_store(struct device *dev,
 			     struct device_attribute *attr, const char *buf,
@@ -677,10 +682,15 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	int ext;
+	int temp;
+
+	mutex_lock(&data->update_lock);
 	ext = nr == 0 ?
 	    ((data->ext_temp[nr] >> 6) & 0x3) * 2 :
 	    (((data->ext_temp[nr] >> ((nr - 1) * 3)) & 7));
-	return sprintf(buf, "%d\n", TEMP_FROM_REG_EXT(data->temp[nr], ext));
+	temp = TEMP_FROM_REG_EXT(data->temp[nr], ext);
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", temp);
 }
 static ssize_t temp_offset_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
-- 
2.43.0

[PATCH 3/3] hwmon: (adm1031) Serialize update rate changes

[Gui-Dong Han]

update_interval_store() updates the hardware rate bits in
ADM1031_REG_FAN_FILTER and then updates data->update_interval. Those
steps are currently split across unlocked and locked sections. This also
leaves the register read-modify-write sequence unprotected.

Hold data->update_lock across the whole sequence so that the register
update and the cached software state remain synchronized.

Fixes: 87c33daadbfe ("hwmon: (adm1031) Allow setting update rate")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
 drivers/hwmon/adm1031.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 887fba9ea149..a46db83471f2 100644
--- a/drivers/hwmon/adm1031.c
+++ b/drivers/hwmon/adm1031.c
@@ -897,13 +897,14 @@ static ssize_t update_interval_store(struct device *dev,
 	}
 	/* if not found, we point to the last entry (lowest update interval) */
 
+	mutex_lock(&data->update_lock);
+
 	/* set the new update rate while preserving other settings */
 	reg = adm1031_read_value(client, ADM1031_REG_FAN_FILTER);
 	reg &= ~ADM1031_UPDATE_RATE_MASK;
 	reg |= i << ADM1031_UPDATE_RATE_SHIFT;
 	adm1031_write_value(client, ADM1031_REG_FAN_FILTER, reg);
 
-	mutex_lock(&data->update_lock);
 	data->update_interval = update_intervals[i];
 	mutex_unlock(&data->update_lock);
 
-- 
2.43.0