[patch] ses: tighten range checks in ses_intf_add()

[patch] ses: tighten range checks in ses_intf_add()

[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 程君(成淼)" <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;
 	}
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] ses: tighten range checks in ses_intf_add()

[Willy Tarreau]

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

Re: [patch] ses: tighten range checks in ses_intf_add()

[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

Re: [patch] ses: tighten range checks in ses_intf_add()

[James Bottomley]

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