[PATCH 0/3] scsi: check the whole result in some places

[PATCH 0/3] scsi: check the whole result in some places

[Jason Yan]

Some bugs have been found related to the result of the command been
returned by the driver or the middle layer in some cases. Such as in
scsi_queue_rq() when the device is offline only the host byte is set.

Jason Yan (3):
  scsi: check the whole result for reading write protect flag
  scsi: only copy data to user when the whole result is good
  scsi: switch to use scsi_result_is_good() in
    scsi_result_to_blk_status()

 block/scsi_ioctl.c      |  2 +-
 drivers/scsi/scsi_lib.c |  2 +-
 drivers/scsi/sd.c       |  6 +++---
 include/scsi/scsi.h     | 13 +++++++++++++
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.25.4

[PATCH 1/3] scsi: check the whole result for reading write protect flag

[Jason Yan]

When the scsi device status is offline, mode sense command will return a
result with only DID_NO_CONNECT set. Then in sd_read_write_protect_flag(),
only status byte of the result is checked, we still consider the command
returned good, and read sdkp->write_prot from the buffer. And because of
bug [1], garbage data is copied to the buffer, the disk sometimes
be set readonly. When the scsi device is set running again, users cannot
write data to the disk.

Fix this by check the whole result returned by the driver.

[1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/sd.c   |  6 +++---
 include/scsi/scsi.h | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ed0b1bb99f08..16f8cd2895fd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2669,18 +2669,18 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer)
 		 * 5: Illegal Request, Sense Code 24: Invalid field in
 		 * CDB.
 		 */
-		if (!scsi_status_is_good(res))
+		if (!scsi_result_is_good(res))
 			res = sd_do_mode_sense(sdkp, 0, 0, buffer, 4, &data, NULL);
 
 		/*
 		 * Third attempt: ask 255 bytes, as we did earlier.
 		 */
-		if (!scsi_status_is_good(res))
+		if (!scsi_result_is_good(res))
 			res = sd_do_mode_sense(sdkp, 0, 0x3F, buffer, 255,
 					       &data, NULL);
 	}
 
-	if (!scsi_status_is_good(res)) {
+	if (!scsi_result_is_good(res)) {
 		sd_first_printk(KERN_WARNING, sdkp,
 			  "Test WP failed, assume Write Enabled\n");
 	} else {
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index e75cca25338a..db0f346a31b2 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
 		(status == SAM_STAT_COMMAND_TERMINATED));
 }
 
+/** scsi_result_is_good - check the result return.
+ *
+ * @result: the result passed up from the driver (including host and
+ *          driver components)
+ *
+ * Drivers may only set other bytes but not status byte.
+ * This checks both the status byte and other bytes.
+ */
+static inline int scsi_result_is_good(int result)
+{
+	return scsi_status_is_good(result) && (result & ~0xff) == 0;
+}
+
 
 /*
  * standard mode-select header prepended to all mode-select commands
-- 
2.25.4

[PATCH 2/3] scsi: only copy data to user when the whole result is good

[Jason Yan]

When the scsi device status is offline, mode sense command will return a
result with only DID_NO_CONNECT set. Then in sg_scsi_ioctl(),
only status byte of the result is checked, and because of
bug [1], garbage data is copied to the userspace.

Only copy the buffer to userspace when the whole result is good.

[1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 block/scsi_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 6599bac0a78c..359bf0003af4 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 			if (copy_to_user(sic->data, req->sense, bytes))
 				err = -EFAULT;
 		}
-	} else {
+	} else if (scsi_result_is_good(req->result)) {
 		if (copy_to_user(sic->data, buffer, out_len))
 			err = -EFAULT;
 	}
-- 
2.25.4

[PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status()

[Jason Yan]

Use scsi_result_is_good() to simplify the code.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7d52a11e1b61..d1ec9f6a93f0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -626,7 +626,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 		 * to handle the case when a SCSI LLD sets result to
 		 * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION.
 		 */
-		if (scsi_status_is_good(result) && (result & ~0xff) == 0)
+		if (scsi_result_is_good(result))
 			return BLK_STS_OK;
 		return BLK_STS_IOERR;
 	case DID_TRANSPORT_FAILFAST:
