Skip to contentSkip to conversation

ata: pata_hpt3x2n: prevent potential forever loop in hpt3xn_calibrate_dpll()

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

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

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ata: pata_hpt3x2n: prevent potential forever loop in hpt3xn_calibrate_dpll()

This code accidentally reuses "tries" as the iterator for both the inside and outside loops. It means that the potentially the "tries" could be reset to 0x1000 and never reach 0x5000.

Fixes 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/ata/pata_hpt3x2n.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt3x2n.c +2 -2
index 617c95522f43..447dc287a2d4 100644
@@ -380,14 +380,14 @@ static int hpt3xn_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
Quoted 6 lines · Dan Carpenter

This code accidentally reuses "tries" as the iterator for both the inside and outside loops. It means that the potentially the "tries" could be reset to 0x1000 and never reach 0x5000.

Fixes 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
Signed-off-by Dan Carpenter <[email protected]>

[...]

Again, I'm OK with this simplistic fix:

Reviewed-by Sergey Shtylyov <[email protected]>

[...]

MBR, Sergey

#2 lore raw
In reply to Dan Carpenter
Quoted 21 lines · Dan Carpenter

This code accidentally reuses "tries" as the iterator for both the inside and outside loops. It means that the potentially the "tries" could be reset to 0x1000 and never reach 0x5000.

Fixes 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/ata/pata_hpt3x2n.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt3x2n.c +1 -1
index 617c95522f43..447dc287a2d4 100644
@@ -380,14 +380,14 @@ static int hpt3xn_calibrate_dpll(struct pci_dev *dev)
 {
 	u8 reg5b;
 	u32 reg5c;
-	int tries;
+	int tries, tries2;

drivers/ata/pata_hpt3x2n.c static int hpt3xn_calibrate_dpll(struct pci_dev *dev) · line 393

Quoted 10 lines
drivers/ata/pata_hpt3x2n.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)

I am assuming this one is the same as for pata_hpt37x: a false positive ?

Signature · 2 lines
Damien Le Moal
Western Digital Research
author
#3 lore raw
Quoted 22 lines · Damien Le Moal
Quoted 21 lines · Dan Carpenter

This code accidentally reuses "tries" as the iterator for both the inside and outside loops. It means that the potentially the "tries" could be reset to 0x1000 and never reach 0x5000.

Fixes 669a5db411d8 ("[libata] Add a bunch of PATA drivers.")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/ata/pata_hpt3x2n.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ata/pata_hpt3x2n.c +1 -1
index 617c95522f43..447dc287a2d4 100644
@@ -380,14 +380,14 @@ static int hpt3xn_calibrate_dpll(struct pci_dev *dev)
 {
 	u8 reg5b;
 	u32 reg5c;
-	int tries;
+	int tries, tries2;
Quoted 12 lines

drivers/ata/pata_hpt3x2n.c static int hpt3xn_calibrate_dpll(struct pci_dev *dev) · line 393

Quoted 10 lines
drivers/ata/pata_hpt3x2n.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)

I am assuming this one is the same as for pata_hpt37x: a false positive ?

Yes, sorry.

regards,
dan carpenter