Re: [PATCH] lm85: Bounds check to_sensor_dev_attr()->index usage

From: Guenter Roeck <linux@roeck-us.net>
To: Kees Cook <keescook@chromium.org>
Cc: Jean Delvare <jdelvare@suse.com>,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] lm85: Bounds check to_sensor_dev_attr()->index usage
Date: Sun, 5 Feb 2023 09:37:22 -0800	[thread overview]
Message-ID: <b3829a0f-ff83-651e-f78d-794565b023ec@roeck-us.net> (raw)
In-Reply-To: <63deaa0b.050a0220.bf1e1.6c26@mx.google.com>

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