| Message ID | 20151019134820.GA28752@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dan, On Mon, Oct 19, 2015 at 04:48:20PM +0300, Dan Carpenter wrote: > 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 ??????(??????)" <chengmiao.cj@alibaba-inc.com> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > 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. > > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c > index dcb0d76..39f69b0 100644 > --- a/drivers/scsi/ses.c > +++ b/drivers/scsi/ses.c > @@ -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". > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-11-06 at 00:52 +0300, Dan Carpenter wrote: > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index dcb0d76..39f69b0 100644 --- a/drivers/scsi/ses.c +++ b/drivers/scsi/ses.c @@ -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];
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 ??(??)" <chengmiao.cj@alibaba-inc.com> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- 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. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html