[PATCH] iio staging: tsl2x7x: clean up limit checks

[PATCH] iio staging: tsl2x7x: clean up limit checks

[Dan Carpenter]

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@oracle.com>

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,

Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

[Jonathan Cameron]

On Mon, 21 Aug 2017 13:11:03 +0300
Dan Carpenter <dan.carpenter@oracle.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@oracle.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,

Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

[Brian Masney]

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@oracle.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@oracle.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.

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.

Brian

Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

[Dan Carpenter]

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@oracle.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@oracle.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

Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

[Brian Masney]

On Tue, Sep 05, 2017 at 05:58:54PM +0300, Dan Carpenter wrote:
> 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@oracle.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@oracle.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.

Agreed. Looking through this again, this improves the existing code and
can be applied. Jonathan, you can add my:

Acked-by: Brian Masney <masneyb@onstation.org>

I think we need a followup patch to improve this further before the
driver can be moved out of staging. Namely, if a new entry is added
to tsl2x7x_default_lux_table_group with more than 3 elements, then the
user will possibly see erroneous readings from the sensor. To make it
easier to review future changes, what do you think about removing the
MAX_DEFAULT_TABLE_BYTES #define and replacing it with a new array
below tsl2x7x_default_lux_table_group that has the size of each default
lux table? The new array will only be used by the memcpy() call above.
If it sounds acceptable, then I can add that to my queue. I'm going to
start working on this driver again this week after a few months of being
away from it.

Brian

Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

[Brian Masney]

On Tue, Sep 05, 2017 at 05:02:55PM -0400, Brian Masney wrote:
> On Tue, Sep 05, 2017 at 05:58:54PM +0300, Dan Carpenter wrote:
> > 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@oracle.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@oracle.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.
> 
> Agreed. Looking through this again, this improves the existing code and
> can be applied. Jonathan, you can add my:
> 
> Acked-by: Brian Masney <masneyb@onstation.org>
> 
> I think we need a followup patch to improve this further before the
> driver can be moved out of staging. Namely, if a new entry is added
> to tsl2x7x_default_lux_table_group with more than 3 elements, then the
> user will possibly see erroneous readings from the sensor. To make it
> easier to review future changes, what do you think about removing the
> MAX_DEFAULT_TABLE_BYTES #define and replacing it with a new array
> below tsl2x7x_default_lux_table_group that has the size of each default
> lux table? The new array will only be used by the memcpy() call above.
> If it sounds acceptable, then I can add that to my queue. I'm going to
> start working on this driver again this week after a few months of being
> away from it.

Alternatively, we can keep this simple and just add a comment above the
definition of tsl2x7x_default_lux_table_group saying that the number of
rows in all of the various defaults need to be the same. If additional
rows are added, then the definition of MAX_DEFAULT_TABLE_BYTES needs to
be changed as well.

Brian

Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

[Dan Carpenter]

Let me try send a v2 and see what you think.

regards,
dan carpenter

[PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Dan Carpenter]

The background of this code is that we can either use the default
tables or load our own table with sysfs.  The default tables are three
element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
then we can have as many as nine elements.  Which ever way we do it, the
last element is always zeroed out.

The most interesting part of this patch is in the
in_illuminance0_lux_table_show() function.  We were using the wrong
limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
"TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
that we are going of of bounds.  However, since the last element is
always zeroed out, that means we hit the break statement and the code
works correctly despite the wrong limit check.

I made several related readability changes.  The most notable that I
changed the MAX_DEFAULT_TABLE_BYTES define which was:

#define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)

I renamed the define to TSL2X7X_DEFAULT_TABLE_BYTES because it's not the
max size, it's the only size.  Also the size should really be expressed
as sizeof(struct tsl2x7x_lux) * 3.  In other words, 12 * 3 instead of
4 * 9.  It's 36 bytes either way, so this doesn't change the behavior.

Finally, I created the TSL2X7X_DEF_LUX_TABLE_SZ define instead of using
the magic number 3.  I declared the default tables using that define
to hopefully signal to future programmers that if they want to use a
different size they have to update all the related code.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h
index ecae92211216..a216c6943a84 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,13 @@ struct tsl2x7x_lux {
 	unsigned int ch1;
 };
 
