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@xxxxxxxxxx>
>>>
>>> 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
>
