[PATCH][next] driver core: platform: fix u32 greater or equal to zero comparison

[PATCH][next] driver core: platform: fix u32 greater or equal to zero comparison

[Colin King]

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

Currently the check that a u32 variable i is >= 0 is always true because
the unsigned variable will never be negative, causing the loop to run
forever.  Fix this by changing the pre-decrement check to a zero check on
i followed by a decrement of i.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: 39cc539f90d0 ("driver core: platform: Prevent resouce overflow from causing infinite loops")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/base/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 864b53b3d598..7fa654f1288b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -571,7 +571,7 @@ int platform_device_add(struct platform_device *pdev)
 		pdev->id = PLATFORM_DEVID_AUTO;
 	}
 
-	while (--i >= 0) {
+	while (i--) {
 		struct resource *r = &pdev->resource[i];
 		if (r->parent)
 			release_resource(r);
-- 
2.24.0

Re: [PATCH][next] driver core: platform: fix u32 greater or equal to zero comparison

[Rafael J. Wysocki]

On Thu, Jan 16, 2020 at 6:58 PM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the check that a u32 variable i is >= 0 is always true because
> the unsigned variable will never be negative, causing the loop to run
> forever.  Fix this by changing the pre-decrement check to a zero check on
> i followed by a decrement of i.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 39cc539f90d0 ("driver core: platform: Prevent resouce overflow from causing infinite loops")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/base/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 864b53b3d598..7fa654f1288b 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -571,7 +571,7 @@ int platform_device_add(struct platform_device *pdev)
>                 pdev->id = PLATFORM_DEVID_AUTO;
>         }
>
> -       while (--i >= 0) {
> +       while (i--) {
>                 struct resource *r = &pdev->resource[i];
>                 if (r->parent)
>                         release_resource(r);
> --
> 2.24.0
>

Re: [PATCH][next] driver core: platform: fix u32 greater or equal to zero comparison

[Dan Carpenter]

On Thu, Jan 16, 2020 at 05:57:58PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the check that a u32 variable i is >= 0 is always true because
> the unsigned variable will never be negative, causing the loop to run
> forever.  Fix this by changing the pre-decrement check to a zero check on
> i followed by a decrement of i.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 39cc539f90d0 ("driver core: platform: Prevent resouce overflow from causing infinite loops")

A better fix would be to revert this patch.  It doesn't fix a real bug.
The ->num_resources is typically under 5.  It's not going to overflow
INT_MAX any time soon.  There are "architectures with smaller ints."

It should always be "int i" unless there is a valid real life reason.
People think that declaring everything as u32 will fix bugs but it
normally just introduces bugs as it does here.  u32 makes the code
harder to read.

This is a sore spot for me because apparently there is a static
analysis tool which tells people to use "u32 i;" everywhere.  It's bad
advice.  I have asked around but I haven't found which tool it is, but
which  we should find the tool and delete it to prevent this kind of
stuff in the future.

regards,
dan carpenter

Re: [PATCH][next] driver core: platform: fix u32 greater or equal to zero comparison

[Dan Carpenter]

On Fri, Jan 17, 2020 at 07:42:16AM +0300, Dan Carpenter wrote:
> On Thu, Jan 16, 2020 at 05:57:58PM +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently the check that a u32 variable i is >= 0 is always true because
> > the unsigned variable will never be negative, causing the loop to run
> > forever.  Fix this by changing the pre-decrement check to a zero check on
> > i followed by a decrement of i.
> > 
> > Addresses-Coverity: ("Unsigned compared against 0")
> > Fixes: 39cc539f90d0 ("driver core: platform: Prevent resouce overflow from causing infinite loops")
> 
> A better fix would be to revert this patch.  It doesn't fix a real bug.
> The ->num_resources is typically under 5.  It's not going to overflow
> INT_MAX any time soon.  There are "architectures with smaller ints."

I left out a word.  There are *no* "architectures with smaller ints."...

I mean there used to be systems with 16 bit int but that was before I
was born.  You could never run linux on them.

regards,
dan carpenter

Re: [PATCH][next] driver core: platform: fix u32 greater or equal to zero comparison

[Dan Carpenter]

On Fri, Jan 17, 2020 at 07:42:16AM +0300, Dan Carpenter wrote:
> On Thu, Jan 16, 2020 at 05:57:58PM +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently the check that a u32 variable i is >= 0 is always true because
> > the unsigned variable will never be negative, causing the loop to run
> > forever.  Fix this by changing the pre-decrement check to a zero check on
> > i followed by a decrement of i.
> > 
> > Addresses-Coverity: ("Unsigned compared against 0")
> > Fixes: 39cc539f90d0 ("driver core: platform: Prevent resouce overflow from causing infinite loops")
> 

Also by the way say you have:

	unsigned int limit = (unsigned)INT_MAX + 4;
	int i;

	for (i = 0; i < limit; i++)
		;
	printf("%d\n", i);


The loop will work the same way regardless of if int is signed or not
because of type promotion.

regards,
dan carpenter