diff mbox

thermal: underflow in trip_point_temp_store()

Message ID 20151103221434.GB19280@mwanda (mailing list archive)
State Changes Requested
Delegated to: Eduardo Valentin
Headers show

Commit Message

Dan Carpenter Nov. 3, 2015, 10:14 p.m. UTC 
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.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Someday we will use super cooled CPUs and we will need to rethink this
code.  :)

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eduardo Valentin Nov. 4, 2015, 5:57 a.m. UTC | #1 
Hello Dan,

On Wed, Nov 04, 2015 at 01:14:34AM +0300, Dan Carpenter wrote:
> 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.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> 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 :-)


> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525c..151a630 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -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)

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

>  		return -EINVAL;
>  
>  	ret = tz->ops->set_trip_temp(tz, trip, temperature);
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter Nov. 4, 2015, 11:32 a.m. UTC | #2 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Walter Harms Nov. 4, 2015, 12:26 p.m. UTC | #3 
Am 03.11.2015 23:14, schrieb 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" 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.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Someday we will use super cooled CPUs and we will need to rethink this
> code.  :)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index d9e525c..151a630 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -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);


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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin Nov. 4, 2015, 4:32 p.m. UTC | #4 
On Wed, Nov 04, 2015 at 02:32:32PM +0300, Dan Carpenter wrote:
> 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

> 
> regards,
> dan carpenter
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..151a630 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -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);