Skip to contentSkip to conversation

ses: tighten range checks in ses_intf_add()

PATCH In review Dan Carpenter 3 participants 4 messages Oct 19, 2015 – Nov 5, 2015 1 message not in the archive SCSI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): ----An------QYmAn$10b010ca-f710-44aa-8ea3-3b65a3c21286@alibaba-inc.com

Dan Carpenter

author
#1 lore raw

PATCH 1/1 ses: tighten range checks in ses_intf_add()

We test that "type_ptr" is within the buffer but then we read from "type_ptr[3]" so we could be reading beyond the end of the buffer.

Reported-by "Berry Cheng 程君(成淼)" <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

---
This isn't a complete fix because we still need more range checking in all the other places which use type_ptr like ses_get_page2_descriptor(). We record len as page1_len but we don't use it anywhere...

I wonder if someone knew the expected format we could make reject too short lengths earlier.

drivers/scsi/ses.c +2 -2
index dcb0d76..39f69b0 100644
@@ -641,7 +641,7 @@ static int ses_intf_add(struct device *cdev,
 	/* begin at the enclosure descriptor */
 	type_ptr = buf + 8;
 	/* skip all the enclosure descriptors */
-	for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) {
+	for (i = 0; i < num_enclosures && type_ptr + 4 < buf + len; i++) {
 		types += type_ptr[2];
 		type_ptr += type_ptr[3] + 4;
 	}

@@ -649,7 +649,7 @@ static int ses_intf_add(struct device *cdev,
 	ses_dev->page1_types = type_ptr;
 	ses_dev->page1_num_types = types;
 
-	for (i = 0; i < types && type_ptr < buf + len; i++, type_ptr += 4) {
+	for (i = 0; i < types && type_ptr + 2 < buf + len; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 			components += type_ptr[1];

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw

Hi Dan,

Quoted 18 lines · Dan Carpenter

We test that "type_ptr" is within the buffer but then we read from "type_ptr[3]" so we could be reading beyond the end of the buffer.

Reported-by "Berry Cheng ??????(??????)" <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

---
This isn't a complete fix because we still need more range checking in all the other places which use type_ptr like ses_get_page2_descriptor(). We record len as page1_len but we don't use it anywhere...

I wonder if someone knew the expected format we could make reject too short lengths earlier.

drivers/scsi/ses.c +0 -0
index dcb0d76..39f69b0 100644

drivers/scsi/ses.c static int ses_intf_add(struct device *cdev, · line 646

Quoted 7 lines
drivers/scsi/ses.c +1 -1
@@ -641,7 +641,7 @@ static int ses_intf_add(struct device *cdev,
 	/* begin at the enclosure descriptor */
 	type_ptr = buf + 8;
 	/* skip all the enclosure descriptors */
-	for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) {
+	for (i = 0; i < num_enclosures && type_ptr + 4 < buf + len; i++) {
 		types += type_ptr[2];
 		type_ptr += type_ptr[3] + 4;

why "type_ptr + 4 < buf + len" here ? You're using type_ptr[3] only, so it's either "type_ptr + 4 <= buf + len" or "type_ptr + 3 < buf + len".

drivers/scsi/ses.c static int ses_intf_add(struct device *cdev, · line 655

Quoted 9 lines
drivers/scsi/ses.c +1 -1
@@ -649,7 +649,7 @@ static int ses_intf_add(struct device *cdev,
 	ses_dev->page1_types = type_ptr;
 	ses_dev->page1_num_types = types;
 
-	for (i = 0; i < types && type_ptr < buf + len; i++, type_ptr += 4) {
+	for (i = 0; i < types && type_ptr + 2 < buf + len; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 			components += type_ptr[1];

Same here where I'd expect "type_ptr + 1 < buf + len"

Regards,
Willy

author
#3 lore raw
In reply to Dan Carpenter

James, could you take a look at this? My patch was really incomplete and even the bits that I wrote were buggy... :/ This might be easier for someone who is familiar with the code and can say what the expected ranges are etc.

regards,
dan carpenter

maintainer
#4 lore raw
Quoted 4 lines · Dan Carpenter

James, could you take a look at this? My patch was really incomplete and even the bits that I wrote were buggy... :/ This might be easier for someone who is familiar with the code and can say what the expected ranges are etc.

Yes, I've got a couple of SES things to look into, but not until after the merge window closes, probably.

James