From mboxrd@z Thu Jan 1 00:00:00 1970 From: Colin Ian King Subject: Re: [PATCH] gpio: ep93xx: fix test for end of loop Date: Thu, 6 Sep 2018 16:18:44 +0100 Message-ID: <6098ff32-7f64-38db-8937-a9d257935c97@canonical.com> References: <20180906133348.brzvqt53nzukn7n2@kili.mountain> <21c6b967-f4b5-db68-89b4-a9caa4e76fda@canonical.com> <20180906144838.2dtqexjqieqjdpub@mwanda> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20180906144838.2dtqexjqieqjdpub@mwanda> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Dan Carpenter Cc: Linus Walleij , linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org List-Id: linux-gpio@vger.kernel.org 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 >>> >>> 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Colin Ian King Date: Thu, 06 Sep 2018 15:18:44 +0000 Subject: Re: [PATCH] gpio: ep93xx: fix test for end of loop Message-Id: <6098ff32-7f64-38db-8937-a9d257935c97@canonical.com> List-Id: References: <20180906133348.brzvqt53nzukn7n2@kili.mountain> <21c6b967-f4b5-db68-89b4-a9caa4e76fda@canonical.com> <20180906144838.2dtqexjqieqjdpub@mwanda> In-Reply-To: <20180906144838.2dtqexjqieqjdpub@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter Cc: Linus Walleij , linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org 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 >>> >>> 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 >