Re: [PATCH] scsi: fix scsi_mode_sense()

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/08/20 2:04, Bart Van Assche wrote:
> On 8/19/21 12:37 AM, Damien Le Moal wrote:
>> The allocation length field of the MODE SENSE 10 command is 16-bits,
>> occupying bytes 7 and 8 of the CDB. With this command, access to mode
>> pages larger than 255 bytes is thus possible. Make sure that
>> scsi_mode_sense() code reflects this by initializing the allocation
>> length field using put_unaligned_be16() instead of directly setting
>> only byte 8 of the CDB with the buffer length.
>>
>> While at it, also fix the folowing:
>> * use get_unaligned_be16() to retrieve the mode data length and block
>>    descriptor length fields of the mode sense reply header instead of
>>    using an open coded calculation.
>> * Fix the kdoc dbd argument explanation: the DBD bit stands for
>>    Disable Block Descriptor, which is the opposite of what the dbd
>>    argument description was.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>   drivers/scsi/scsi_lib.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 7456a26aef51..92db00d81733 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -2070,7 +2070,7 @@ EXPORT_SYMBOL_GPL(scsi_mode_select);
>>   /**
>>    *	scsi_mode_sense - issue a mode sense, falling back from 10 to six bytes if necessary.
>>    *	@sdev:	SCSI device to be queried
>> - *	@dbd:	set if mode sense will allow block descriptors to be returned
>> + *	@dbd:	set to prevent mode sense from returning block descriptors
>>    *	@modepage: mode page being requested
>>    *	@buffer: request buffer (may not be smaller than eight bytes)
>>    *	@len:	length of request buffer.
>> @@ -2112,7 +2112,7 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>>   			len = 8;
>>   
>>   		cmd[0] = MODE_SENSE_10;
>> -		cmd[8] = len;
>> +		put_unaligned_be16(len, &cmd[7]);
>>   		header_length = 8;
>>   	} else {
>>   		if (len < 4)
>> @@ -2166,12 +2166,11 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
>>   		data->longlba = 0;
>>   		data->block_descriptor_length = 0;
>>   	} else if (use_10_for_ms) {
>> -		data->length = buffer[0]*256 + buffer[1] + 2;
>> +		data->length = get_unaligned_be16(&buffer[0]) + 2;
>>   		data->medium_type = buffer[2];
>>   		data->device_specific = buffer[3];
>>   		data->longlba = buffer[4] & 0x01;
>> -		data->block_descriptor_length = buffer[6]*256
>> -			+ buffer[7];
>> +		data->block_descriptor_length = get_unaligned_be16(&buffer[6]);
>>   	} else {
>>   		data->length = buffer[0] + 1;
>>   		data->medium_type = buffer[1];
> 
> If the type of the scsi_mode_sense() 'len' argument is changed from 
> 'int' into 'uint16_t', feel free to add:

Bart,

I would rather keep the argument as an int and add a check for
"len > UINT16_MAX" to return an error (-EINVAL) rather than having the interface
automatically cast/truncate len values that are too large. Doing so, buggy
drivers will get back an error and can be fixed. With the change to uint16_t,
errors may end up being hidden.

scsi_mode_select() has such check. And looking at that function, it also has
problems with the buffer length max possible values as the added length of the
header is not accounted for. I fixed that too in a different patch (not sent
yet). Thoughts ?

> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 
> 
> 


-- 
Damien Le Moal
Western Digital Research