ses: fix VPD inquiry overrun

ses: fix VPD inquiry overrun

[James Bottomley]

There are a few kerneloops.org reports like this one:

http://www.kerneloops.org/search.php?search=ses_match_to_enclosure

That seem to imply we're running off the end of the VPD inquiry data
(although at 512 bytes, it should be long enough for just about
anything).  we should be using correctly sized buffers anyway, so put
those in and hope this oops goes away.

James

---

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 0fe031f..f2d89be 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -345,14 +345,14 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
 	return 0;
 }
 
-#define VPD_INQUIRY_SIZE 512
+#define VPD_INQUIRY_SIZE 36
 
 static void ses_match_to_enclosure(struct enclosure_device *edev,
 				   struct scsi_device *sdev)
 {
 	unsigned char *buf = kmalloc(VPD_INQUIRY_SIZE, GFP_KERNEL);
 	unsigned char *desc;
-	int len;
+	u16 vpd_len;
 	struct efd efd = {
 		.addr = 0,
 	};
@@ -372,9 +372,19 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
 			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES))
 		goto free;
 
-	len = (buf[2] << 8) + buf[3];
+	vpd_len = (buf[2] << 8) + buf[3];
+	kfree(buf);
+	buf = kmalloc(vpd_len, GFP_KERNEL);
+	if (!buf)
+		return;
+	cmd[3] = vpd_len >> 8;
+	cmd[4] = vpd_len & 0xff;
+	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
+			     vpd_len, NULL, SES_TIMEOUT, SES_RETRIES))
+		goto free;
+	
 	desc = buf + 4;
-	while (desc < buf + len) {
+	while (desc < buf + vpd_len) {
 		enum scsi_protocol proto = desc[0] >> 4;
 		u8 code_set = desc[0] & 0x0f;
 		u8 piv = desc[1] & 0x80;

Re: ses: fix VPD inquiry overrun

[Boaz Harrosh]

James Bottomley wrote:
> There are a few kerneloops.org reports like this one:
> 
> http://www.kerneloops.org/search.php?search=ses_match_to_enclosure
> 
> That seem to imply we're running off the end of the VPD inquiry data
> (although at 512 bytes, it should be long enough for just about
> anything).  we should be using correctly sized buffers anyway, so put
> those in and hope this oops goes away.
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 0fe031f..f2d89be 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -345,14 +345,14 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
>  	return 0;
>  }
>  
> -#define VPD_INQUIRY_SIZE 512
> +#define VPD_INQUIRY_SIZE 36
>  
>  static void ses_match_to_enclosure(struct enclosure_device *edev,
>  				   struct scsi_device *sdev)
>  {
>  	unsigned char *buf = kmalloc(VPD_INQUIRY_SIZE, GFP_KERNEL);
>  	unsigned char *desc;
> -	int len;
> +	u16 vpd_len;
>  	struct efd efd = {
>  		.addr = 0,
>  	};
> @@ -372,9 +372,19 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
>  			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES))
>  		goto free;
>  
> -	len = (buf[2] << 8) + buf[3];
> +	vpd_len = (buf[2] << 8) + buf[3];

+	vpd_len = get_unaligned_be16(&buf[2]);

> +	kfree(buf);
> +	buf = kmalloc(vpd_len, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +	cmd[3] = vpd_len >> 8;
> +	cmd[4] = vpd_len & 0xff;

+	put_unaligned_be16(vpd_len, &cmd[3]);

> +	if (scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf,
> +			     vpd_len, NULL, SES_TIMEOUT, SES_RETRIES))
> +		goto free;
> +	
>  	desc = buf + 4;
> -	while (desc < buf + len) {
> +	while (desc < buf + vpd_len) {
>  		enum scsi_protocol proto = desc[0] >> 4;
>  		u8 code_set = desc[0] & 0x0f;
>  		u8 piv = desc[1] & 0x80;
> 
> 
> 

Harvey where are these bitwise accessors that do an *aligned* access on a 
void pointer. the equivalent of:
	u16 = be16_to_cpup((const __be16 *p)&buff[x]);
Where I know that x above is some even offset.

I've seen you sent them but I can't find them in my tree

Boaz

Re: ses: fix VPD inquiry overrun

[James Bottomley]

On Wed, 2008-07-30 at 12:44 +0300, Boaz Harrosh wrote:
> James Bottomley wrote:
> > There are a few kerneloops.org reports like this one:
> > 
> > http://www.kerneloops.org/search.php?search=ses_match_to_enclosure
> > 
> > That seem to imply we're running off the end of the VPD inquiry data
> > (although at 512 bytes, it should be long enough for just about
> > anything).  we should be using correctly sized buffers anyway, so put
> > those in and hope this oops goes away.
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> > index 0fe031f..f2d89be 100644
> > --- a/drivers/scsi/ses.c
> > +++ b/drivers/scsi/ses.c
> > @@ -345,14 +345,14 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
> >  	return 0;
> >  }
> >  
> > -#define VPD_INQUIRY_SIZE 512
> > +#define VPD_INQUIRY_SIZE 36
> >  
> >  static void ses_match_to_enclosure(struct enclosure_device *edev,
> >  				   struct scsi_device *sdev)
> >  {
> >  	unsigned char *buf = kmalloc(VPD_INQUIRY_SIZE, GFP_KERNEL);
> >  	unsigned char *desc;
> > -	int len;
> > +	u16 vpd_len;
> >  	struct efd efd = {
> >  		.addr = 0,
> >  	};
> > @@ -372,9 +372,19 @@ static void ses_match_to_enclosure(struct enclosure_device *edev,
> >  			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES))
> >  		goto free;
> >  
> > -	len = (buf[2] << 8) + buf[3];
> > +	vpd_len = (buf[2] << 8) + buf[3];
> 
> +	vpd_len = get_unaligned_be16(&buf[2]);

Not at the moment.  Matthew had a patch to add the necessary scsi
wrappers (including the missing 24 bit one) to replace our current u32
one.

But that goes after this, and this will be a backport.

James

Re: ses: fix VPD inquiry overrun

[Harvey Harrison]

On Wed, 2008-07-30 at 12:44 +0300, Boaz Harrosh wrote:
> James Bottomley wrote:

> Harvey where are these bitwise accessors that do an *aligned* access on a 
> void pointer. the equivalent of:
> 	u16 = be16_to_cpup((const __be16 *p)&buff[x]);
> Where I know that x above is some even offset.
> 
> I've seen you sent them but I can't find them in my tree
> 

They weren't accepted.  No need to duplicate be16_to_cpup etc.

Cheers,

Harvey