[PATCH] ses: Fix problems with simple enclosures

[PATCH] ses: Fix problems with simple enclosures

[James Bottomley]

Simple enclosure implementations (mostly USB) are allowed to return only
page 8 to every diagnostic query.  That really confuses our
implementation because we assume the return is the page we asked for and
end up doing incorrect offsets based on bogus information leading to
accesses outside of allocated ranges.  Fix that by checking the page
code of the return and giving an error if it isn't the one we asked for.
This should fix reported bugs with USB storage by simply refusing to
attach to enclosures that behave like this.  It's also good defensive
practise now that we're starting to see more USB enclosures.

Reported-by: Andrea Gelmini <andrea.gelmini@gelma.net>
Cc: stable@vger.kernel.org
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index dcb0d76..7d9cec5 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char *dest_desc,
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 			 void *buf, int bufflen)
 {
+	int ret;
 	unsigned char cmd[] = {
 		RECEIVE_DIAGNOSTIC,
 		1,		/* Set PCV bit */
@@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 		bufflen & 0xff,
 		0
 	};
+	unsigned char recv_page_code;
 
-	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
+	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
 				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
+	if (unlikely(!ret))
+		return ret;
+
+	recv_page_code = ((unsigned char *)buf)[0];
+
+	if (likely(recv_page_code == page_code))
+		return ret;
+
+	/* successful diagnostic but wrong page code.  This happens to some
+	 * USB devices, just print a message and pretend there was an error */
+
+	sdev_printk(KERN_ERR, sdev,
+		    "Wrong diagnostic page; asked for %d got %u\n",
+		    page_code, recv_page_code);
+
+	return -EINVAL;
 }
 
 static int ses_send_diag(struct scsi_device *sdev, int page_code,

Re: [PATCH] ses: Fix problems with simple enclosures

[Ewan Milne]

On Tue, 2015-12-08 at 09:00 -0800, James Bottomley wrote:
> Simple enclosure implementations (mostly USB) are allowed to return only
> page 8 to every diagnostic query.  That really confuses our
> implementation because we assume the return is the page we asked for and
> end up doing incorrect offsets based on bogus information leading to
> accesses outside of allocated ranges.  Fix that by checking the page
> code of the return and giving an error if it isn't the one we asked for.
> This should fix reported bugs with USB storage by simply refusing to
> attach to enclosures that behave like this.  It's also good defensive
> practise now that we're starting to see more USB enclosures.
> 
> Reported-by: Andrea Gelmini <andrea.gelmini@gelma.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index dcb0d76..7d9cec5 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char *dest_desc,
>  static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>  			 void *buf, int bufflen)
>  {
> +	int ret;
>  	unsigned char cmd[] = {
>  		RECEIVE_DIAGNOSTIC,
>  		1,		/* Set PCV bit */
> @@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>  		bufflen & 0xff,
>  		0
>  	};
> +	unsigned char recv_page_code;
>  
> -	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
> +	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
>  				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
> +	if (unlikely(!ret))
> +		return ret;
> +
> +	recv_page_code = ((unsigned char *)buf)[0];
> +
> +	if (likely(recv_page_code == page_code))
> +		return ret;
> +
> +	/* successful diagnostic but wrong page code.  This happens to some
> +	 * USB devices, just print a message and pretend there was an error */
> +
> +	sdev_printk(KERN_ERR, sdev,
> +		    "Wrong diagnostic page; asked for %d got %u\n",
> +		    page_code, recv_page_code);
> +
> +	return -EINVAL;
>  }
>  
>  static int ses_send_diag(struct scsi_device *sdev, int page_code,
> 
> 

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH] ses: Fix problems with simple enclosures

[Tomas Henzl]

