Re: [PATCH v2 08/12] scsi: sd: Fix scsi_mode_sense caller's sshdr use

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/4/23 5:37 PM, Damien Le Moal wrote:
> On 10/5/23 06:00, Mike Christie wrote:
>> The sshdr passed into scsi_execute_cmd is only initialized if
>> scsi_execute_cmd returns >= 0, and scsi_mode_sense will convert all non
>> good statuses like check conditions to -EIO. This has scsi_mode_sense
>> callers that were possibly accessing an uninitialized sshdrs to only
>> access it if we got -EIO.
>>
>> Signed-off-by: Mike Christie <michael.christie@xxxxxxxxxx>
>> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>> Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
>> ---
>>  drivers/scsi/sd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 6d4787ff6e96..538ebdf42c69 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -2942,7 +2942,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
>>  	}
>>  
>>  bad_sense:
>> -	if (scsi_sense_valid(&sshdr) &&
>> +	if (res == -EIO && scsi_sense_valid(&sshdr) &&
> 
> 	if (ret < 0 && ...
> 
> would be safer and avoid any issue if we ever change scsi_execute_cmd() to
> return other error codes than -EIO, no ?

If we do that, then we will have the same problem we have today
where we can access the sshdr when it's not setup.

If scsi_execute_cmd returns < 0, then the sshdr is not setup, so
we shouldn't access it. The res value above is from scsi_mode_sense
which actually does the scsi_execute_cmd call, but it doesn't always
pass the return vale from scsi_execute_cmd directly to its callers.

If there is valid sense then scsi_mode_sense returns -EIO so above
that's why we check for that return code.

As far as future safety goes, this patch is not great. Right now
we assume scsi_execute_cmd and the functions it calls does not
return -EIO. To make it safer we could change scsi_mode_sense to
return 1 for the case there is sense or add another arg which
gets set when there is sense.






[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux