[PATCH] gpio: ep93xx: fix incorrect array element size check

[PATCH] gpio: ep93xx: fix incorrect array element size check

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Currently the while loop checks for the end of the array using
the size of egp->gc rather that the number of elements in the array,
so fix this. Also, perform the array size check first as stylistically
it is always good to bounds check on an array first before referencing
the array (in this case, we're just computing the address of an
element in an array so this is a moot point).

Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpio/gpio-ep93xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 68a416fc3141..dd22ea19c3ed 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -76,7 +76,7 @@ static int ep93xx_gpio_port(struct gpio_chip *gc)
 	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
 	int port = 0;
 
-	while (gc != &epg->gc[port] && port < sizeof(epg->gc))
+	while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port])
 		port++;
 
 	/* This should not happen but is there as a last safeguard */
-- 
2.17.1

Re: [PATCH] gpio: ep93xx: fix incorrect array element size check

[Linus Walleij]

On Thu, Sep 6, 2018 at 1:58 PM Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the while loop checks for the end of the array using
> the size of egp->gc rather that the number of elements in the array,
> so fix this. Also, perform the array size check first as stylistically
> it is always good to bounds check on an array first before referencing
> the array (in this case, we're just computing the address of an
> element in an array so this is a moot point).
>
> Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Oh that was really neat code! Thanks a lot.

Patch applied.

Yours,
Linus Walleij

[PATCH] gpio: ep93xx: fix test for end of loop

[Dan Carpenter]

The problem is that if port == ARRAY_SIZE() and "gc == &epg->gc[port]"
then that should be treated as invalid.

Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
index 68a416fc3141..b0699f57ddf5 100644
--- a/drivers/gpio/gpio-ep93xx.c
+++ b/drivers/gpio/gpio-ep93xx.c
@@ -80,7 +80,7 @@ static int ep93xx_gpio_port(struct gpio_chip *gc)
 		port++;
 
 	/* This should not happen but is there as a last safeguard */