On 8.12.2015 18:00, James Bottomley wrote:
> Simple enclosure implementations (mostly USB) are allowed to return only
> page 8 to every diagnostic query.  That really confuses our
> implementation because we assume the return is the page we asked for and
> end up doing incorrect offsets based on bogus information leading to
> accesses outside of allocated ranges.  Fix that by checking the page
> code of the return and giving an error if it isn't the one we asked for.
> This should fix reported bugs with USB storage by simply refusing to
> attach to enclosures that behave like this.  It's also good defensive
> practise now that we're starting to see more USB enclosures.

Ideally this patch also fixes all callers so they evaluate the return value
from ses_recv_diag. That is missed in ses_enclosure_data_process
and ses_get_page2_descriptor.

-tms 

>
> Reported-by: Andrea Gelmini <andrea.gelmini@gelma.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
>
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index dcb0d76..7d9cec5 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char *dest_desc,
>  static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>  			 void *buf, int bufflen)
>  {
> +	int ret;
>  	unsigned char cmd[] = {
>  		RECEIVE_DIAGNOSTIC,
>  		1,		/* Set PCV bit */
> @@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>  		bufflen & 0xff,
>  		0
>  	};
> +	unsigned char recv_page_code;
>  
> -	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
> +	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
>  				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
> +	if (unlikely(!ret))
> +		return ret;
> +
> +	recv_page_code = ((unsigned char *)buf)[0];
> +
> +	if (likely(recv_page_code == page_code))
> +		return ret;
> +
> +	/* successful diagnostic but wrong page code.  This happens to some
> +	 * USB devices, just print a message and pretend there was an error */
> +
> +	sdev_printk(KERN_ERR, sdev,
> +		    "Wrong diagnostic page; asked for %d got %u\n",
> +		    page_code, recv_page_code);
> +
> +	return -EINVAL;
>  }
>  
>  static int ses_send_diag(struct scsi_device *sdev, int page_code,
>
>
> --
> 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: Fix problems with simple enclosures

[James Bottomley]

On Wed, 2015-12-09 at 18:07 +0100, Tomas Henzl wrote:
> On 8.12.2015 18:00, James Bottomley wrote:
> > Simple enclosure implementations (mostly USB) are allowed to return only
> > page 8 to every diagnostic query.  That really confuses our
> > implementation because we assume the return is the page we asked for and
> > end up doing incorrect offsets based on bogus information leading to
> > accesses outside of allocated ranges.  Fix that by checking the page
> > code of the return and giving an error if it isn't the one we asked for.
> > This should fix reported bugs with USB storage by simply refusing to
> > attach to enclosures that behave like this.  It's also good defensive
> > practise now that we're starting to see more USB enclosures.
> 
> Ideally this patch also fixes all callers so they evaluate the return value
> from ses_recv_diag. That is missed in ses_enclosure_data_process
> and ses_get_page2_descriptor.

Well, it wouldn't be a bug fix and it's strictly not necessary.  in
ses_intf_add() we won't attach if the initial retrieve of page 2 fails.
That means we already have an old copy.  So if there's a failure in
ses_get_page2_descriptor() then we're just working from old data.
Essentially there's nothing else we could do (except perhaps log the
problem).

James

