On Sun, Jul 08, 2012 at 12:21:10PM +0200, Jean Delvare wrote: > Hi Guenter, > > On Sat, 7 Jul 2012 21:48:57 -0700, Guenter Roeck wrote: > > On Sat, Jul 07, 2012 at 04:48:34PM +0200, Jean Delvare wrote: > > > 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, > > I don't know how you can be sure of that, but if this is the case then > I agree your patch is good to go. > Trip point values are stored in mW, and reported/set in uW. I just thought it unlikely that we ever have to deal with trip points above 4 Billion uW or 40 Million Watt. On the other side, everything else is kept and stored in u64, so from that perspective it makes sense to use the same units for trip points. So I'll change the code to use s64 instead of int for trip[], which means that I can drop the check for INT_MAX in set_trip(). > > and having to add even more code > > and maybe variables to handle the not-set case does not sound very compelling > > to me either. > > You could use s64 and keep -1 for unset, or u64 and 0 or ULLONG_MAX for > unset; no need for extra variables. But if you're sure it's not needed, > we can keep the code as it is. > > > > 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 ? > > No, I was. I hadn't seen the patch "hwmon: (acpi_power_meter) Cleanup > and optimizations" in your hwmon-staging tree. This indeed solves my > above concern. > Uh, sorry, I forgot about it too. On the other side I missed your Ack for it ... > > > 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. > > Not sure what you call "real" limits in this context, but anyway, if > 64-bit values are supposed to be possible, then kstrtoull should be > used instead of kstrtoul. There's no way acpi_evaluate_integer() can > work around this afterward. Still, I agree it would be a separate > patch, if anyone cares. > "real" as in actual limits as provided and/or determined by ACPI. Anyway, I may have been wrong with that, as apparently acpi_evaluate_integer() is not used to validate a value, but to set it (or both ?). _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
