| Message ID | e8c20e3c-3cbe-b5c1-f673-0a7f22566879@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | usb: storage: isd200: fix sloppy typing in isd200_scsi_to_ata() | expand |
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@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 resason is simple: Svace didn't complain about those. I'll fix them up in v2 if you;re not opposed to this patch... > Alan Stern MBR, Sergey
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@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 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
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@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
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; usb_stor_dbg(us, " ATA OUT - SCSIOP_READ_CAPACITY\n");
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@omp.ru> --- drivers/usb/storage/isd200.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)