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@xxxxxx>
>>>>
>>>> ---
>>>> 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