-	if (gc != &epg->gc[port]) {
+	if (port == ARRAY_SIZE(epg->gc)) {
 		pr_crit("can't find the GPIO port\n");
 		return 0;
 	}

Re: [PATCH] gpio: ep93xx: fix test for end of loop

[Colin Ian King]

On 06/09/18 14:33, Dan Carpenter wrote:
> The problem is that if port == ARRAY_SIZE() and "gc == &epg->gc[port]"
> then that should be treated as invalid.
> 
> Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 68a416fc3141..b0699f57ddf5 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -80,7 +80,7 @@ static int ep93xx_gpio_port(struct gpio_chip *gc)
>  		port++;
>  
>  	/* This should not happen but is there as a last safeguard */
> -	if (gc != &epg->gc[port]) {
> +	if (port == ARRAY_SIZE(epg->gc)) {
>  		pr_crit("can't find the GPIO port\n");
>  		return 0;
>  	}
> 

Good catch! I overlooked that one.

Re: [PATCH] gpio: ep93xx: fix test for end of loop

[Linus Walleij]

On Thu, Sep 6, 2018 at 3:34 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:

> The problem is that if port == ARRAY_SIZE() and "gc == &epg->gc[port]"
> then that should be treated as invalid.
>
> Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch applied!

Yours,
Linus Walleij

Re: [PATCH] gpio: ep93xx: fix test for end of loop

[Dan Carpenter]

On Thu, Sep 06, 2018 at 02:50:44PM +0100, Colin Ian King wrote:
> On 06/09/18 14:33, Dan Carpenter wrote:
> > The problem is that if port == ARRAY_SIZE() and "gc == &epg->gc[port]"
> > then that should be treated as invalid.
> > 
> > Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> > index 68a416fc3141..b0699f57ddf5 100644
> > --- a/drivers/gpio/gpio-ep93xx.c
> > +++ b/drivers/gpio/gpio-ep93xx.c
> > @@ -80,7 +80,7 @@ static int ep93xx_gpio_port(struct gpio_chip *gc)
> >  		port++;
> >  
> >  	/* This should not happen but is there as a last safeguard */
> > -	if (gc != &epg->gc[port]) {
> > +	if (port == ARRAY_SIZE(epg->gc)) {
> >  		pr_crit("can't find the GPIO port\n");
> >  		return 0;
> >  	}
> > 
> 
> Good catch! I overlooked that one.

It's unfortunate but I don't think any of our static checkers would have
caught it.  You found this bug because of cppcheck, right?  I know it
warns about the bounds test after use.  Does it also warn about the out
of bounds?

Smatch doesn't warn about the out of bounds read because Smatch has bad
handling of loops.  Smatch is supposed to warn about the test after use
but that code is buggy.  I will investigate what's going on.

I have an unpublished test so Smatch does warn that the port < sizeof()
means that port is in terms of bytes but we're using it as an array
offset.  That's how I noticed this code, but it only warns about the
first use, so it warns about the loop but not the post-loop test.

I should fix Smatch's handling of loops so that we know this function
originally could return 0-8 and you maybe get a warning in the caller.
Unfortunately, I'm not sure I would have paid very much attention to a
warning like that because they tend to be prone to false positives.  We
have a lot of loops that do:

	for (i = 0; i < ARRAY_SIZE(array); i++) {
		if (foo <= array[i])
			break;
	}

And the last element of the array[] is UINT_MAX so we always break.
It's a lot of work but not necessarily difficult work.

regards,
dan carpenter

Re: [PATCH] gpio: ep93xx: fix test for end of loop

[Colin Ian King]

On 06/09/18 15:48, Dan Carpenter wrote:
> On Thu, Sep 06, 2018 at 02:50:44PM +0100, Colin Ian King wrote:
>> On 06/09/18 14:33, Dan Carpenter wrote:
>>> The problem is that if port == ARRAY_SIZE() and "gc == &epg->gc[port]"
>>> then that should be treated as invalid.
>>>
>>> Fixes: fd935fc421e7 ("gpio: ep93xx: Do not pingpong irq numbers")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
>>> index 68a416fc3141..b0699f57ddf5 100644
>>> --- a/drivers/gpio/gpio-ep93xx.c
>>> +++ b/drivers/gpio/gpio-ep93xx.c
>>> @@ -80,7 +80,7 @@ static int ep93xx_gpio_port(struct gpio_chip *gc)
>>>  		port++;
>>>  
>>>  	/* This should not happen but is there as a last safeguard */
>>> -	if (gc != &epg->gc[port]) {
>>> +	if (port == ARRAY_SIZE(epg->gc)) {
>>>  		pr_crit("can't find the GPIO port\n");
>>>  		return 0;
>>>  	}
>>>
>>
>> Good catch! I overlooked that one.
> 
> It's unfortunate but I don't think any of our static checkers would have
> caught it.  You found this bug because of cppcheck, right?  I know it
> warns about the bounds test after use.  Does it also warn about the out
> of bounds?

It can catch these, not sure how well it compares to the other tools I use.

> 
> Smatch doesn't warn about the out of bounds read because Smatch has bad
> handling of loops.  Smatch is supposed to warn about the test after use
> but that code is buggy.  I will investigate what's going on.
> 
> I have an unpublished test so Smatch does warn that the port < sizeof()
> means that port is in terms of bytes but we're using it as an array
> offset.  That's how I noticed this code, but it only warns about the
> first use, so it warns about the loop but not the post-loop test.
> 
> I should fix Smatch's handling of loops so that we know this function
> originally could return 0-8 and you maybe get a warning in the caller.
> Unfortunately, I'm not sure I would have paid very much attention to a
> warning like that because they tend to be prone to false positives.  We
> have a lot of loops that do:
> 
> 	for (i = 0; i < ARRAY_SIZE(array); i++) {
> 		if (foo <= array[i])
> 			break;
> 	}

..yep, the are a lot of these to fix up for sure.

> 
> And the last element of the array[] is UINT_MAX so we always break.
> It's a lot of work but not necessarily difficult work.
> 
> regards,
> dan carpenter
>