On Sat, Jul 07, 2012 at 04:48:34PM +0200, Jean Delvare wrote: Hi Jean, > Hi Guenter, > > On Thu, 21 Jun 2012 06:56:53 -0700, Guenter Roeck wrote: > > Expression with two integer variables is calculated as integer before it is > > converted to u64. This may result in an integer overflow. Fix by declaring > > the constant as ULL. > > > > This patch addresses Coverity #200596: Unintentional integer overflow. > > > > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx> > > --- > > drivers/hwmon/acpi_power_meter.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c > > index 9a0821f1..c8ac1c6 100644 > > --- a/drivers/hwmon/acpi_power_meter.c > > +++ b/drivers/hwmon/acpi_power_meter.c > > @@ -434,7 +434,7 @@ static ssize_t show_val(struct device *dev, > > if (resource->trip[attr->index - 7] < 0) > > return sprintf(buf, "unknown\n"); > > > > - val = resource->trip[attr->index - 7] * 1000; > > + val = resource->trip[attr->index - 7] * 1000ULL; > > break; > > default: > > BUG(); > > That's one way to fix it... Me, I wonder why resource->trip is an > arrays of ints, when every other power value handled by the driver is a > u64. The trip values end up feeding acpi objects in set_acpi_trip, > where 64-bit values would be handled properly. So I don't know much > about this driver and I'm not using it, but maybe making trip values > 64-bit would be a better fix. Then you still have the problem of signed > vs. unsigned, as currently trip == -1 means unset, but this is probably > easy to solve. > It is not needed and would just consume memory, and having to add even more code and maybe variables to handle the not-set case does not sound very compelling to me either. > Also note that the trip values are gathered from sysfs using kstrtoul, > so a value between 0x80000000 and 0xffffffff would be silently stored > as a negative number. Maybe worth fixing... > The code in set_trip ensures that stored values are not larger than INT_MAX. Am I missing something ? > And I also see that kstrtoul is used for cap and avg_interval, while > they are stored as u64 in struct acpi_power_meter_resource. No overflow > here, but an arbitrary limitation on what values the user can set. > My understanding is that acpi_evaluate_integer() actually enforces "real" limits. Maybe I am wrong, though. Either case, that would be a separate patch. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
