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

[Hannes Reinecke <hare@suse.de>]

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