Re: [PATCH] lpfc: Read buffer overflow

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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