| 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
| ||
|
Message-ID: <20170905143910.myrudtgydjbu7dci@mwanda>
Date: Tue, 5 Sep 2017 17:58:54 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: Brian Masney <masneyb@...tation.org>
Cc: Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
Jon.Brenner@....com
Subject: Re: [PATCH] iio staging: tsl2x7x: clean up limit checks
On Sun, Sep 03, 2017 at 10:12:46PM -0400, Brian Masney wrote:
> On Sun, Sep 03, 2017 at 12:35:05PM +0100, Jonathan Cameron wrote:
> > On Mon, 21 Aug 2017 13:11:03 +0300
> > Dan Carpenter <dan.carpenter@...cle.com> wrote:
> >
> > > The second part of this patch is probably the most interesting. We
> > > use "TSL2X7X_MAX_LUX_TABLE_SIZE * 3" as the limit instead of just
> > > "TSL2X7X_MAX_LUX_TABLE_SIZE". It creates a static checker warning that
> > > we are going of of bounds, but in real life we always hit the break
> > > statement on the last element so it's fine.
> > >
> > > The situation is that we normally have arrays with 3 elements of struct
> > > tsl2x7x_lux which has 3 unsigned integers. If we load the table with
> > > sysfs then we're allow to have 9 elements instead.
> > >
> > > So the size of the default table in bytes is sizeof(int) times 3 struct
> > > members times 3 elements. The original code wrote it as sizeof(int)
> > > times the number of elements in the bigger table (9). It happens that
> > > 9 is the same thing as 3 * 3 but expressing it that way is misleading.
> > >
> > > For the second part of the patch, the original code just had an extra
> > > "multiply by three" and now that is removed. The last element in the
> > > array is always zeroed memory whether this uses the default tables or it
> > > gets loaded with sysfs so we always hit the break statement anyway.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> >
> > Looks sensible to me.
> >
> > Cc'd Brian who has been working extensively on this driver recently as I'd
> > like his input.
> >
> > Jonathan
> > >
> > > diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h
> > > index ecae92211216..1beb8d2eb848 100644
> > > --- a/drivers/staging/iio/light/tsl2x7x.h
> > > +++ b/drivers/staging/iio/light/tsl2x7x.h
> > > @@ -23,10 +23,6 @@
> > > #define __TSL2X7X_H
> > > #include <linux/pm.h>
> > >
> > > -/* Max number of segments allowable in LUX table */
> > > -#define TSL2X7X_MAX_LUX_TABLE_SIZE 9
> > > -#define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)
> > > -
> > > struct iio_dev;
> > >
> > > struct tsl2x7x_lux {
> > > @@ -35,6 +31,11 @@ struct tsl2x7x_lux {
> > > unsigned int ch1;
> > > };
> > >
> > > +/* Max number of segments allowable in LUX table */
> > > +#define TSL2X7X_MAX_LUX_TABLE_SIZE 9
> > > +/* The default tables are all 3 elements */
> > > +#define MAX_DEFAULT_TABLE_BYTES (sizeof(struct tsl2x7x_lux) * 3)
> > > +
> > > /**
> > > * struct tsl2x7x_default_settings - power on defaults unless
> > > * overridden by platform data.
> > > diff --git a/drivers/staging/iio/light/tsl2x7x.c b/drivers/staging/iio/light/tsl2x7x.c
> > > index 786e93f16ce9..2db1715ff659 100644
> > > --- a/drivers/staging/iio/light/tsl2x7x.c
> > > +++ b/drivers/staging/iio/light/tsl2x7x.c
> > > @@ -1113,7 +1113,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev,
> > > int i = 0;
> > > int offset = 0;
> > >
> > > - while (i < (TSL2X7X_MAX_LUX_TABLE_SIZE * 3)) {
> > > + while (i < TSL2X7X_MAX_LUX_TABLE_SIZE) {
> > > offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,%u,",
> > > chip->tsl2x7x_device_lux[i].ratio,
> > > chip->tsl2x7x_device_lux[i].ch0,
>
> There is a minor issue regarding the structure sizes in with both this
> patch and the in-tree code. The following two structures define nine
> rows in the lux table:
>
> tsl2x7x.h:
> #define TSL2X7X_MAX_LUX_TABLE_SIZE 9
>
> struct tsl2X7X_platform_data {
> ...
> struct tsl2x7x_lux platform_lux_table[TSL2X7X_MAX_LUX_TABLE_SIZE];
> }
>
> tsl2x7x.c:
> struct tsl2X7X_chip {
> ...
> struct tsl2x7x_lux tsl2x7x_device_lux[TSL2X7X_MAX_LUX_TABLE_SIZE];
> }
>
> tsl2x7x_defaults() has this code snippet:
>
> memcpy(chip->tsl2x7x_device_lux,
> (struct tsl2x7x_lux *)tsl2x7x_default_lux_table_group[chip->id],
> MAX_DEFAULT_TABLE_BYTES);
>
> With the old and new code, memcpy will only copy the first three rows of
> the lux table. There is no security issue though with the actual
> implementation since the four *_lux_table structures that are defined in
> code only have three rows defined.
Agreed.
>
> I believe that the correct fix is to define MAX_DEFAULT_TABLE_BYTES in
> Dan's patch as follows (and keeping his other changes):
>
> #define MAX_DEFAULT_TABLE_BYTES (sizeof(struct tsl2x7x_lux) *
> TSL2X7X_MAX_LUX_TABLE_SIZE)
>
> We may also want to shorten those #defines to keep it under 80
> characters.
>
That's not a good idea because we would be filling chip->tsl2x7x_device_lux
with garbage from beyond the end of the tsl2x7x_default_lux_table_group[]
element. It would be harmless but ugly. We could just add a:
memset(chip->tsl2x7x_device_lux, 0, sizeof(chip->tsl2x7x_device_lux));
to the start of the tsl2x7x_defaults() and the in_illuminance0_lux_table_store()
functions. But I don't really see a need... This code will need to be
restructured a bit if we start using different sized default tables,
yes, but we can't really "future proof" this code without seeing what
the future change is.
regards,
dan carpenter
Powered by blists - more mailing lists