[PATCH] ata: pata_hpt37x: fix potential forever loop

[PATCH] ata: pata_hpt37x: fix potential forever loop

[Dan Carpenter]

This code accidentally reuses the "tries" iterator for both the inside
and outside loops.  It could result in a forever loop if the "tries"
variable gets reset to 0x1000 and never reaches 0x5000.

Fixes: 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
 drivers/ata/pata_hpt37x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
index ce3c5eaa7e76..35be9a095b18 100644
--- a/drivers/ata/pata_hpt37x.c
+++ b/drivers/ata/pata_hpt37x.c
@@ -621,14 +621,14 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
 {
 	u8 reg5b;
 	u32 reg5c;
-	int tries;
+	int tries, tries2;
 
 	for (tries = 0; tries < 0x5000; tries++) {
 		udelay(50);
 		pci_read_config_byte(dev, 0x5b, &reg5b);
 		if (reg5b & 0x80) {
 			/* See if it stays set */
-			for (tries = 0; tries < 0x1000; tries++) {
+			for (tries2 = 0; tries2 < 0x1000; tries2++) {
 				pci_read_config_byte(dev, 0x5b, &reg5b);
 				/* Failed ? */
 				if ((reg5b & 0x80) == 0)
-- 
2.35.1

Re: [PATCH] ata: pata_hpt37x: fix potential forever loop

[Sergey Shtylyov]

Hello!

On 2/1/23 5:10 PM, Dan Carpenter wrote:

> This code accidentally reuses the "tries" iterator for both the inside
> and outside loops.  It could result in a forever loop if the "tries"
> variable gets reset to 0x1000 and never reaches 0x5000.
> 
> Fixes: 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
>  drivers/ata/pata_hpt37x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
> index ce3c5eaa7e76..35be9a095b18 100644
> --- a/drivers/ata/pata_hpt37x.c
> +++ b/drivers/ata/pata_hpt37x.c
> @@ -621,14 +621,14 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
>  {
>  	u8 reg5b;
>  	u32 reg5c;
> -	int tries;
> +	int tries, tries2;
>  
>  	for (tries = 0; tries < 0x5000; tries++) {
>  		udelay(50);
>  		pci_read_config_byte(dev, 0x5b, &reg5b);
>  		if (reg5b & 0x80) {
>  			/* See if it stays set */
> -			for (tries = 0; tries < 0x1000; tries++) {
> +			for (tries2 = 0; tries2 < 0x1000; tries2++) {
>  				pci_read_config_byte(dev, 0x5b, &reg5b);
>  				/* Failed ? */
>  				if ((reg5b & 0x80) == 0)

   Looks like Alan tried to "fix" the DPLL calibration code imported from drivers/ide/hpt366.c
and failed at that... :-) The iriginal code has 2 sequential loops (thus 'tries' could be used
for both). In principle, I agree with a simplistic patch:

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

MBR, Sergey

Re: [PATCH] ata: pata_hpt37x: fix potential forever loop

[Sergey Shtylyov]

On 2/1/23 5:59 PM, Sergey Shtylyov wrote:
[...]
>> This code accidentally reuses the "tries" iterator for both the inside
>> and outside loops.  It could result in a forever loop if the "tries"
>> variable gets reset to 0x1000 and never reaches 0x5000.

   Have you actually seen this happening?

>>
>> Fixes: 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>> ---
>>  drivers/ata/pata_hpt37x.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
>> index ce3c5eaa7e76..35be9a095b18 100644
>> --- a/drivers/ata/pata_hpt37x.c
>> +++ b/drivers/ata/pata_hpt37x.c
>> @@ -621,14 +621,14 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
>>  {
>>  	u8 reg5b;
>>  	u32 reg5c;
>> -	int tries;
>> +	int tries, tries2;
>>  
>>  	for (tries = 0; tries < 0x5000; tries++) {
>>  		udelay(50);
>>  		pci_read_config_byte(dev, 0x5b, &reg5b);
>>  		if (reg5b & 0x80) {
>>  			/* See if it stays set */
>> -			for (tries = 0; tries < 0x1000; tries++) {
>> +			for (tries2 = 0; tries2 < 0x1000; tries2++) {
>>  				pci_read_config_byte(dev, 0x5b, &reg5b);
>>  				/* Failed ? */
>>  				if ((reg5b & 0x80) == 0)
> 
>    Looks like Alan tried to "fix" the DPLL calibration code imported from drivers/ide/hpt366.c
> and failed at that... :-) The iriginal code has 2 sequential loops (thus 'tries' could be used
> for both). In principle, I agree with a simplistic patch:

   Err, wait! Once we have bit 7 of the PCI register at 0x5b set, we'll never
return to the outer loop... So, I'm ot seeing the bug here...

> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

   So I have to take this back...

MBR, Sergey

Re: [PATCH] ata: pata_hpt37x: fix potential forever loop

[Dan Carpenter]

On Wed, Feb 01, 2023 at 08:18:13PM +0300, Sergey Shtylyov wrote:
> On 2/1/23 5:59 PM, Sergey Shtylyov wrote:
> [...]
> >> This code accidentally reuses the "tries" iterator for both the inside
> >> and outside loops.  It could result in a forever loop if the "tries"
> >> variable gets reset to 0x1000 and never reaches 0x5000.
> 
>    Have you actually seen this happening?
> 

No, this is from static analysis.

drivers/ata/pata_hpt3x2n.c:390 hpt3xn_calibrate_dpll() warn: reusing outside iterator: 'tries'

You're right that this is a false positive.  I thought I had addressed
that particular type of false positive so the check wouldn't warn about
it but apparently I hadn't.  Sorry!

Thanks again for reviewing this.

regards,
dan carpenter