scsi_transport_sas: Write outside array bounds

PATCH In review Roel Kluin 3 participants 5 messages Jul 28, 2009 – Jul 29, 2009 SCSI

lore

Share & tools

lore

Roel Kluin

author

2009-07-28 10:17 UTC #0 lore raw

PATCH 1/1 scsi_transport_sas: Write outside array bounds

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

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

---
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.

drivers/scsi/scsi_transport_sas.c +2 -1

index 0895d3c..c784ae4 100644
@@ -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);

James Bottomley

maintainer

2009-07-28 16:39 UTC #1 lore raw

Quoted 16 lines · Roel Kluin

SETUP_PORT_ATTRIBUTE increments count, making the write out of bounds (array of size 1)
Signed-off-by Roel Kluin <[email protected]>
---
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.
drivers/scsi/scsi_transport_sas.c +0 -0
index 0895d3c..c784ae4 100644
@@ -1693,9 +1693,10 @@ sas_attach_transport(struct sas_function_template *ft)
 

drivers/scsi/scsi_transport_sas.c sas_attach_transport(struct sas_function_template *ft) · line 1702

Quoted 10 lines

drivers/scsi/scsi_transport_sas.c +2 -1

 	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

---

drivers/scsi/scsi_transport_sas.c +0 -4

index 0895d3c..fd47cb1 100644
@@ -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);

Jeff Garzik

2009-07-28 20:10 UTC #2 lore raw

Quoted 19 lines · James Bottomley

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
---
drivers/scsi/scsi_transport_sas.c +0 -0
index 0895d3c..fd47cb1 100644

drivers/scsi/scsi_transport_sas.c sas_attach_transport(struct sas_function_template *ft) · line 1697

Quoted 10 lines

drivers/scsi/scsi_transport_sas.c +0 -4

@@ -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

James Bottomley

maintainer

2009-07-28 21:31 UTC #3 lore raw

Quoted 33 lines · Jeff Garzik

Quoted 19 lines · James Bottomley
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
---
drivers/scsi/scsi_transport_sas.c +0 -0
index 0895d3c..fd47cb1 100644
drivers/scsi/scsi_transport_sas.c sas_attach_transport(struct sas_function_template *ft) · line 1697
Quoted 10 lines
drivers/scsi/scsi_transport_sas.c +0 -4
@@ -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?

Quoted 4 lines

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

Jeff Garzik

2009-07-29 02:50 UTC #4 lore raw

Quoted 33 lines · James Bottomley

Quoted 32 lines · Jeff Garzik
Quoted 19 lines · James Bottomley
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
---
drivers/scsi/scsi_transport_sas.c +0 -0
index 0895d3c..fd47cb1 100644
drivers/scsi/scsi_transport_sas.c sas_attach_transport(struct sas_function_template *ft) · line 1697
Quoted 10 lines
drivers/scsi/scsi_transport_sas.c +0 -4
@@ -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?

Quoted 6 lines

Quoted 3 lines
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