| Message ID | 20210407152457.497346-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: uniphier: Fix potential infinite loop | expand |
On Thu, Apr 8, 2021 at 12:25 AM Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > The for-loop iterates with a u8 loop counter i and compares this > with the loop upper limit of num_parents that is an int type. > There is a potential infinite loop if num_parents is larger than > the u8 loop counter. Fix this by making the loop counter the same > type as num_parents. > > Addresses-Coverity: ("Infinite loop") > Fixes: 734d82f4a678 ("clk: uniphier: add core support code for UniPhier clock driver") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/uniphier/clk-uniphier-mux.c b/drivers/clk/uniphier/clk-uniphier-mux.c > index 462c84321b2d..ce219e0d2a85 100644 > --- a/drivers/clk/uniphier/clk-uniphier-mux.c > +++ b/drivers/clk/uniphier/clk-uniphier-mux.c > @@ -34,7 +34,7 @@ static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw) > int num_parents = clk_hw_get_num_parents(hw); > int ret; > unsigned int val; > - u8 i; > + int i; > > ret = regmap_read(mux->regmap, mux->reg, &val); > if (ret) > -- > 2.30.2 > clk_hw_get_num_parents() returns 'unsigned int', so I think 'num_parents' should also have been 'unsigned int'. Maybe, the loop counter 'i' also should be 'unsigned int' then?
On 09/04/2021 07:46, Masahiro Yamada wrote: > On Thu, Apr 8, 2021 at 12:25 AM Colin King <colin.king@canonical.com> wrote: >> >> From: Colin Ian King <colin.king@canonical.com> >> >> The for-loop iterates with a u8 loop counter i and compares this >> with the loop upper limit of num_parents that is an int type. >> There is a potential infinite loop if num_parents is larger than >> the u8 loop counter. Fix this by making the loop counter the same >> type as num_parents. >> >> Addresses-Coverity: ("Infinite loop") >> Fixes: 734d82f4a678 ("clk: uniphier: add core support code for UniPhier clock driver") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/uniphier/clk-uniphier-mux.c b/drivers/clk/uniphier/clk-uniphier-mux.c >> index 462c84321b2d..ce219e0d2a85 100644 >> --- a/drivers/clk/uniphier/clk-uniphier-mux.c >> +++ b/drivers/clk/uniphier/clk-uniphier-mux.c >> @@ -34,7 +34,7 @@ static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw) >> int num_parents = clk_hw_get_num_parents(hw); >> int ret; >> unsigned int val; >> - u8 i; >> + int i; >> >> ret = regmap_read(mux->regmap, mux->reg, &val); >> if (ret) >> -- >> 2.30.2 >> > > clk_hw_get_num_parents() returns 'unsigned int', so > I think 'num_parents' should also have been 'unsigned int'. > > Maybe, the loop counter 'i' also should be 'unsigned int' then? > > Good point. I'll send a V2.
On Fri, Apr 09, 2021 at 03:46:47PM +0900, Masahiro Yamada wrote: > On Thu, Apr 8, 2021 at 12:25 AM Colin King <colin.king@canonical.com> wrote: > > > > From: Colin Ian King <colin.king@canonical.com> > > > > The for-loop iterates with a u8 loop counter i and compares this > > with the loop upper limit of num_parents that is an int type. > > There is a potential infinite loop if num_parents is larger than > > the u8 loop counter. Fix this by making the loop counter the same > > type as num_parents. > > > > Addresses-Coverity: ("Infinite loop") > > Fixes: 734d82f4a678 ("clk: uniphier: add core support code for UniPhier clock driver") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/clk/uniphier/clk-uniphier-mux.c b/drivers/clk/uniphier/clk-uniphier-mux.c > > index 462c84321b2d..ce219e0d2a85 100644 > > --- a/drivers/clk/uniphier/clk-uniphier-mux.c > > +++ b/drivers/clk/uniphier/clk-uniphier-mux.c > > @@ -34,7 +34,7 @@ static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw) > > int num_parents = clk_hw_get_num_parents(hw); > > int ret; > > unsigned int val; > > - u8 i; > > + int i; > > > > ret = regmap_read(mux->regmap, mux->reg, &val); > > if (ret) > > -- > > 2.30.2 > > > > clk_hw_get_num_parents() returns 'unsigned int', so > I think 'num_parents' should also have been 'unsigned int'. > > Maybe, the loop counter 'i' also should be 'unsigned int' then? The clk_hw_get_num_parents() function returns 0-255 so the original code works fine. It should basically always be "int i;" That's the safest assumption. There are other case where it has to be size_t but in those cases I think people should call the list iterator something else instead of "i" like "size_t pg_idx;". Making everthing u32 causes more bugs than it prevents. Signedness bugs with comparing to zero, type promotion bugs, or subtraction bugs where subtracting wraps to a high value. It's rare to loop more than INT_MAX times in the kernel. When we do need to count about 2 million then we're probably not going to stop counting at 4 million, we're going to go to 10 million or higher so size_t is more appropriate than u32. Btw, if you have a loop that does: for (i = 0; i < UINT_MAX; i++) { that loop works exactly the same if "i" is an int or if it's a u32 because of type promotion. So you have to look really hard to find a place where changing a loop iterator from int to u32 fixes bug in real life. regards, dan carpenter
On Fri, Apr 16, 2021 at 3:19 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Fri, Apr 09, 2021 at 03:46:47PM +0900, Masahiro Yamada wrote: > > On Thu, Apr 8, 2021 at 12:25 AM Colin King <colin.king@canonical.com> wrote: > > > > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > The for-loop iterates with a u8 loop counter i and compares this > > > with the loop upper limit of num_parents that is an int type. > > > There is a potential infinite loop if num_parents is larger than > > > the u8 loop counter. Fix this by making the loop counter the same > > > type as num_parents. > > > > > > Addresses-Coverity: ("Infinite loop") > > > Fixes: 734d82f4a678 ("clk: uniphier: add core support code for UniPhier clock driver") > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > --- > > > drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/clk/uniphier/clk-uniphier-mux.c b/drivers/clk/uniphier/clk-uniphier-mux.c > > > index 462c84321b2d..ce219e0d2a85 100644 > > > --- a/drivers/clk/uniphier/clk-uniphier-mux.c > > > +++ b/drivers/clk/uniphier/clk-uniphier-mux.c > > > @@ -34,7 +34,7 @@ static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw) > > > int num_parents = clk_hw_get_num_parents(hw); > > > int ret; > > > unsigned int val; > > > - u8 i; > > > + int i; > > > > > > ret = regmap_read(mux->regmap, mux->reg, &val); > > > if (ret) > > > -- > > > 2.30.2 > > > > > > > clk_hw_get_num_parents() returns 'unsigned int', so > > I think 'num_parents' should also have been 'unsigned int'. > > > > Maybe, the loop counter 'i' also should be 'unsigned int' then? > > The clk_hw_get_num_parents() function returns 0-255 so the original code > works fine. True. clk->core->num_parents is u8, but it is not clear just by looking at the prototype of clk_hw_get_num_parents(). At least, it is not clear enough for tools, and actually Coverity raised a flag. Personally, I prefer 'unsigned int' (or 'int') when I count the number of something. Historically, the clk subsystem uses u8, (maybe to save memory??), and there exists distortion. For example, the return type of uniphier_clk_mux_get_parent() is u8, but it actually returns -EINVAL for error cases. So, u8 is not wide enough, in my opinion. > > It should basically always be "int i;" That's the safest assumption. > There are other case where it has to be size_t but in those cases I > think people should call the list iterator something else instead of "i" > like "size_t pg_idx;". > > Making everthing u32 causes more bugs than it prevents. Signedness bugs > with comparing to zero, type promotion bugs, or subtraction bugs where > subtracting wraps to a high value. It's rare to loop more than INT_MAX > times in the kernel. When we do need to count about 2 million then > we're probably not going to stop counting at 4 million, we're going to > go to 10 million or higher so size_t is more appropriate than u32. > > Btw, if you have a loop that does: > > for (i = 0; i < UINT_MAX; i++) { > > that loop works exactly the same if "i" is an int or if it's a u32 > because of type promotion. You are right. Perhaps, in hindsight, the following were natural: unsigned int num_parents = clk_hw_get_num_parents(hw); ... int i; I am fine with this if it is not too late. But, Stephen has already picked up this patch. > So you have to look really hard to find a > place where changing a loop iterator from int to u32 fixes bug in real > life. > > regards, > dan carpenter
diff --git a/drivers/clk/uniphier/clk-uniphier-mux.c b/drivers/clk/uniphier/clk-uniphier-mux.c index 462c84321b2d..ce219e0d2a85 100644 --- a/drivers/clk/uniphier/clk-uniphier-mux.c +++ b/drivers/clk/uniphier/clk-uniphier-mux.c @@ -34,7 +34,7 @@ static u8 uniphier_clk_mux_get_parent(struct clk_hw *hw) int num_parents = clk_hw_get_num_parents(hw); int ret; unsigned int val; - u8 i; + int i; ret = regmap_read(mux->regmap, mux->reg, &val); if (ret)