+/* Max number of segments allowable in LUX table */
+#define TSL2X7X_MAX_LUX_TABLE_SIZE		9
+/* The default LUX tables all have 3 elements.  */
+#define TSL2X7X_DEF_LUX_TABLE_SZ		3
+#define TSL2X7X_DEFAULT_TABLE_BYTES (sizeof(struct tsl2x7x_lux) * \
+				     TSL2X7X_DEF_LUX_TABLE_SZ)
+
 /**
  * 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..b3a9efecf536 100644
--- a/drivers/staging/iio/light/tsl2x7x.c
+++ b/drivers/staging/iio/light/tsl2x7x.c
@@ -192,25 +192,25 @@ struct tsl2X7X_chip {
 };
 
 /* Different devices require different coefficents */
-static const struct tsl2x7x_lux tsl2x71_lux_table[] = {
+static const struct tsl2x7x_lux tsl2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
 	{ 14461,   611,   1211 },
 	{ 18540,   352,    623 },
 	{     0,     0,      0 },
 };
 
-static const struct tsl2x7x_lux tmd2x71_lux_table[] = {
+static const struct tsl2x7x_lux tmd2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
 	{ 11635,   115,    256 },
 	{ 15536,    87,    179 },
 	{     0,     0,      0 },
 };
 
-static const struct tsl2x7x_lux tsl2x72_lux_table[] = {
+static const struct tsl2x7x_lux tsl2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
 	{ 14013,   466,   917 },
 	{ 18222,   310,   552 },
 	{     0,     0,     0 },
 };
 
