Skip to contentSkip to conversation

lpfc: Read buffer overflow

PATCH NAK Roel Kluin 5 participants 7 messages Aug 2, 2009 – Aug 4, 2009

Subsystems (2)
SCSI Emulex/Broadcom lpfc FC/FCoE SCSI driver
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 lpfc: Read buffer overflow

Check whether index is within bounds before testing the element.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/scsi/lpfc/lpfc_vport.c +1 -1
index e0b4992..ade2df6 100644
@@ -762,7 +762,7 @@ lpfc_destroy_vport_work_array(struct lpfc_hba *phba, struct lpfc_vport **vports)
 	int i;
 	if (vports == NULL)
 		return;
-	for (i = 0; vports[i] != NULL && i <= phba->max_vports; i++)
+	for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++)
 		scsi_host_put(lpfc_shost_from_vport(vports[i]));
 	kfree(vports);
 }
NAK
#1 lore raw

NACK - the vports array is created such that it is sized for phba->max_vports + 1.

-- james s

Quoted 19 lines · Roel Kluin

Check whether index is within bounds before testing the element.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/scsi/lpfc/lpfc_vport.c +1 -1
index e0b4992..ade2df6 100644
@@ -762,7 +762,7 @@ lpfc_destroy_vport_work_array(struct lpfc_hba *phba, struct lpfc_vport **vports)
 	int i;
 	if (vports == NULL)
 		return;
-	for (i = 0; vports[i] != NULL && i <= phba->max_vports; i++)
+	for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++)
 		scsi_host_put(lpfc_shost_from_vport(vports[i]));
 	kfree(vports);
 }
#2 lore raw
Quoted 1 line · James Smart

NACK - the vports array is created such that it is sized for phba->max_vports + 1.

It's not an off-by-one problem, it's a look-before-leaping problem.

Regards,
Joe

Quoted 28 lines

-- james s

Quoted 20 lines · Roel Kluin

Check whether index is within bounds before testing the element.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/scsi/lpfc/lpfc_vport.c +1 -1
index e0b4992..ade2df6 100644
@@ -762,7 +762,7 @@ lpfc_destroy_vport_work_array(struct lpfc_hba 
 *phba, struct lpfc_vport **vports)
     int i;
     if (vports == NULL)
         return;
-    for (i = 0; vports[i] != NULL && i <= phba->max_vports; i++)
+    for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++)
         scsi_host_put(lpfc_shost_from_vport(vports[i]));
     kfree(vports);

}

Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#4 lore raw
In reply to James Smart
Quoted 13 lines · On Mon, 3 Aug 2009 11:02:37 -0400 James Smart <[email protected]> wrote:
Quoted 12 lines · Roel Kluin

Check whether index is within bounds before testing the element.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/scsi/lpfc/lpfc_vport.c +0 -0
index e0b4992..ade2df6 100644
@@ -762,7 +762,7 @@ lpfc_destroy_vport_work_array(struct lpfc_hba *phba, struct lpfc_vport **vports)
 	int i;
 	if (vports == NULL)
 		return;
Quoted 8 lines

drivers/scsi/lpfc/lpfc_vport.c lpfc_destroy_vport_work_array(struct lpfc_hba *phba, struct lpfc_vport **vports) · line 768

Quoted 5 lines
drivers/scsi/lpfc/lpfc_vport.c +1 -1
-	for (i = 0; vports[i] != NULL && i <= phba->max_vports; i++)
+	for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++)
 		scsi_host_put(lpfc_shost_from_vport(vports[i]));
 	kfree(vports);
 }

NACK - the vports array is created such that it is sized for phba->max_vports + 1.

(top-posting repaired so that I can feasibly reply to the email, dammit)

There's no need to allocate the extra slot in the vports array if we're also retaining its size. I'd suggest that we merge Roel's patch and then reduce the size of vports[].

What prevents the loop in lpfc_create_vport_work_array() from wandering off the end of vports[], btw?

maintainer
#5 lore raw
Quoted 25 lines · Andrew Morton
Quoted 13 lines · On Mon, 3 Aug 2009 11:02:37 -0400 James Smart <[email protected]> wrote:
Quoted 12 lines · Roel Kluin

