Re: [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow

[Jean Delvare <khali@linux-fr.org>]

Hi Guenter,

Adding Andreas to Cc...

On Thu, 21 Jun 2012 06:56:54 -0700, Guenter Roeck wrote:
> Expression with two unsigned integer variables is calculated as unsigned integer
> before it is converted to u64. This may result in an integer overflow.
> Fix by typecasting the left operand to u64 before performing the left shift.
> 
> This patch addresses Coverity #402320: Unintentional integer overflow.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/fam15h_power.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> index 6b13f1a..2764b78 100644
> --- a/drivers/hwmon/fam15h_power.c
> +++ b/drivers/hwmon/fam15h_power.c
> @@ -67,7 +67,8 @@ static ssize_t show_power(struct device *dev,
>  				  REG_TDP_LIMIT3, &val);
>  
>  	tdp_limit = val >> 16;
> -	curr_pwr_watts = (tdp_limit + data->base_tdp) << running_avg_range;
> +	curr_pwr_watts = ((u64)(tdp_limit +
> +				data->base_tdp)) << running_avg_range;

Even then, "tdp_limit + data->base_tdp" could overflow already, on
32-bit architectures [1]. So I think what you really want is:

	curr_pwr_watts = ((u64)tdp_limit + data->base_tdp) << running_avg_range;

[1] I'm not quite sure why one would run a 32-bit Linux on an AMD
Family 15h machine, but you never know...

>  	curr_pwr_watts -= running_avg_capture;
>  	curr_pwr_watts *= data->tdp_to_watts;

And BTW, this might be good to push to stable trees, as we've had real
problems with overflows in this driver in the past. I don't know the
realistic values for tdp_limit, base_tdp and running_avg_range so I'm
not sure.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors