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

[Sergey Shtylyov]

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.sht...@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;

usb_stor_dbg(us, " ATA OUT - SCSIOP_READ_CAPACITY\n");

[Alan Stern]

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?

Alan Stern

[Sergey Shtylyov]

On 3/22/24 3:57 AM, Alan Stern wrote:
[...]

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

[Sergey Shtylyov]

On 3/22/24 3:57 AM, Alan Stern wrote:
[...]

The reason is simple: Svace didn't complain about those. I'll fix

[Alan Stern]

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

[Sergey Shtylyov]

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

[Sergey Shtylyov]

When isd200_scsi_to_ata() emulates the SCSI READ/WRITE (10) commands,
the LBA is a 32-bit CDB field and the transfer length is a 16-bit CDB
field, so using *unsigned long* (which is 32-bit type on the 32-bit
arches and 64-bit type on the 64-bit arches) to declare the lba and
blockCount variables doesn't make much sense. Also, when it emulates
the READ CAPACITY command, the returned LBA is a 32-bit parameter data
field and the ATA device CHS mode capacity fits into 32 bits as well,
so using *unsigned long* to declare the capacity variable doesn't make
much sense as well. Let's use the u16/u32 types for those variables...

Found by Linux Verification Center (linuxtesting.org) with the SVACE
static analysis tool.

Signed-off-by: Sergey Shtylyov <s.sht...@omp.ru>

---

This patch is against the 'usb-next' branch of Greg KH's usb.git repo...

Changes in version 2:
- fixed up the lba and blockCount variable declarations;
- removed the typecasts from the blockCount variable calculation;
- undid the reordering of the capacity variable declaration;
- completely rewrote the patch description.

drivers/usb/storage/isd200.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

Index: usb/drivers/usb/storage/isd200.c
===================================================================
--- usb.orig/drivers/usb/storage/isd200.c
+++ usb/drivers/usb/storage/isd200.c

@@ -1232,8 +1232,8 @@ static int isd200_scsi_to_ata(struct scs
int sendToTransport = 1;
unsigned char sectnum, head;
unsigned short cylinder;
- unsigned long lba;
- unsigned long blockCount;
+ u32 lba;
+ u16 blockCount;
unsigned char senseData[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };

memset(ataCdb, 0, sizeof(union ata_cdb));
@@ -1291,7 +1291,7 @@ static int isd200_scsi_to_ata(struct scs

case READ_CAPACITY:
{
- unsigned long capacity;

+ u32 capacity;
struct read_capacity_data readCapacityData;

usb_stor_dbg(us, " ATA OUT - SCSIOP_READ_CAPACITY\n");

@@ -1316,7 +1316,7 @@ static int isd200_scsi_to_ata(struct scs
usb_stor_dbg(us, " ATA OUT - SCSIOP_READ\n");

lba = be32_to_cpu(*(__be32 *)&srb->cmnd[2]);
- blockCount = (unsigned long)srb->cmnd[7]<<8 | (unsigned long)srb->cmnd[8];
+ blockCount = srb->cmnd[7] << 8 | srb->cmnd[8];

if (ata_id_has_lba(id)) {
sectnum = (unsigned char)(lba);
@@ -1348,7 +1348,7 @@ static int isd200_scsi_to_ata(struct scs
usb_stor_dbg(us, " ATA OUT - SCSIOP_WRITE\n");

lba = be32_to_cpu(*(__be32 *)&srb->cmnd[2]);
- blockCount = (unsigned long)srb->cmnd[7]<<8 | (unsigned long)srb->cmnd[8];
+ blockCount = srb->cmnd[7] << 8 | srb->cmnd[8];

if (ata_id_has_lba(id)) {
sectnum = (unsigned char)(lba);

[Sergey Shtylyov]

On 3/23/24 10:55 PM, Sergey Shtylyov wrote:

> When isd200_scsi_to_ata() emulates the SCSI READ/WRITE (10) commands,
> the LBA is a 32-bit CDB field and the transfer length is a 16-bit CDB
> field, so using *unsigned long* (which is 32-bit type on the 32-bit
> arches and 64-bit type on the 64-bit arches) to declare the lba and
> blockCount variables doesn't make much sense. Also, when it emulates
> the READ CAPACITY command, the returned LBA is a 32-bit parameter data
> field and the ATA device CHS mode capacity fits into 32 bits as well,

Oops, it should have been s/CHS mode//... :-/

> so using *unsigned long* to declare the capacity variable doesn't make
> much sense as well. Let's use the u16/u32 types for those variables...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE
> static analysis tool.
>
> Signed-off-by: Sergey Shtylyov <s.sht...@omp.ru>

[...]

MBR, Sergey

[Alan Stern]

On Sat, Mar 23, 2024 at 10:55:51PM +0300, Sergey Shtylyov wrote:
> When isd200_scsi_to_ata() emulates the SCSI READ/WRITE (10) commands,
> the LBA is a 32-bit CDB field and the transfer length is a 16-bit CDB
> field, so using *unsigned long* (which is 32-bit type on the 32-bit
> arches and 64-bit type on the 64-bit arches) to declare the lba and
> blockCount variables doesn't make much sense. Also, when it emulates
> the READ CAPACITY command, the returned LBA is a 32-bit parameter data
> field and the ATA device CHS mode capacity fits into 32 bits as well,
> so using *unsigned long* to declare the capacity variable doesn't make
> much sense as well. Let's use the u16/u32 types for those variables...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE
> static analysis tool.
>
> Signed-off-by: Sergey Shtylyov <s.sht...@omp.ru>

Reviewed-by: Alan Stern <st...@rowland.harvard.edu>

[Sergey Shtylyov]

When isd200_scsi_to_ata() emulates the SCSI READ/WRITE (10) commands,
the LBA is a 32-bit CDB field and the transfer length is a 16-bit CDB
field, so using *unsigned long* (which is 32-bit type on the 32-bit
arches and 64-bit type on the 64-bit arches) to declare the lba and
blockCount variables doesn't make much sense. Also, when it emulates
the READ CAPACITY command, the returned LBA is a 32-bit parameter data

field and the ATA device capacity fits into 32 bits as well, so using
*unsigned long* to declare the capacity variable doesn't make sense as

well. Let's use the u16/u32 types for those variables...

Found by Linux Verification Center (linuxtesting.org) with the Svace

static analysis tool.

Signed-off-by: Sergey Shtylyov <s.sht...@omp.ru>
Reviewed-by: Alan Stern <st...@rowland.harvard.edu>

---
This patch is against the 'usb-next' branch of Greg KH's usb.git repo...

Changes in version 3:
- added Alan Stern's tag;
- removed the mentioning of the CHS mode;
- fixed the static analyzer's name.