On Fri, Mar 22, 2024 at 12:49:23PM +0300, Sergey Shtylyov wrote:
> On 3/22/24 3:57 AM, 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
their reports, and look around the vicinity to see if they missed
something obvious.
> I'll fix
> them up in v2 if you're not opposed to this patch...
Sure, go ahead.
Alan Stern
