[lvc-project] [PATCH] usb: storage: isd200: fix sloppy typing in isd200_scsi_to_ata()

[Sergey Shtylyov]

On 3/22/24 5:38 PM, Alan Stern wrote:
[...]

>>>> When isd200_scsi_to_ata() emulates the SCSI READ CAPACITY command, the
>>>> capacity local variable is needlessly typed as *unsigned long* -- which
>>>> is 32-bit type on the 32-bit arches and 64-bit type on the 64-bit arches;
>>>> this variable's value should always fit into 32 bits for both the ATA and
>>>> the SCSI capacity data...
>>>>
>>>> While at it, arrange the local variable declarations in the reverse Xmas
>>>> tree order...
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>>> analysis tool.
>>>>
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov at omp.ru>
>>>>
>>>> ---
>>>>  drivers/usb/storage/isd200.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> Index: usb/drivers/usb/storage/isd200.c
>>>> ===================================================================
>>>> --- usb.orig/drivers/usb/storage/isd200.c
>>>> +++ usb/drivers/usb/storage/isd200.c
>>>> @@ -1283,8 +1283,8 @@ static int isd200_scsi_to_ata(struct scs
>>>>  
>>>>  	case READ_CAPACITY:
>>>>  	{
>>>> -		unsigned long capacity;
>>>>  		struct read_capacity_data readCapacityData;
>>>> +		u32 capacity;
>>>
>>> This is a bit peculiar.  Why bother to change the type of the capacity 
>>> variable without also changing the types of lba and blockCount at the 
>>> start of the routine?
>>
>>    The reason is simple: Svace didn't complain about those.
> 
> You shouldn't trust automated code checkers too far.  Always verify 

   I never do. In this case, Svace suggested a cast to 64-bit type to
avoid what it suspected to be an overflow. :-)

> their reports, and look around the vicinity to see if they missed 
> something obvious.

   Yeah, I forgot to do that. :-)

>>  I'll fix
>> them up in v2 if you're not opposed to this patch...
> 
> Sure, go ahead.

   OK! :-)

> Alan Stern

MBR. Sergey