Messages in this thread | | | Date | Sun, 5 Feb 2023 09:37:22 -0800 | Subject | Re: [PATCH] lm85: Bounds check to_sensor_dev_attr()->index usage | From | Guenter Roeck <> |
| |
On 2/4/23 10:55, Kees Cook wrote: > On Fri, Feb 03, 2023 at 05:57:00PM -0800, Guenter Roeck wrote: >> That line of argument would suggest that we should perform parameter checks >> on each function entry all over the place, no matter if the range is known >> to be valid or not. Maybe that is the way things are going, but I don't >> like it at all. I have seen that kind of code before, in the telco space, >> where it typically at least doubled code size and resulted in mediocre >> performance, just because of a rule that mandated checking all parameters >> at the beginning of each function. > > Well, I doubt I'll be able to change your opinion of telco code, but I > do think robustness is not an unreasonable default state for software, > and that GCC and Clang do a pretty good job with optimization, etc. > >> I assume this is just one of many many patches you plan to send to add >> parameter checks to similar hwmon code ? I _really_ don't want to have >> the hwmon code cluttered with such unnecessary checks. > > I was trying to provide complete coverage inspired by the specific > complaint GCC had, but this would also silence the warning: > > diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c > index 8d33c2484755..87d2455e721f 100644 > --- a/drivers/hwmon/lm85.c > +++ b/drivers/hwmon/lm85.c > @@ -1106,6 +1106,7 @@ static ssize_t pwm_auto_pwm_minctl_store(struct device *dev, > mutex_lock(&data->update_lock); > data->autofan[nr].min_off = val; > tmp = lm85_read_value(client, LM85_REG_AFAN_SPIKE1); > + nr = clamp_t(int, nr, 0, ARRAY_SIZE(data->autofan) - 1); > tmp &= ~(0x20 << nr); > if (data->autofan[nr].min_off) > tmp |= 0x20 << nr; > > What's happening is GCC see that "nr" is used as a shift argument, so it > believes (not unreasonably) that this otherwise unknown value could be > up to 32. Here we can give it the bounded range ahead of time, keeping > it happy. > I'll accept that if you also add a note clarifying that this is to silence a gcc/clang warning.
Guenter
| |