Re: [PATCH] scsi_transport_sas: Write outside array bounds

James Bottomley
2009-07-28T16:39:10Z

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);