| Message ID | 8d6beef7-5995-c831-a7b6-ff98d3887231@omp.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: storage: isd200: fix sloppy typing in isd200_scsi_to_ata() | expand |
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.shtylyov@omp.ru> [...] MBR, Sergey
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.shtylyov@omp.ru> Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > --- > 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);
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);
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.shtylyov@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(-)