-- 
2.25.4

Re: [PATCH 1/3] scsi: check the whole result for reading write protect flag

[Hannes Reinecke]

On 3/19/21 4:01 AM, Jason Yan wrote:
> When the scsi device status is offline, mode sense command will return a
> result with only DID_NO_CONNECT set. Then in sd_read_write_protect_flag(),
> only status byte of the result is checked, we still consider the command
> returned good, and read sdkp->write_prot from the buffer. And because of
> bug [1], garbage data is copied to the buffer, the disk sometimes
> be set readonly. When the scsi device is set running again, users cannot
> write data to the disk.
> 
> Fix this by check the whole result returned by the driver.
> 
> [1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   drivers/scsi/sd.c   |  6 +++---
>   include/scsi/scsi.h | 13 +++++++++++++
>   2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index ed0b1bb99f08..16f8cd2895fd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2669,18 +2669,18 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer)
>   		 * 5: Illegal Request, Sense Code 24: Invalid field in
>   		 * CDB.
>   		 */
> -		if (!scsi_status_is_good(res))
> +		if (!scsi_result_is_good(res))
>   			res = sd_do_mode_sense(sdkp, 0, 0, buffer, 4, &data, NULL);
>   
>   		/*
>   		 * Third attempt: ask 255 bytes, as we did earlier.
>   		 */
> -		if (!scsi_status_is_good(res))
> +		if (!scsi_result_is_good(res))
>   			res = sd_do_mode_sense(sdkp, 0, 0x3F, buffer, 255,
>   					       &data, NULL);
>   	}
>   
> -	if (!scsi_status_is_good(res)) {
> +	if (!scsi_result_is_good(res)) {
>   		sd_first_printk(KERN_WARNING, sdkp,
>   			  "Test WP failed, assume Write Enabled\n");
>   	} else {
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index e75cca25338a..db0f346a31b2 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
>   		(status == SAM_STAT_COMMAND_TERMINATED));
>   }
>   
> +/** scsi_result_is_good - check the result return.
> + *
> + * @result: the result passed up from the driver (including host and
> + *          driver components)
> + *
> + * Drivers may only set other bytes but not status byte.
> + * This checks both the status byte and other bytes.
> + */
> +static inline int scsi_result_is_good(int result)
> +{
> +	return scsi_status_is_good(result) && (result & ~0xff) == 0;
> +}
> +
>   
>   /*
>    * standard mode-select header prepended to all mode-select commands
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/3] scsi: only copy data to user when the whole result is good

[Hannes Reinecke]

On 3/19/21 4:01 AM, Jason Yan wrote:
> When the scsi device status is offline, mode sense command will return a
> result with only DID_NO_CONNECT set. Then in sg_scsi_ioctl(),
> only status byte of the result is checked, and because of
> bug [1], garbage data is copied to the userspace.
> 
> Only copy the buffer to userspace when the whole result is good.
> 
> [1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   block/scsi_ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 6599bac0a78c..359bf0003af4 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
>   			if (copy_to_user(sic->data, req->sense, bytes))
>   				err = -EFAULT;
>   		}
> -	} else {
> +	} else if (scsi_result_is_good(req->result)) {
>   		if (copy_to_user(sic->data, buffer, out_len))
>   			err = -EFAULT;
>   	}
> 
Hmm. Not sure about this one.
The prime motivator behind sg is to get _precisely_ all flags of the 
command, and not do in-kernel error handling.
So one could argue that this behaviour is intentional, and would break 
existing use-cases.

Doug?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status()

[Hannes Reinecke]

On 3/19/21 4:01 AM, Jason Yan wrote:
> Use scsi_result_is_good() to simplify the code.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   drivers/scsi/scsi_lib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7d52a11e1b61..d1ec9f6a93f0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -626,7 +626,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
>   		 * to handle the case when a SCSI LLD sets result to
>   		 * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION.
>   		 */
> -		if (scsi_status_is_good(result) && (result & ~0xff) == 0)
> +		if (scsi_result_is_good(result))
>   			return BLK_STS_OK;
>   		return BLK_STS_IOERR;
>   	case DID_TRANSPORT_FAILFAST:
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/3] scsi: only copy data to user when the whole result is good

[Jason Yan]

Hi Hannes,

在 2021/3/19 15:56, Hannes Reinecke 写道:
> On 3/19/21 4:01 AM, Jason Yan wrote:
>> When the scsi device status is offline, mode sense command will return a
>> result with only DID_NO_CONNECT set. Then in sg_scsi_ioctl(),
>> only status byte of the result is checked, and because of
>> bug [1], garbage data is copied to the userspace.
>>
>> Only copy the buffer to userspace when the whole result is good.
>>
>> [1] 
>> https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/ 
>>
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>>   block/scsi_ioctl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
>> index 6599bac0a78c..359bf0003af4 100644
>> --- a/block/scsi_ioctl.c
>> +++ b/block/scsi_ioctl.c
>> @@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct 
>> gendisk *disk, fmode_t mode,
>>               if (copy_to_user(sic->data, req->sense, bytes))
>>                   err = -EFAULT;
>>           }
>> -    } else {
>> +    } else if (scsi_result_is_good(req->result)) {
>>           if (copy_to_user(sic->data, buffer, out_len))
>>               err = -EFAULT;
>>       }
>>
> Hmm. Not sure about this one.
> The prime motivator behind sg is to get _precisely_ all flags of the 
> command, and not do in-kernel error handling.
> So one could argue that this behaviour is intentional, and would break 
> existing use-cases.
> 

Thanks for the review.

The existing usersapce can do nothing with the uninitialized data. Or 
the driver or disk may fill some data and at the same time set host_byte 
or driver_byte to non-zero? I'm not sure about this. And the return 
value of sg_scsi_ioctl() just get the status byte(only 8 bit), how can 
the users know about this situation?

Thanks,
Jason

> Doug?
> 
> Cheers,
> 
> Hannes

Re: [PATCH 1/3] scsi: check the whole result for reading write protect flag

[Martin K. Petersen]

Hi Jason!

> @@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
>  		(status == SAM_STAT_COMMAND_TERMINATED));
>  }
>  
> +/** scsi_result_is_good - check the result return.
> + *
> + * @result: the result passed up from the driver (including host and
> + *          driver components)
> + *
> + * Drivers may only set other bytes but not status byte.
> + * This checks both the status byte and other bytes.
> + */
> +static inline int scsi_result_is_good(int result)
> +{
> +	return scsi_status_is_good(result) && (result & ~0xff) == 0;
> +}
> +
>  
>  /*
>   * standard mode-select header prepended to all mode-select commands

Instead of introducing a "don't be broken" variant of
scsi_status_is_good(), I'd prefer you to fix the latter to do the right
thing wrt. offline devices.

There aren't a ton of scsi_result_is_good() call sites to check. And I
suspect that most of them wouldn't actually consider the DID_NO_CONNECT
scenario to be "good".

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/3] scsi: check the whole result for reading write protect flag

[Jason Yan]

Hi Martin,

在 2021/3/25 10:50, Martin K. Petersen 写道:
> 
> Hi Jason!
> 
>> @@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
>>   		(status == SAM_STAT_COMMAND_TERMINATED));
>>   }
>>   
>> +/** scsi_result_is_good - check the result return.
>> + *
>> + * @result: the result passed up from the driver (including host and
>> + *          driver components)
>> + *
>> + * Drivers may only set other bytes but not status byte.
>> + * This checks both the status byte and other bytes.
>> + */
>> +static inline int scsi_result_is_good(int result)
>> +{
>> +	return scsi_status_is_good(result) && (result & ~0xff) == 0;
>> +}
>> +
>>   
>>   /*
>>    * standard mode-select header prepended to all mode-select commands
> 
> Instead of introducing a "don't be broken" variant of
> scsi_status_is_good(), I'd prefer you to fix the latter to do the right
> thing wrt. offline devices.
> 
> There aren't a ton of scsi_result_is_good() call sites to check. And I
> suspect that most of them wouldn't actually consider the DID_NO_CONNECT
> scenario to be "good".
> 

OK, I'll have a try and check if it works for most of the call sites.

Thanks,
Jason