Skip to contentSkip to conversation

hwmon: (lm77) Prevent overflow problem when writing large limits

PATCH Superseded Axel Lin 2 participants 4 messages Jul 31, 2014 Hardware monitoring

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH v2 1/2 hwmon: (lm77) Prevent overflow problem when writing large limits

On platforms with sizeof(int) < sizeof(long), writing a temperature limit larger than MAXINT will result in unpredictable limit values written to the chip.
Clamp the input values to the supported limits first to fix the problem.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/hwmon/lm77.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/hwmon/lm77.c +3 -3
index 5ceb443..72a02b0 100644
@@ -80,8 +80,7 @@ struct lm77_data {
  */
 static inline s16 LM77_TEMP_TO_REG(int temp)
 {
-	int ntemp = clamp_val(temp, LM77_TEMP_MIN, LM77_TEMP_MAX);
-	return (ntemp / 500) * 8;
+	return (temp / 500) * 8;
 }
 
 static inline int LM77_TEMP_FROM_REG(s16 reg)

@@ -176,7 +175,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
 		return err;
 
 	mutex_lock(&data->update_lock);
-	data->temp[nr] = val;
+	data->temp[nr] = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);
 	lm77_write_value(client, temp_regs[nr], LM77_TEMP_TO_REG(val));
 	mutex_unlock(&data->update_lock);
 	return count;

