lkml.org 
[lkml]   [2023]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] lm85: Bounds check to_sensor_dev_attr()->index usage
From
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

\
 
 \ /
  Last update: 2023-03-27 00:10    [W:0.059 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site