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

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]

Message-ID: <20170903123505.13ee5a25@archlinux>
Date:   Sun, 3 Sep 2017 12:35:05 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     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,
        Brian Masney <masneyb@...tation.org>
Subject: Re: [PATCH] iio staging: tsl2x7x: clean up limit checks

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,