@@ -199,6 +198,7 @@ static ssize_t set_temp_hyst(struct device *dev,
 	if (err)
 		return err;
 
+	val = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);
 	mutex_lock(&data->update_lock);
 	data->temp[t_hyst] = data->temp[t_crit] - val;
 	lm77_write_value(client, LM77_REG_TEMP_HYST,
Signature · 9 lines
1.9.1




_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
maintainer
#1 lore raw
Quoted 34 lines · Axel Lin

On platforms with sizeof(int) < sizeof(long), writing a temperature limit larger than MAXINT will result in unpredictable limit values written to the chip.
Clamp the input values to the supported limits first to fix the problem.

Signed-off-by Axel Lin <[email protected]>
---
  drivers/hwmon/lm77.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
drivers/hwmon/lm77.c +2 -3
index 5ceb443..72a02b0 100644
@@ -80,8 +80,7 @@ struct lm77_data {
   */
  static inline s16 LM77_TEMP_TO_REG(int temp)
  {
-	int ntemp = clamp_val(temp, LM77_TEMP_MIN, LM77_TEMP_MAX);
-	return (ntemp / 500) * 8;
+	return (temp / 500) * 8;
  }
 
  static inline int LM77_TEMP_FROM_REG(s16 reg)

@@ -176,7 +175,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
  		return err;
 
  	mutex_lock(&data->update_lock);
-	data->temp[nr] = val;
+	data->temp[nr] = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);
  	lm77_write_value(client, temp_regs[nr], LM77_TEMP_TO_REG(val));
  	mutex_unlock(&data->update_lock);
  	return count;

drivers/hwmon/lm77.c static ssize_t set_temp_hyst(struct device *dev, · line 201

Quoted 4 lines
drivers/hwmon/lm77.c +1 -0
@@ -199,6 +198,7 @@ static ssize_t set_temp_hyst(struct device *dev,
  	if (err)
  		return err;
 
+	val = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);

Unfortunately that doesn't work, for a number of reasons.
First, the temperature is read as unsigned and stored in an unsigned long. This is wrong; nothing in the datasheet suggests that the value (the absolute temperature) must be positive.

Second, the value written into the register is actually based on ...

drivers/hwmon/lm77.c static ssize_t set_temp_hyst(struct device *dev, · line 203

Quoted 2 lines
drivers/hwmon/lm77.c +0 -0
  	mutex_lock(&data->update_lock);
  	data->temp[t_hyst] = data->temp[t_crit] - val;

"data->temp[t_crit] - val", so applying the clamp on val won't help since the final value can still be out of range. Changing the code to use long / kstrtol and to

	val = clamp_val(data->temp[t_crit] - val, LM77_TEMP_MIN, LM77_TEMP_MAX);
         data->temp[t_hyst] = val;

solved the problem and made my test happy.

Unfortunately, there is yet another problem: The cached value after a write and the temperature read back from the chip after the cache expires do not match. Specifically, writing a large value as limit returns 125000 initially, but 1696000 (!) after the cache expires. Right now I have no idea how that happens. Unfortunately I won't have time to track it down before early next week.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

maintainer
#2 lore raw
In reply to Axel Lin
Quoted 25 lines · Axel Lin

On platforms with sizeof(int) < sizeof(long), writing a temperature limit larger than MAXINT will result in unpredictable limit values written to the chip.
Clamp the input values to the supported limits first to fix the problem.

Signed-off-by Axel Lin <[email protected]>
---
  drivers/hwmon/lm77.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
drivers/hwmon/lm77.c +1 -2
index 5ceb443..72a02b0 100644
@@ -80,8 +80,7 @@ struct lm77_data {
   */
  static inline s16 LM77_TEMP_TO_REG(int temp)
  {
-	int ntemp = clamp_val(temp, LM77_TEMP_MIN, LM77_TEMP_MAX);
-	return (ntemp / 500) * 8;
+	return (temp / 500) * 8;
  }
 
  static inline int LM77_TEMP_FROM_REG(s16 reg)

drivers/hwmon/lm77.c static ssize_t set_temp(struct device *dev, struct device_attribute *devattr, · line 179

Quoted 6 lines
drivers/hwmon/lm77.c +1 -1
@@ -176,7 +175,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
  		return err;
 
  	mutex_lock(&data->update_lock);
-	data->temp[nr] = val;
+	data->temp[nr] = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);
  	lm77_write_value(client, temp_regs[nr], LM77_TEMP_TO_REG(val));

Found it: Clamp applied to data->temp[nr], but you are still using val here. Has to be LM77_TEMP_TO_REG(data->temp[nr]).

Guenter

drivers/hwmon/lm77.c static ssize_t set_temp_hyst(struct device *dev, · line 204

Quoted 11 lines
drivers/hwmon/lm77.c +1 -0
  	mutex_unlock(&data->update_lock);
  	return count;

@@ -199,6 +198,7 @@ static ssize_t set_temp_hyst(struct device *dev,
  	if (err)
  		return err;
 
+	val = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);
  	mutex_lock(&data->update_lock);
  	data->temp[t_hyst] = data->temp[t_crit] - val;
  	lm77_write_value(client, LM77_REG_TEMP_HYST,

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#3 lore raw
In reply to Axel Lin

2014-07-31 22:46 GMT+08:00 Guenter Roeck <[email protected]>:

Quoted 30 lines · Axel Lin
Quoted 29 lines

On platforms with sizeof(int) < sizeof(long), writing a temperature limit larger than MAXINT will result in unpredictable limit values written to the chip.
Clamp the input values to the supported limits first to fix the problem.

Signed-off-by Axel Lin <[email protected]>
---
  drivers/hwmon/lm77.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
drivers/hwmon/lm77.c +1 -2
index 5ceb443..72a02b0 100644
@@ -80,8 +80,7 @@ struct lm77_data {
   */
  static inline s16 LM77_TEMP_TO_REG(int temp)
  {
-       int ntemp = clamp_val(temp, LM77_TEMP_MIN, LM77_TEMP_MAX);
-       return (ntemp / 500) * 8;
+       return (temp / 500) * 8;
  }
 
  static inline int LM77_TEMP_FROM_REG(s16 reg)

@@ -176,7 +175,7 @@ static ssize_t set_temp(struct device *dev, struct
 device_attribute *devattr,
                return err;
Quoted 8 lines

drivers/hwmon/lm77.c static ssize_t set_temp(struct device *dev, struct device_attribute *devattr, · line 179

Quoted 4 lines
drivers/hwmon/lm77.c +1 -1
        mutex_lock(&data->update_lock);
-       data->temp[nr] = val;
+       data->temp[nr] = clamp_val(val, LM77_TEMP_MIN, LM77_TEMP_MAX);
        lm77_write_value(client, temp_regs[nr], LM77_TEMP_TO_REG(val));

Found it: Clamp applied to data->temp[nr], but you are still using val here. Has to be LM77_TEMP_TO_REG(data->temp[nr]).

Thanks a lot.
I'll send a v3 soon.

Regards,
Axel

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors