[lvc-project] [PATCH v2] usb: storage: isd200: fix sloppy typing in isd200_scsi_to_ata()

[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.shtylyov at omp.ru>

Reviewed-by: Alan Stern <stern at 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);