> -tms 
> 
> >
> > Reported-by: Andrea Gelmini <andrea.gelmini@gelma.net>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >
> > ---
> >
> > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> > index dcb0d76..7d9cec5 100644
> > --- a/drivers/scsi/ses.c
> > +++ b/drivers/scsi/ses.c
> > @@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char *dest_desc,
> >  static int ses_recv_diag(struct scsi_device *sdev, int page_code,
> >  			 void *buf, int bufflen)
> >  {
> > +	int ret;
> >  	unsigned char cmd[] = {
> >  		RECEIVE_DIAGNOSTIC,
> >  		1,		/* Set PCV bit */
> > @@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
> >  		bufflen & 0xff,
> >  		0
> >  	};
> > +	unsigned char recv_page_code;
> >  
> > -	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
> > +	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
> >  				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
> > +	if (unlikely(!ret))
> > +		return ret;
> > +
> > +	recv_page_code = ((unsigned char *)buf)[0];
> > +
> > +	if (likely(recv_page_code == page_code))
> > +		return ret;
> > +
> > +	/* successful diagnostic but wrong page code.  This happens to some
> > +	 * USB devices, just print a message and pretend there was an error */
> > +
> > +	sdev_printk(KERN_ERR, sdev,
> > +		    "Wrong diagnostic page; asked for %d got %u\n",
> > +		    page_code, recv_page_code);
> > +
> > +	return -EINVAL;
> >  }
> >  
> >  static int ses_send_diag(struct scsi_device *sdev, int page_code,
> >
> >
> > --
> > 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
> 
> --
> 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: Fix problems with simple enclosures

[Tomas Henzl]

On 9.12.2015 19:15, James Bottomley wrote:
> On Wed, 2015-12-09 at 18:07 +0100, Tomas Henzl wrote:
>> On 8.12.2015 18:00, James Bottomley wrote:
>>> Simple enclosure implementations (mostly USB) are allowed to return only
>>> page 8 to every diagnostic query.  That really confuses our
>>> implementation because we assume the return is the page we asked for and
>>> end up doing incorrect offsets based on bogus information leading to
>>> accesses outside of allocated ranges.  Fix that by checking the page
>>> code of the return and giving an error if it isn't the one we asked for.
>>> This should fix reported bugs with USB storage by simply refusing to
>>> attach to enclosures that behave like this.  It's also good defensive
>>> practise now that we're starting to see more USB enclosures.
>> Ideally this patch also fixes all callers so they evaluate the return value
>> from ses_recv_diag. That is missed in ses_enclosure_data_process
>> and ses_get_page2_descriptor.
> Well, it wouldn't be a bug fix and it's strictly not necessary.  in
> ses_intf_add() we won't attach if the initial retrieve of page 2 fails.
> That means we already have an old copy.  So if there's a failure in
> ses_get_page2_descriptor() then we're just working from old data.
> Essentially there's nothing else we could do (except perhaps log the
> problem).

You are right it is very unlikely that we read page2 with success in ses_intf_add
and later page 8 is read instead in ses_get_page2_descriptor and rewrites the page 2. 
That means that we no longer have the old copy.
Anyway its almost impossible and your patch per se is correct.

Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Tomas

>
> James
>
>> -tms 
>>
>>> Reported-by: Andrea Gelmini <andrea.gelmini@gelma.net>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>>
>>> ---
>>>
>>> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
>>> index dcb0d76..7d9cec5 100644
>>> --- a/drivers/scsi/ses.c
>>> +++ b/drivers/scsi/ses.c
>>> @@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char *dest_desc,
>>>  static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>>>  			 void *buf, int bufflen)
>>>  {
>>> +	int ret;
>>>  	unsigned char cmd[] = {
>>>  		RECEIVE_DIAGNOSTIC,
>>>  		1,		/* Set PCV bit */
>>> @@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>>>  		bufflen & 0xff,
>>>  		0
>>>  	};
>>> +	unsigned char recv_page_code;
>>>  
>>> -	return scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
>>> +	ret =  scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, bufflen,
>>>  				NULL, SES_TIMEOUT, SES_RETRIES, NULL);
>>> +	if (unlikely(!ret))
>>> +		return ret;
>>> +
>>> +	recv_page_code = ((unsigned char *)buf)[0];
>>> +
>>> +	if (likely(recv_page_code == page_code))
>>> +		return ret;
>>> +
>>> +	/* successful diagnostic but wrong page code.  This happens to some
>>> +	 * USB devices, just print a message and pretend there was an error */
>>> +
>>> +	sdev_printk(KERN_ERR, sdev,
>>> +		    "Wrong diagnostic page; asked for %d got %u\n",
>>> +		    page_code, recv_page_code);
>>> +
>>> +	return -EINVAL;
>>>  }
>>>  
>>>  static int ses_send_diag(struct scsi_device *sdev, int page_code,
>>>
>>>
>>> --
>>> 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
>> --
>> 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
>>
>
>
> --
> 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