Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022/05/13 22:50, Sergey Shtylyov wrote:
> The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
> to get a disk capacity implicitly uses the *int* type for that calculation
> and casting the result to 'u64' before returning ensues a sign extension.
> Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
> a sign extension instruction and so in a more compact code...
> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> 
> ---
> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
> 
>  drivers/ata/libata-core.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> Index: libata/drivers/ata/libata-core.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-core.c
> +++ libata/drivers/ata/libata-core.c
> @@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
>  			return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
>  	} else {
>  		if (ata_id_current_chs_valid(id))
> -			return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
> -			       id[ATA_ID_CUR_SECTORS];
> +			return (u32)id[ATA_ID_CUR_CYLS] *
> +			       (u32)id[ATA_ID_CUR_HEADS] *
> +			       (u32)id[ATA_ID_CUR_SECTORS];
>  		else

While at it, you can drop this useless "else". The 2 else above this one are
actually also useless...

> -			return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
> -			       id[ATA_ID_SECTORS];
> +			return (u32)id[ATA_ID_CYLS] *
> +			       (u32)id[ATA_ID_HEADS] *
> +			       (u32)id[ATA_ID_SECTORS];

Given that the function returns an u64, I would cast everything to u64. That
will avoid overflows too, which was possible before, eventhough no problems seem
to have been reported... Who uses CHS these days :)

>  	}
>  }
>  


-- 
Damien Le Moal
Western Digital Research



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux