[PATCH] power: pfuze100: Ensure loop index is incremented

[PATCH] power: pfuze100: Ensure loop index is incremented

[Andrew Goodbody]

The for loop in se_desc uses i as the loop index and also to cause the
loop to end if the passed in name is not found. However i is not
incremented which could cause the loop to continue indefinitely and
access out of bounds memory.
Add an increment of i to ensure that the loop terminates correctly in
the case where name is not found.

This issue found by Smatch.

Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
 drivers/power/regulator/pfuze100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/regulator/pfuze100.c b/drivers/power/regulator/pfuze100.c
index bf3a7019411..eff166b368b 100644
--- a/drivers/power/regulator/pfuze100.c
+++ b/drivers/power/regulator/pfuze100.c
@@ -247,7 +247,7 @@ static struct pfuze100_regulator_desc *se_desc(struct pfuze100_regulator_desc *d
 {
 	int i;
 
-	for (i = 0; i < size; desc++) {
+	for (i = 0; i < size; i++, desc++) {
 		if (!strcmp(desc->name, name))
 			return desc;
 		continue;

---
base-commit: 7027b445cc0bfb86204ecb1f1fe596f5895048d9
change-id: 20250703-pfuze100_fix-4a0a1c3084cf

Best regards,
-- 
Andrew Goodbody <andrew.goodbody@linaro.org>

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Quentin Schulz]

Hi Andrew,

On 7/3/25 1:31 PM, Andrew Goodbody wrote:
> The for loop in se_desc uses i as the loop index and also to cause the
> loop to end if the passed in name is not found. However i is not
> incremented which could cause the loop to continue indefinitely and
> access out of bounds memory.
> Add an increment of i to ensure that the loop terminates correctly in
> the case where name is not found.
> 
> This issue found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Andrew Goodbody]

ping?

On 03/07/2025 12:31, Andrew Goodbody wrote:
> The for loop in se_desc uses i as the loop index and also to cause the
> loop to end if the passed in name is not found. However i is not
> incremented which could cause the loop to continue indefinitely and
> access out of bounds memory.
> Add an increment of i to ensure that the loop terminates correctly in
> the case where name is not found.
> 
> This issue found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>   drivers/power/regulator/pfuze100.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/regulator/pfuze100.c b/drivers/power/regulator/pfuze100.c
> index bf3a7019411..eff166b368b 100644
> --- a/drivers/power/regulator/pfuze100.c
> +++ b/drivers/power/regulator/pfuze100.c
> @@ -247,7 +247,7 @@ static struct pfuze100_regulator_desc *se_desc(struct pfuze100_regulator_desc *d
>   {
>   	int i;
>   
> -	for (i = 0; i < size; desc++) {
> +	for (i = 0; i < size; i++, desc++) {
>   		if (!strcmp(desc->name, name))
>   			return desc;
>   		continue;
> 
> ---
> base-commit: 7027b445cc0bfb86204ecb1f1fe596f5895048d9
> change-id: 20250703-pfuze100_fix-4a0a1c3084cf
> 
> Best regards,

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Andrew Goodbody]

On 14/07/2025 13:25, Quentin Schulz wrote:
> Hi Andrew,
> 
> On 7/3/25 1:31 PM, Andrew Goodbody wrote:
>> The for loop in se_desc uses i as the loop index and also to cause the
>> loop to end if the passed in name is not found. However i is not
>> incremented which could cause the loop to continue indefinitely and
>> access out of bounds memory.
>> Add an increment of i to ensure that the loop terminates correctly in
>> the case where name is not found.
>>
>> This issue found by Smatch.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin

Is there anything else needed before this can be merged please?

Thanks,
Andrew

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Thu, Jul 03, 2025 at 12:31:50PM +0100, Andrew Goodbody wrote:

> The for loop in se_desc uses i as the loop index and also to cause the
> loop to end if the passed in name is not found. However i is not
> incremented which could cause the loop to continue indefinitely and
> access out of bounds memory.
> Add an increment of i to ensure that the loop terminates correctly in
> the case where name is not found.
> 
> This issue found by Smatch.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
>  drivers/power/regulator/pfuze100.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I size tested this as part of merging and saw unexpected shrinkage. In
turn, this got me to look harder at the code and I think the best answer
is to refactor things so that se_desc(...) follow the normal (linux
kernel) pattern of for (i = 0; i < ARRAY_SIZE(desc); i++) instead of
being passed size. That's I think the root of this confusion too. I'll
post a patch shortly.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

On Sun, Aug 31, 2025 at 09:35:13AM -0600, Tom Rini wrote:
> On Thu, Jul 03, 2025 at 12:31:50PM +0100, Andrew Goodbody wrote:
> 
> > The for loop in se_desc uses i as the loop index and also to cause the
> > loop to end if the passed in name is not found. However i is not
> > incremented which could cause the loop to continue indefinitely and
> > access out of bounds memory.
> > Add an increment of i to ensure that the loop terminates correctly in
> > the case where name is not found.
> > 
> > This issue found by Smatch.
> > 
> > Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> > ---
> >  drivers/power/regulator/pfuze100.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I size tested this as part of merging and saw unexpected shrinkage. In
> turn, this got me to look harder at the code and I think the best answer
> is to refactor things so that se_desc(...) follow the normal (linux
> kernel) pattern of for (i = 0; i < ARRAY_SIZE(desc); i++) instead of
> being passed size. That's I think the root of this confusion too. I'll
> post a patch shortly.

While I really wanted to make this suggested change, I'm just missing
something as to how it should work, and perhaps the better answer is to
rework the caller a bit to handle the check inline? I'm not sure...

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Andrew Goodbody]

On 13/09/2025 16:51, Tom Rini wrote:
> On Sun, Aug 31, 2025 at 09:35:13AM -0600, Tom Rini wrote:
>> On Thu, Jul 03, 2025 at 12:31:50PM +0100, Andrew Goodbody wrote:
>>
>>> The for loop in se_desc uses i as the loop index and also to cause the
>>> loop to end if the passed in name is not found. However i is not
>>> incremented which could cause the loop to continue indefinitely and
>>> access out of bounds memory.
>>> Add an increment of i to ensure that the loop terminates correctly in
>>> the case where name is not found.
>>>
>>> This issue found by Smatch.
>>>
>>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>>> ---
>>>   drivers/power/regulator/pfuze100.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> I size tested this as part of merging and saw unexpected shrinkage. In
>> turn, this got me to look harder at the code and I think the best answer
>> is to refactor things so that se_desc(...) follow the normal (linux
>> kernel) pattern of for (i = 0; i < ARRAY_SIZE(desc); i++) instead of
>> being passed size. That's I think the root of this confusion too. I'll
>> post a patch shortly.
> 
> While I really wanted to make this suggested change, I'm just missing
> something as to how it should work, and perhaps the better answer is to
> rework the caller a bit to handle the check inline? I'm not sure...

Sorry Tom, I am just not sure if this is an action item on me or are you 
still looking at it? I do not know the code well but could take a look 
at it if needed.

Thanks,
Andrew

Re: [PATCH] power: pfuze100: Ensure loop index is incremented

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

On Mon, Sep 15, 2025 at 10:45:56AM +0100, Andrew Goodbody wrote:
> On 13/09/2025 16:51, Tom Rini wrote:
> > On Sun, Aug 31, 2025 at 09:35:13AM -0600, Tom Rini wrote:
> > > On Thu, Jul 03, 2025 at 12:31:50PM +0100, Andrew Goodbody wrote:
> > > 
> > > > The for loop in se_desc uses i as the loop index and also to cause the
> > > > loop to end if the passed in name is not found. However i is not
> > > > incremented which could cause the loop to continue indefinitely and
> > > > access out of bounds memory.
> > > > Add an increment of i to ensure that the loop terminates correctly in
> > > > the case where name is not found.
> > > > 
> > > > This issue found by Smatch.
> > > > 
> > > > Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> > > > ---
> > > >   drivers/power/regulator/pfuze100.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > I size tested this as part of merging and saw unexpected shrinkage. In
> > > turn, this got me to look harder at the code and I think the best answer
> > > is to refactor things so that se_desc(...) follow the normal (linux
> > > kernel) pattern of for (i = 0; i < ARRAY_SIZE(desc); i++) instead of
> > > being passed size. That's I think the root of this confusion too. I'll
> > > post a patch shortly.
> > 
> > While I really wanted to make this suggested change, I'm just missing
> > something as to how it should work, and perhaps the better answer is to
> > rework the caller a bit to handle the check inline? I'm not sure...
> 
> Sorry Tom, I am just not sure if this is an action item on me or are you
> still looking at it? I do not know the code well but could take a look at it
> if needed.

Sorry for being unclear. The original patch isn't right I think, the
size change leads me to believe that we're changing the loop behavior.
Looking harder at the code in question, it seems like it's an odd way to
iterate over every element in the array ('i' isn't used, we just
increment desc and quite possibly don't handle failure to find a match
correctly?). I think the best path is reworking the code a bit, but no,
I don't have the time to. If you don't have time to, perhaps Fabio or
Peng can?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH] power: pfuze100: Ensure loop index is incremented

[Peng Fan]

> Subject: Re: [PATCH] power: pfuze100: Ensure loop index is
> incremented
> 
> Sorry for being unclear. The original patch isn't right I think, the size
> change leads me to believe that we're changing the loop behavior.
> Looking harder at the code in question, it seems like it's an odd way to
> iterate over every element in the array ('i' isn't used, we just increment
> desc and quite possibly don't handle failure to find a match correctly?).
> I think the best path is reworking the code a bit, but no, I don't have
> the time to. If you don't have time to, perhaps Fabio or Peng can?

I just post a patch, please see whether it is good for you.

Thanks,
Peng.

> 
> --
> Tom