linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Feb 2023 17:57:00 -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 Fri, Feb 03, 2023 at 10:35:05PM +0000, Kees Cook wrote:
> On Sat, Jan 28, 2023 at 05:13:19AM -0800, Guenter Roeck wrote:
> > On Fri, Jan 27, 2023 at 02:37:45PM -0800, Kees Cook wrote:
> > > The index into various register arrays was not bounds checked. Add
> > > checking. Seen under GCC 13:
> > > 
> > > drivers/hwmon/lm85.c: In function 'pwm_auto_pwm_minctl_store':
> > > drivers/hwmon/lm85.c:1110:26: warning: array subscript [0, 31] is outside array bounds of 'struct lm85_autofan[3]' [-Warray-bounds=]
> > >  1110 |         if (data->autofan[nr].min_off)
> > >       |             ~~~~~~~~~~~~~^~~~
> > > drivers/hwmon/lm85.c:317:29: note: while referencing 'autofan'
> > >   317 |         struct lm85_autofan autofan[3];
> > >       |                             ^~~~~~~
> > > 
> > 
> > This is a false positive. The value can never be >= 3.
> > It is derived from the last value of the following
> > SENSOR_DEVICE_ATTR_RW() entries.
> > 
> > I resist making changes like this to the code just because
> > the compiler can not determine the range of a variable.
> > It blows up code size amd makes it hard to read just to
> > make the compiler happy.
> 
> I think it's worth it given the index is an "int" and it'd be very easy
> for things to go wrong in the face of other memory corruption, etc. I've
> sent a v2 that I think is much more readable and non-invasive but
> provides similar robustness.
> 

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.

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.

Guenter