Check whether index is within bounds before testing the element.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/scsi/lpfc/lpfc_vport.c +0 -0
index e0b4992..ade2df6 100644
@@ -762,7 +762,7 @@ lpfc_destroy_vport_work_array(struct lpfc_hba *phba, struct lpfc_vport **vports)
 	int i;
 	if (vports == NULL)
 		return;
Quoted 8 lines

drivers/scsi/lpfc/lpfc_vport.c lpfc_destroy_vport_work_array(struct lpfc_hba *phba, struct lpfc_vport **vports) · line 768

Quoted 5 lines
drivers/scsi/lpfc/lpfc_vport.c +1 -1
-	for (i = 0; vports[i] != NULL && i <= phba->max_vports; i++)
+	for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++)
 		scsi_host_put(lpfc_shost_from_vport(vports[i]));
 	kfree(vports);
 }

NACK - the vports array is created such that it is sized for phba->max_vports + 1.

Quoted 5 lines

(top-posting repaired so that I can feasibly reply to the email, dammit)

There's no need to allocate the extra slot in the vports array if we're also retaining its size. I'd suggest that we merge Roel's patch and then reduce the size of vports[].

I'd suggest not:

jejb@mulgrave:~/git/linux-2.6/drivers/scsi/lpfc> git grep 'max_vports && vports'|wc -l 17

Assuming this secures agreement as a patch, I don't really want to wade through another 16 patches for the remaining ones. You definitely can't reduce the vport array length until they're all fixed.

What I hear is that it's not a problem, but it's a possible micro-optimisation. Assuming the maintainer agrees, it's far better just to fix everything at once. However, I'm equally happy to leave it as is since there's no actual problem given the allocation definition.

Quoted 2 lines

What prevents the loop in lpfc_create_vport_work_array() from wandering off the end of vports[], btw?

We refuse to create any vports beyond this number ... the iterator counts the number of created vports (it's actually a firmware limited number, I believe).

James

#6 lore raw
Quoted 14 lines · James Bottomley

I'd suggest not:

jejb@mulgrave:~/git/linux-2.6/drivers/scsi/lpfc> git grep 'max_vports && vports'|wc -l 17

Assuming this secures agreement as a patch, I don't really want to wade through another 16 patches for the remaining ones. You definitely can't reduce the vport array length until they're all fixed.

What I hear is that it's not a problem, but it's a possible micro-optimisation. Assuming the maintainer agrees, it's far better just to fix everything at once. However, I'm equally happy to leave it as is since there's no actual problem given the allocation definition.

Quoted 8 lines
Quoted 2 lines

What prevents the loop in lpfc_create_vport_work_array() from wandering off the end of vports[], btw?

We refuse to create any vports beyond this number ... the iterator counts the number of created vports (it's actually a firmware limited number, I believe).

James

Agree. It's a style thing from one of the contributors to the driver. There is no issue, I don't believe that optimizing 1 less pointer's worth of allocation is a big deal (good, yes, but...). However, I do recognize it is a much better coding style.

I'll roll the changes for it into the next driver patch set. (Yes, it is a firmware limited number).

-- james s

#3 lore raw
In reply to Joe Eykholt

Ok - it's good practice. But really, it's change for change sake, rather than fixing any problem.

-- james s

Quoted 37 lines · Joe Eykholt
Quoted 1 line · James Smart

NACK - the vports array is created such that it is sized for phba->max_vports + 1.

It's not an off-by-one problem, it's a look-before-leaping problem.

Regards,
Joe

Quoted 27 lines

-- james s

Quoted 20 lines · Roel Kluin

Check whether index is within bounds before testing the element.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/scsi/lpfc/lpfc_vport.c +1 -1
index e0b4992..ade2df6 100644
@@ -762,7 +762,7 @@ lpfc_destroy_vport_work_array(struct lpfc_hba 
 *phba, struct lpfc_vport **vports)
     int i;
     if (vports == NULL)
         return;
-    for (i = 0; vports[i] != NULL && i <= phba->max_vports; i++)
+    for (i = 0; i <= phba->max_vports && vports[i] != NULL; i++)
         scsi_host_put(lpfc_shost_from_vport(vports[i]));
     kfree(vports);

}

Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html