-static const struct tsl2x7x_lux tmd2x72_lux_table[] = {
+static const struct tsl2x7x_lux tmd2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
 	{ 13218,   130,   262 },
 	{ 17592,   92,    169 },
 	{     0,     0,     0 },
@@ -530,7 +530,7 @@ static void tsl2x7x_defaults(struct tsl2X7X_chip *chip)
 	else
 		memcpy(chip->tsl2x7x_device_lux,
 		(struct tsl2x7x_lux *)tsl2x7x_default_lux_table_group[chip->id],
-				MAX_DEFAULT_TABLE_BYTES);
+				TSL2X7X_DEFAULT_TABLE_BYTES);
 }
 
 /**
@@ -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,

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[walter harms]

Am 08.09.2017 12:53, schrieb Dan Carpenter:
> The background of this code is that we can either use the default
> tables or load our own table with sysfs.  The default tables are three
> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> then we can have as many as nine elements.  Which ever way we do it, the
> last element is always zeroed out.
> 
> The most interesting part of this patch is in the
> in_illuminance0_lux_table_show() function.  We were using the wrong
> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> that we are going of of bounds.  However, since the last element is
> always zeroed out, that means we hit the break statement and the code
> works correctly despite the wrong limit check.
> 
> I made several related readability changes.  The most notable that I
> changed the MAX_DEFAULT_TABLE_BYTES define which was:
> 
> #define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)
> 
> I renamed the define to TSL2X7X_DEFAULT_TABLE_BYTES because it's not the
> max size, it's the only size.  Also the size should really be expressed
> as sizeof(struct tsl2x7x_lux) * 3.  In other words, 12 * 3 instead of
> 4 * 9.  It's 36 bytes either way, so this doesn't change the behavior.
> 
> Finally, I created the TSL2X7X_DEF_LUX_TABLE_SZ define instead of using
> the magic number 3.  I declared the default tables using that define
> to hopefully signal to future programmers that if they want to use a
> different size they have to update all the related code.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h
> index ecae92211216..a216c6943a84 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,13 @@ struct tsl2x7x_lux {
>  	unsigned int ch1;
>  };
>  
> +/* Max number of segments allowable in LUX table */
> +#define TSL2X7X_MAX_LUX_TABLE_SIZE		9
> +/* The default LUX tables all have 3 elements.  */
> +#define TSL2X7X_DEF_LUX_TABLE_SZ		3
> +#define TSL2X7X_DEFAULT_TABLE_BYTES (sizeof(struct tsl2x7x_lux) * \
> +				     TSL2X7X_DEF_LUX_TABLE_SZ)
> +
>  /**
>   * 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..b3a9efecf536 100644
> --- a/drivers/staging/iio/light/tsl2x7x.c
> +++ b/drivers/staging/iio/light/tsl2x7x.c
> @@ -192,25 +192,25 @@ struct tsl2X7X_chip {
>  };
>  
>  /* Different devices require different coefficents */
> -static const struct tsl2x7x_lux tsl2x71_lux_table[] = {
> +static const struct tsl2x7x_lux tsl2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 14461,   611,   1211 },
>  	{ 18540,   352,    623 },
>  	{     0,     0,      0 },
>  };
>  
> -static const struct tsl2x7x_lux tmd2x71_lux_table[] = {
> +static const struct tsl2x7x_lux tmd2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 11635,   115,    256 },
>  	{ 15536,    87,    179 },
>  	{     0,     0,      0 },
>  };
>  
> -static const struct tsl2x7x_lux tsl2x72_lux_table[] = {
> +static const struct tsl2x7x_lux tsl2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 14013,   466,   917 },
>  	{ 18222,   310,   552 },
>  	{     0,     0,     0 },
>  };
>  
> -static const struct tsl2x7x_lux tmd2x72_lux_table[] = {
> +static const struct tsl2x7x_lux tmd2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 13218,   130,   262 },
>  	{ 17592,   92,    169 },
>  	{     0,     0,     0 },
> @@ -530,7 +530,7 @@ static void tsl2x7x_defaults(struct tsl2X7X_chip *chip)
>  	else
>  		memcpy(chip->tsl2x7x_device_lux,
>  		(struct tsl2x7x_lux *)tsl2x7x_default_lux_table_group[chip->id],
> -				MAX_DEFAULT_TABLE_BYTES);
> +				TSL2X7X_DEFAULT_TABLE_BYTES);
>  }
>  
>  /**
> @@ -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,

Is that TSL2X7X_MAX_LUX_TABLE_SIZE needed at all ? because:

if (chip->tsl2x7x_device_lux[i].ratio == 0) ... break;

So its only needed for protection agains faulty updates. (maybe i missed an other use).


NTL so far i see the user will always get something like:
14013,   466,   917, 15536,    87,    179 , 0,     0,     0
are the last three 0 need for something ? compatibility ? otherwise i would suggest
something like:

for (i=0;
     chip->tsl2x7x_device_lux[i].ratio != 0;
     i++)
 snprintf() ...

just my 2 cents,
re
 wh
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Dan Carpenter]

On Fri, Sep 08, 2017 at 03:48:58PM +0200, walter harms wrote:
> > -	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,
> 
> Is that TSL2X7X_MAX_LUX_TABLE_SIZE needed at all ?

Nope.  Not needed but not harmful.  Adding redundant limit checks is
pretty normal so it's fine.  I'm not going to remove it.

regards,
dan carpenter

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Brian Masney]

On Fri, Sep 08, 2017 at 01:53:43PM +0300, Dan Carpenter wrote:
> The background of this code is that we can either use the default
> tables or load our own table with sysfs.  The default tables are three
> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> then we can have as many as nine elements.  Which ever way we do it, the
> last element is always zeroed out.
> 
> The most interesting part of this patch is in the
> in_illuminance0_lux_table_show() function.  We were using the wrong
> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> that we are going of of bounds.  However, since the last element is
> always zeroed out, that means we hit the break statement and the code
> works correctly despite the wrong limit check.
> 
> I made several related readability changes.  The most notable that I
> changed the MAX_DEFAULT_TABLE_BYTES define which was:
> 
> #define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)
> 
> I renamed the define to TSL2X7X_DEFAULT_TABLE_BYTES because it's not the
> max size, it's the only size.  Also the size should really be expressed
> as sizeof(struct tsl2x7x_lux) * 3.  In other words, 12 * 3 instead of
> 4 * 9.  It's 36 bytes either way, so this doesn't change the behavior.
> 
> Finally, I created the TSL2X7X_DEF_LUX_TABLE_SZ define instead of using
> the magic number 3.  I declared the default tables using that define
> to hopefully signal to future programmers that if they want to use a
> different size they have to update all the related code.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Brian Masney <masneyb@onstation.org>

Brian

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Paolo Cretaro]

Hi Dan,
just minor nitpicking on the commit message:

On 08/09/2017 12:53, Dan Carpenter wrote:
> The background of this code is that we can either use the default
> tables or load our own table with sysfs.  The default tables are three
> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> then we can have as many as nine elements.  Which ever way we do it, the
> last element is always zeroed out.
> 
> The most interesting part of this patch is in the
> in_illuminance0_lux_table_show() function.  We were using the wrong
> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> that we are going of of bounds.  However, since the last element is
out of bounds

Regards,
P.

> always zeroed out, that means we hit the break statement and the code
> works correctly despite the wrong limit check.
> 
> I made several related readability changes.  The most notable that I
> changed the MAX_DEFAULT_TABLE_BYTES define which was:
> 
> #define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)
> 
> I renamed the define to TSL2X7X_DEFAULT_TABLE_BYTES because it's not the
> max size, it's the only size.  Also the size should really be expressed
> as sizeof(struct tsl2x7x_lux) * 3.  In other words, 12 * 3 instead of
> 4 * 9.  It's 36 bytes either way, so this doesn't change the behavior.
> 
> Finally, I created the TSL2X7X_DEF_LUX_TABLE_SZ define instead of using
> the magic number 3.  I declared the default tables using that define
> to hopefully signal to future programmers that if they want to use a
> different size they have to update all the related code.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/staging/iio/light/tsl2x7x.h b/drivers/staging/iio/light/tsl2x7x.h
> index ecae92211216..a216c6943a84 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,13 @@ struct tsl2x7x_lux {
>  	unsigned int ch1;
>  };
>  
> +/* Max number of segments allowable in LUX table */
> +#define TSL2X7X_MAX_LUX_TABLE_SIZE		9
> +/* The default LUX tables all have 3 elements.  */
> +#define TSL2X7X_DEF_LUX_TABLE_SZ		3
> +#define TSL2X7X_DEFAULT_TABLE_BYTES (sizeof(struct tsl2x7x_lux) * \
> +				     TSL2X7X_DEF_LUX_TABLE_SZ)
> +
>  /**
>   * 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..b3a9efecf536 100644
> --- a/drivers/staging/iio/light/tsl2x7x.c
> +++ b/drivers/staging/iio/light/tsl2x7x.c
> @@ -192,25 +192,25 @@ struct tsl2X7X_chip {
>  };
>  
>  /* Different devices require different coefficents */
> -static const struct tsl2x7x_lux tsl2x71_lux_table[] = {
> +static const struct tsl2x7x_lux tsl2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 14461,   611,   1211 },
>  	{ 18540,   352,    623 },
>  	{     0,     0,      0 },
>  };
>  
> -static const struct tsl2x7x_lux tmd2x71_lux_table[] = {
> +static const struct tsl2x7x_lux tmd2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 11635,   115,    256 },
>  	{ 15536,    87,    179 },
>  	{     0,     0,      0 },
>  };
>  
> -static const struct tsl2x7x_lux tsl2x72_lux_table[] = {
> +static const struct tsl2x7x_lux tsl2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 14013,   466,   917 },
>  	{ 18222,   310,   552 },
>  	{     0,     0,     0 },
>  };
>  
> -static const struct tsl2x7x_lux tmd2x72_lux_table[] = {
> +static const struct tsl2x7x_lux tmd2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] = {
>  	{ 13218,   130,   262 },
>  	{ 17592,   92,    169 },
>  	{     0,     0,     0 },
> @@ -530,7 +530,7 @@ static void tsl2x7x_defaults(struct tsl2X7X_chip *chip)
>  	else
>  		memcpy(chip->tsl2x7x_device_lux,
>  		(struct tsl2x7x_lux *)tsl2x7x_default_lux_table_group[chip->id],
> -				MAX_DEFAULT_TABLE_BYTES);
> +				TSL2X7X_DEFAULT_TABLE_BYTES);
>  }
>  
>  /**
> @@ -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,
> 

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Dan Carpenter]

On Sat, Sep 16, 2017 at 01:11:29PM +0200, Paolo Cretaro wrote:
> Hi Dan,
> just minor nitpicking on the commit message:
> 
> On 08/09/2017 12:53, Dan Carpenter wrote:
> > The background of this code is that we can either use the default
> > tables or load our own table with sysfs.  The default tables are three
> > element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> > then we can have as many as nine elements.  Which ever way we do it, the
> > last element is always zeroed out.
> > 
> > The most interesting part of this patch is in the
> > in_illuminance0_lux_table_show() function.  We were using the wrong
> > limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> > "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> > that we are going of of bounds.  However, since the last element is
> out of bounds
> 
> Regards,
> P.
> 
> > always zeroed out, that means we hit the break statement and the code
> > works correctly despite the wrong limit check.

What?  No no.  I meant it how I wrote it.  The last element is
always zeroed out meaning it's just a series of zeroes.

regards,
dan carpenter

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Paolo Cretaro]

On 16/09/2017 13:37, Dan Carpenter wrote:
> On Sat, Sep 16, 2017 at 01:11:29PM +0200, Paolo Cretaro wrote:
>> Hi Dan,
>> just minor nitpicking on the commit message:
>>
>> On 08/09/2017 12:53, Dan Carpenter wrote:
>>> The background of this code is that we can either use the default
>>> tables or load our own table with sysfs.  The default tables are three
>>> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
>>> then we can have as many as nine elements.  Which ever way we do it, the
>>> last element is always zeroed out.
>>>
>>> The most interesting part of this patch is in the
>>> in_illuminance0_lux_table_show() function.  We were using the wrong
>>> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
>>> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
>>> that we are going of of bounds.  However, since the last element is
>> out of bounds
>>
>> Regards,
>> P.
>>
>>> always zeroed out, that means we hit the break statement and the code
>>> works correctly despite the wrong limit check.
> 
> What?  No no.  I meant it how I wrote it.  The last element is
> always zeroed out meaning it's just a series of zeroes.

Sorry, I meant the previous sentence "This creates a static checker warning
that we are going of of bounds".

Regards,
P.

> 
> regards,
> dan carpenter
> 

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Jonathan Cameron]

On Sat, 16 Sep 2017 14:18:52 +0200
Paolo Cretaro <melko@frugalware.org> wrote:

> On 16/09/2017 13:37, Dan Carpenter wrote:
> > On Sat, Sep 16, 2017 at 01:11:29PM +0200, Paolo Cretaro wrote:  
> >> Hi Dan,
> >> just minor nitpicking on the commit message:
> >>
> >> On 08/09/2017 12:53, Dan Carpenter wrote:  
> >>> The background of this code is that we can either use the default
> >>> tables or load our own table with sysfs.  The default tables are three
> >>> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> >>> then we can have as many as nine elements.  Which ever way we do it, the
> >>> last element is always zeroed out.
> >>>
> >>> The most interesting part of this patch is in the
> >>> in_illuminance0_lux_table_show() function.  We were using the wrong
> >>> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> >>> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> >>> that we are going of of bounds.  However, since the last element is  
> >> out of bounds
> >>
> >> Regards,
> >> P.
> >>  
> >>> always zeroed out, that means we hit the break statement and the code
> >>> works correctly despite the wrong limit check.  
> > 
> > What?  No no.  I meant it how I wrote it.  The last element is
> > always zeroed out meaning it's just a series of zeroes.  
> 
> Sorry, I meant the previous sentence "This creates a static checker warning
> that we are going of of bounds".

Double of fixed and patch applied to the togreg branch of iio.git.

Thanks,

Jonathan
> 
> Regards,
> P.
> 
> > 
> > regards,
> > dan carpenter
> >   
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[Dan Carpenter]

On Sat, Sep 16, 2017 at 02:18:52PM +0200, Paolo Cretaro wrote:
> On 16/09/2017 13:37, Dan Carpenter wrote:
> > On Sat, Sep 16, 2017 at 01:11:29PM +0200, Paolo Cretaro wrote:
> >> Hi Dan,
> >> just minor nitpicking on the commit message:
> >>
> >> On 08/09/2017 12:53, Dan Carpenter wrote:
> >>> The background of this code is that we can either use the default
> >>> tables or load our own table with sysfs.  The default tables are three
> >>> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> >>> then we can have as many as nine elements.  Which ever way we do it, the
> >>> last element is always zeroed out.
> >>>
> >>> The most interesting part of this patch is in the
> >>> in_illuminance0_lux_table_show() function.  We were using the wrong
> >>> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> >>> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> >>> that we are going of of bounds.  However, since the last element is
> >> out of bounds
> >>
> >> Regards,
> >> P.
> >>
> >>> always zeroed out, that means we hit the break statement and the code
> >>> works correctly despite the wrong limit check.
> > 
> > What?  No no.  I meant it how I wrote it.  The last element is
> > always zeroed out meaning it's just a series of zeroes.
> 
> Sorry, I meant the previous sentence "This creates a static checker warning
> that we are going of of bounds".
> 

Ah.  Of course.  Thanks Jonathan for fixing this.

regards,
dan carpenter