thermal: underflow in trip_point_temp_store()
Subsystems (2)
PATCH 1/1 thermal: underflow in trip_point_temp_store()
This is to address a static checker warning about an underflow in imx_set_trip_temp(). The checker is complaining that we have a user supplied value for "temp" from kstrtoul() where we treat it as signed, we cap the upper but we accept negative values.
This looks unintentional since the caller is using unsigned longs to represent the temperature. Let's change it to int and reject negatives in the caller.
Also I changed it to reject negative "trip" values as well.
---
Someday we will use super cooled CPUs and we will need to rethink this code. :)
drivers/thermal/thermal_core.c +4 -2
@@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, {struct thermal_zone_device *tz =to_thermal_zone (dev);int trip, ret; -unsigned long temperature; +int temperature;if (!tz->ops->set_trip_temp)return -EPERM ;
@@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,if (!sscanf (attr->attr.name,"trip_point_%d_temp" , &trip))return -EINVAL ; -if (kstrtoul (buf,10 , &temperature)) +if (kstrtoint (buf,10 , &temperature)) +return -EINVAL ; +if (trip <0 || temperature <0 )return -EINVAL ; ret = tz->ops->set_trip_temp (tz, trip, temperature);
Hello Dan,
Quoted 9 lines · Dan Carpenter
This is to address a static checker warning about an underflow in
imx_set_trip_temp(). The checker is complaining that we have a user supplied value for "temp" fromkstrtoul()where we treat it as signed, we cap the upper but we accept negative values.This looks unintentional since the caller is using unsigned longs to represent the temperature. Let's change it to int and reject negatives in the caller.
Quoted 7 lines
Also I changed it to reject negative "trip" values as well.
Signed-off-by Dan Carpenter <[email protected]>---
Someday we will use super cooled CPUs and we will need to rethink this code. :)
I wish cpus would be the only type of devices covered here :-)
Quoted 14 lines
drivers/thermal/thermal_core.c +1
-1@@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, {struct thermal_zone_device *tz =to_thermal_zone (dev);int trip, ret; -unsigned long temperature; +int temperature;if (!tz->ops->set_trip_temp)return -EPERM ;
drivers/thermal/thermal_core.c trip_point_temp_store(struct device *dev, struct device_attribute *attr, · line 677
Quoted 7 lines
drivers/thermal/thermal_core.c +3
-1@@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,if (!sscanf (attr->attr.name,"trip_point_%d_temp" , &trip))return -EINVAL ; -if (kstrtoul (buf,10 , &temperature)) +if (kstrtoint (buf,10 , &temperature)) +return -EINVAL ; +if (trip <0 || temperature <0 )
While this maintains the original code semantics, I would prefer we could accept negative values here. Main reason is to maintain the range accepted by the current type in use for temperature. Second reason is that I heard reports of people willing to use this code to control thermal zones that would be at temperatures below 0.
Also, while here, could you please help cleaning emul_temp_store?
Thanks for sending this patch.
BR,
Eduardo
Quoted 3 lines
drivers/thermal/thermal_core.c +0
-0return -EINVAL ; ret = tz->ops->set_trip_temp (tz, trip, temperature);
Oh... Hm. I actually sent a totally different patch to fix this warning and you just merged it. I had forgotten about that. As far as I am concerned this issue is solved. :) Sorry for sending duplicates.
regards,
dan carpenter
Quoted 3 lines · Dan Carpenter
Oh... Hm. I actually sent a totally different patch to fix this warning and you just merged it. I had forgotten about that. As far as I am concerned this issue is solved. :) Sorry for sending duplicates.
Ok. Thanks :-). I guess this one took more attention for being touching the thermal core. On IMX driver, it might still make sense to remove the negative values as we are dealing with CPU temperature over there.
But it is good as it brings to the radar the point of reviewing again the possibility of supporting negative temperatures.
BR
Eduardo
Quoted 4 lines
regards,
dan carpenter
Am 03.11.2015 23:14, schrieb Dan Carpenter:
Quoted 30 lines
This is to address a static checker warning about an underflow in
imx_set_trip_temp(). The checker is complaining that we have a user supplied value for "temp" fromkstrtoul()where we treat it as signed, we cap the upper but we accept negative values.This looks unintentional since the caller is using unsigned longs to represent the temperature. Let's change it to int and reject negatives in the caller.
Also I changed it to reject negative "trip" values as well.
Signed-off-by Dan Carpenter <[email protected]>---
Someday we will use super cooled CPUs and we will need to rethink this code. :)drivers/thermal/thermal_core.c +1
-1@@ -664,7 +664,7 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, {struct thermal_zone_device *tz =to_thermal_zone (dev);int trip, ret; -unsigned long temperature; +int temperature;if (!tz->ops->set_trip_temp)return -EPERM ;
drivers/thermal/thermal_core.c trip_point_temp_store(struct device *dev, struct device_attribute *attr, · line 680
Quoted 10 lines
drivers/thermal/thermal_core.c +3
-1@@ -672,7 +672,9 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,if (!sscanf (attr->attr.name,"trip_point_%d_temp" , &trip))return -EINVAL ; -if (kstrtoul (buf,10 , &temperature)) +if (kstrtoint (buf,10 , &temperature)) +return -EINVAL ; +if (trip <0 || temperature <0 )return -EINVAL ; ret = tz->ops->set_trip_temp (tz, trip, temperature);
IMHO the test should be near the point where the value is generated.
if (!sscanf(attr->attr.name, "trip_point_%d_temp", &trip))
return -EINVAL;
if (trip < 0)
return -EINVAL;
if (kstrtoint(buf, 10, &temperature))
return -EINVAL;
if (temperature < 0)
return -EINVAL;
That way it is easily visible under what condition -EINVAL is generated (to many).
hope that helps,
re,
wh