[patch] thermal: underflow in trip_point_temp_store()

[patch] thermal: underflow in trip_point_temp_store()

[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);

Re: [patch] thermal: underflow in trip_point_temp_store()

[Eduardo Valentin]

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);

Re: [patch] thermal: underflow in trip_point_temp_store()

[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.

regards,
dan carpenter

Re: [patch] thermal: underflow in trip_point_temp_store()

[Eduardo Valentin]

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
> 

Re: [patch] thermal: underflow in trip_point_temp_store()

[walter harms]

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