[PATCH] lpfc: Read buffer overflow

[PATCH] lpfc: Read buffer overflow

[Roel Kluin]

Check whether index is within bounds before testing the element.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/scsi/lpfc/lpfc_vport.c b/drivers/scsi/lpfc/lpfc_vport.c
index e0b4992..ade2df6 100644
--- a/drivers/scsi/lpfc/lpfc_vport.c
+++ b/drivers/scsi/lpfc/lpfc_vport.c
@@ -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);
 }

Re: [PATCH] lpfc: Read buffer overflow

[James Smart]

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

-- james s

Roel Kluin wrote:
> Check whether index is within bounds before testing the element.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/scsi/lpfc/lpfc_vport.c b/drivers/scsi/lpfc/lpfc_vport.c
> index e0b4992..ade2df6 100644
> --- a/drivers/scsi/lpfc/lpfc_vport.c
> +++ b/drivers/scsi/lpfc/lpfc_vport.c
> @@ -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);
>  }
>
>   

Re: [PATCH] lpfc: Read buffer overflow

[Joe Eykholt]

James Smart wrote:
> 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

> 
> -- james s
> 
> Roel Kluin wrote:
>> Check whether index is within bounds before testing the element.
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> diff --git a/drivers/scsi/lpfc/lpfc_vport.c 
>> b/drivers/scsi/lpfc/lpfc_vport.c
>> index e0b4992..ade2df6 100644
>> --- a/drivers/scsi/lpfc/lpfc_vport.c
>> +++ b/drivers/scsi/lpfc/lpfc_vport.c
>> @@ -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);
>>  }
>>
>>   
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lpfc: Read buffer overflow

[James Smart]

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

-- james s

Joe Eykholt wrote:
> James Smart wrote:
>> 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
> 
>> -- james s
>>
>> Roel Kluin wrote:
>>> Check whether index is within bounds before testing the element.
>>>
>>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>>> ---
>>> diff --git a/drivers/scsi/lpfc/lpfc_vport.c 
>>> b/drivers/scsi/lpfc/lpfc_vport.c
>>> index e0b4992..ade2df6 100644
>>> --- a/drivers/scsi/lpfc/lpfc_vport.c
>>> +++ b/drivers/scsi/lpfc/lpfc_vport.c
>>> @@ -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);
>>>  }
>>>
>>>   
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] lpfc: Read buffer overflow

[Andrew Morton]

On Mon, 3 Aug 2009 11:02:37 -0400
James Smart <James.Smart@Emulex.Com> wrote:

> Roel Kluin wrote:
> > Check whether index is within bounds before testing the element.
> >
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > diff --git a/drivers/scsi/lpfc/lpfc_vport.c b/drivers/scsi/lpfc/lpfc_vport.c
> > index e0b4992..ade2df6 100644
> > --- a/drivers/scsi/lpfc/lpfc_vport.c
> > +++ b/drivers/scsi/lpfc/lpfc_vport.c
> > @@ -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);
> >  }
>
> 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?

Re: [PATCH] lpfc: Read buffer overflow

[James Bottomley]

On Mon, 2009-08-03 at 16:42 -0700, Andrew Morton wrote:
> On Mon, 3 Aug 2009 11:02:37 -0400
> James Smart <James.Smart@Emulex.Com> wrote:
> 
> > Roel Kluin wrote:
> > > Check whether index is within bounds before testing the element.
> > >
> > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > > ---
> > > diff --git a/drivers/scsi/lpfc/lpfc_vport.c b/drivers/scsi/lpfc/lpfc_vport.c
> > > index e0b4992..ade2df6 100644
> > > --- a/drivers/scsi/lpfc/lpfc_vport.c
> > > +++ b/drivers/scsi/lpfc/lpfc_vport.c
> > > @@ -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);
> > >  }
> >
> > 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[].

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.

> 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

Re: [PATCH] lpfc: Read buffer overflow

[James Smart]

James Bottomley wrote:
> 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.
> 
>> 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