* Re: [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow
2012-06-21 13:56 [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow Guenter Roeck
@ 2012-07-07 15:03 ` Jean Delvare
2012-07-08 4:57 ` Guenter Roeck
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2012-07-07 15:03 UTC (permalink / raw)
To: lm-sensors
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow
2012-06-21 13:56 [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow Guenter Roeck
2012-07-07 15:03 ` Jean Delvare
@ 2012-07-08 4:57 ` Guenter Roeck
2012-07-08 7:52 ` Jean Delvare
2012-07-09 10:28 ` Andreas Herrmann
3 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2012-07-08 4:57 UTC (permalink / raw)
To: lm-sensors
On Sat, Jul 07, 2012 at 05:03:09PM +0200, Jean Delvare wrote:
> Hi Guenter,
Hi Jean,
>
> 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;
>
Both tdp_limit and data->base_tdp can not be larger than 65535, so I figured
that the "inner" overflow is not an issue.
> [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.
>
Max: (65535 + 65535) << 16 = 8589803520 = 0x1FFFE0000. No idea if this is can
happen in the real world.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow
2012-06-21 13:56 [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow Guenter Roeck
2012-07-07 15:03 ` Jean Delvare
2012-07-08 4:57 ` Guenter Roeck
@ 2012-07-08 7:52 ` Jean Delvare
2012-07-09 10:28 ` Andreas Herrmann
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2012-07-08 7:52 UTC (permalink / raw)
To: lm-sensors
On Sat, 7 Jul 2012 21:57:47 -0700, Guenter Roeck wrote:
> On Sat, Jul 07, 2012 at 05:03:09PM +0200, Jean Delvare wrote:
> > On Thu, 21 Jun 2012 06:56:54 -0700, Guenter Roeck wrote:
> > > @@ -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;
> >
> Both tdp_limit and data->base_tdp can not be larger than 65535, so I figured
> that the "inner" overflow is not an issue.
Ah, right, I could have easily figured that out by myself, had I taken
the time to read the code. Sorry for the noise, and this patch of yours
is:
Acked-by: Jean Delvare <khali@linux-fr.org>
> > 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.
>
> Max: (65535 + 65535) << 16 = 8589803520 = 0x1FFFE0000. No idea if this is can
> happen in the real world.
No idea either. Maybe Andreas can tell. But if we have no reason to
believe this can't happen, I'd say we play it safe and push the fix to
stable trees.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow
2012-06-21 13:56 [lm-sensors] [PATCH 2/3] hwmon: (fam15h_power) Fix unintentional integer overflow Guenter Roeck
` (2 preceding siblings ...)
2012-07-08 7:52 ` Jean Delvare
@ 2012-07-09 10:28 ` Andreas Herrmann
3 siblings, 0 replies; 5+ messages in thread
From: Andreas Herrmann @ 2012-07-09 10:28 UTC (permalink / raw)
To: lm-sensors
On Sun, Jul 08, 2012 at 09:52:55AM +0200, Jean Delvare wrote:
> On Sat, 7 Jul 2012 21:57:47 -0700, Guenter Roeck wrote:
> > On Sat, Jul 07, 2012 at 05:03:09PM +0200, Jean Delvare wrote:
> > > On Thu, 21 Jun 2012 06:56:54 -0700, Guenter Roeck wrote:
> > > > @@ -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;
> > >
> > Both tdp_limit and data->base_tdp can not be larger than 65535, so I figured
> > that the "inner" overflow is not an issue.
>
> Ah, right, I could have easily figured that out by myself, had I taken
> the time to read the code. Sorry for the noise, and this patch of yours
> is:
>
> Acked-by: Jean Delvare <khali@linux-fr.org>
>
> > > 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.
> >
> > Max: (65535 + 65535) << 16 = 8589803520 = 0x1FFFE0000. No idea if this is can
> > happen in the real world.
>
> No idea either. Maybe Andreas can tell. But if we have no reason to
> believe this can't happen, I'd say we play it safe and push the fix to
> stable trees.
A typical value for base_tdp is 0x3f which would translate to about
14-15 Watts.
Yes in theory the computation can overflow. But that would mean
hardware reports senseless values for base_tdp.
I don't see an issue with current hardware but of course the
computation should be fixed. (I don't think that this fix is required
for stable trees but feel free to push it there.)
Acked-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Andreas
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 5+ messages in thread