linux-hardening - Re: [PATCH] lm85: Bounds check to_sensor_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b3829a0f-ff83-651e-f78d-794565b023ec@roeck-us.net>
Date:   Sun, 5 Feb 2023 09:37:22 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Kees Cook <keescook@...omium.org>
Cc:     Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] lm85: Bounds check to_sensor_dev_attr()->index usage

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