Skip to contentSkip to conversation

ata: pata_hpt37x: fix potential forever loop

PATCH In review Dan Carpenter 2 participants 4 messages Feb 1, 2023 – Feb 2, 2023

Subsystems (2)
Kernel janitors libata
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ata: pata_hpt37x: fix potential forever loop

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 <[email protected]>
---
 drivers/ata/pata_hpt37x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt37x.c +2 -2
index ce3c5eaa7e76..35be9a095b18 100644
@@ -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)
Signature · 1 line
2.35.1
Reviewed-by on v1 1/1
#1 lore raw

Hello!

Quoted 21 lines · 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 <[email protected]>
---
 drivers/ata/pata_hpt37x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt37x.c +1 -1
index ce3c5eaa7e76..35be9a095b18 100644
@@ -621,14 +621,14 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
 {
 	u8 reg5b;
 	u32 reg5c;
-	int tries;
+	int tries, tries2;

drivers/ata/pata_hpt37x.c static int hpt37x_calibrate_dpll(struct pci_dev *dev) · line 634

Quoted 10 lines
drivers/ata/pata_hpt37x.c +1 -1
 	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 <[email protected]>

MBR, Sergey

#2 lore raw

On 2/1/23 5:59 PM, Sergey Shtylyov wrote:
[...]

Quoted 3 lines
Quoted 3 lines

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?

Quoted 18 lines
Quoted 18 lines
Fixes 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/ata/pata_hpt37x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt37x.c +1 -1
index ce3c5eaa7e76..35be9a095b18 100644
@@ -621,14 +621,14 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
 {
 	u8 reg5b;
 	u32 reg5c;
-	int tries;
+	int tries, tries2;
Quoted 14 lines

drivers/ata/pata_hpt37x.c static int hpt37x_calibrate_dpll(struct pci_dev *dev) · line 634

Quoted 10 lines
drivers/ata/pata_hpt37x.c +1 -1
 	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...

Quoted 1 line
Reviewed-by Sergey Shtylyov <[email protected]>

So I have to take this back...

MBR, Sergey

author
#3 lore raw
Quoted 8 lines · Sergey Shtylyov

On 2/1/23 5:59 PM, Sergey Shtylyov wrote:
[...]

Quoted 3 lines
Quoted 3 lines

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