[PATCH] scsi_transport_sas: Write outside array bounds

[PATCH] scsi_transport_sas: Write outside array bounds

[Roel Kluin]

SETUP_PORT_ATTRIBUTE increments count, making the write out of bounds (array of
size 1)

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Credits to Parfait (http://research.sun.com/projects/parfait/)

I suspect this isn't the only location where count shouldn't be incremented,
Somebody should review this function.

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 0895d3c..c784ae4 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1693,9 +1693,10 @@ sas_attach_transport(struct sas_function_template *ft)
 
 	count = 0;
 	SETUP_PORT_ATTRIBUTE(num_phys);
-	i->host_attrs[count] = NULL;
 
 	count = 0;
+	i->host_attrs[count] = NULL;
+
 	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
 	SETUP_PHY_ATTRIBUTE(target_port_protocols);
 	SETUP_PHY_ATTRIBUTE(device_type);

Re: [PATCH] scsi_transport_sas: Write outside array bounds

[James Bottomley]

On Tue, 2009-07-28 at 12:20 +0200, Roel Kluin wrote:
> SETUP_PORT_ATTRIBUTE increments count, making the write out of bounds (array of
> size 1)
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Credits to Parfait (http://research.sun.com/projects/parfait/)
> 
> I suspect this isn't the only location where count shouldn't be incremented,
> Somebody should review this function.
> 
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 0895d3c..c784ae4 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1693,9 +1693,10 @@ sas_attach_transport(struct sas_function_template *ft)
>  
>  	count = 0;
>  	SETUP_PORT_ATTRIBUTE(num_phys);
> -	i->host_attrs[count] = NULL;
>  
>  	count = 0;
> +	i->host_attrs[count] = NULL;
> +
>  	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
>  	SETUP_PHY_ATTRIBUTE(target_port_protocols);
>  	SETUP_PHY_ATTRIBUTE(device_type);

If you're going to use mechanical checkers for this type of thing, it
would really help me if you'd actually think about what the code is
trying to do before sending a patch.

In this case, as you can see all of the SETUP_X are using count to step
through an array and when we're finished we add a NULL to the end.  For
the case of SETUP_PORT_ATTR, the array is i->port_attrs, so the bug here
is apparently that the NULL is going in the wrong array.  Trying to put
a NULL at the zero element is manifestly wrong because sas_internal is
zero allocated.

Actually, as a final weirdness, we have a duplicate initialisation of
this anyway, so the correct patch is below.

James

---

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 0895d3c..fd47cb1 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1692,10 +1692,6 @@ sas_attach_transport(struct sas_function_template *ft)
 	i->f = ft;
 
 	count = 0;
-	SETUP_PORT_ATTRIBUTE(num_phys);
-	i->host_attrs[count] = NULL;
-
-	count = 0;
 	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
 	SETUP_PHY_ATTRIBUTE(target_port_protocols);
 	SETUP_PHY_ATTRIBUTE(device_type);

Re: [PATCH] scsi_transport_sas: Write outside array bounds

[Jeff Garzik]

James Bottomley wrote:
> In this case, as you can see all of the SETUP_X are using count to step
> through an array and when we're finished we add a NULL to the end.  For
> the case of SETUP_PORT_ATTR, the array is i->port_attrs, so the bug here
> is apparently that the NULL is going in the wrong array.  Trying to put
> a NULL at the zero element is manifestly wrong because sas_internal is
> zero allocated.
> 
> Actually, as a final weirdness, we have a duplicate initialisation of
> this anyway, so the correct patch is below.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 0895d3c..fd47cb1 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1692,10 +1692,6 @@ sas_attach_transport(struct sas_function_template *ft)
>  	i->f = ft;
>  
>  	count = 0;
> -	SETUP_PORT_ATTRIBUTE(num_phys);
> -	i->host_attrs[count] = NULL;
> -
> -	count = 0;
>  	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
>  	SETUP_PHY_ATTRIBUTE(target_port_protocols);
>  	SETUP_PHY_ATTRIBUTE(device_type);

er, eh?

It's not a duplicate initialization, as port attributes != phy 
attributes.  Different macro, same counter variable.

Your fix seems quite strange - num_phy attribute goes away?

	Jeff

Re: [PATCH] scsi_transport_sas: Write outside array bounds

[James Bottomley]

On Tue, 2009-07-28 at 16:10 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > In this case, as you can see all of the SETUP_X are using count to step
> > through an array and when we're finished we add a NULL to the end.  For
> > the case of SETUP_PORT_ATTR, the array is i->port_attrs, so the bug here
> > is apparently that the NULL is going in the wrong array.  Trying to put
> > a NULL at the zero element is manifestly wrong because sas_internal is
> > zero allocated.
> > 
> > Actually, as a final weirdness, we have a duplicate initialisation of
> > this anyway, so the correct patch is below.
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> > index 0895d3c..fd47cb1 100644
> > --- a/drivers/scsi/scsi_transport_sas.c
> > +++ b/drivers/scsi/scsi_transport_sas.c
> > @@ -1692,10 +1692,6 @@ sas_attach_transport(struct sas_function_template *ft)
> >  	i->f = ft;
> >  
> >  	count = 0;
> > -	SETUP_PORT_ATTRIBUTE(num_phys);
> > -	i->host_attrs[count] = NULL;
> > -
> > -	count = 0;
> >  	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
> >  	SETUP_PHY_ATTRIBUTE(target_port_protocols);
> >  	SETUP_PHY_ATTRIBUTE(device_type);
> 
> er, eh?
> 
> It's not a duplicate initialization, as port attributes != phy 
> attributes.  Different macro, same counter variable.
> 
> Your fix seems quite strange - num_phy attribute goes away?

Look in the actual file.  About 20 lines down you'll find a duplicate
port setup for num_phys.

James

Re: [PATCH] scsi_transport_sas: Write outside array bounds

[Jeff Garzik]

James Bottomley wrote:
> On Tue, 2009-07-28 at 16:10 -0400, Jeff Garzik wrote:
>> James Bottomley wrote:
>>> In this case, as you can see all of the SETUP_X are using count to step
>>> through an array and when we're finished we add a NULL to the end.  For
>>> the case of SETUP_PORT_ATTR, the array is i->port_attrs, so the bug here
>>> is apparently that the NULL is going in the wrong array.  Trying to put
>>> a NULL at the zero element is manifestly wrong because sas_internal is
>>> zero allocated.
>>>
>>> Actually, as a final weirdness, we have a duplicate initialisation of
>>> this anyway, so the correct patch is below.
>>>
>>> James
>>>
>>> ---
>>>
>>> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
>>> index 0895d3c..fd47cb1 100644
>>> --- a/drivers/scsi/scsi_transport_sas.c
>>> +++ b/drivers/scsi/scsi_transport_sas.c
>>> @@ -1692,10 +1692,6 @@ sas_attach_transport(struct sas_function_template *ft)
>>>  	i->f = ft;
>>>  
>>>  	count = 0;
>>> -	SETUP_PORT_ATTRIBUTE(num_phys);
>>> -	i->host_attrs[count] = NULL;
>>> -
>>> -	count = 0;
>>>  	SETUP_PHY_ATTRIBUTE(initiator_port_protocols);
>>>  	SETUP_PHY_ATTRIBUTE(target_port_protocols);
>>>  	SETUP_PHY_ATTRIBUTE(device_type);
>> er, eh?
>>
>> It's not a duplicate initialization, as port attributes != phy 
>> attributes.  Different macro, same counter variable.
>>
>> Your fix seems quite strange - num_phy attribute goes away?
> 
> Look in the actual file.  About 20 lines down you'll find a duplicate
> port setup for num_phys.

Indeed... sorry for the noise!

	Jeff