[PATCH] scsi: mvumi: fix 32 bit shift of a u32 value

[PATCH] scsi: mvumi: fix 32 bit shift of a u32 value

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Currently the top 32 bits of a 64 bit address is being calculated
by shifting a u32 twice by 16 bits and then being cast into a 64
bit address.  Shifting a u32 twice by 16 bits always ends up with
a zero.  Fix this by casting the u32 to a 64 bit address first
and then shifting it 32 bits.

Addresses-Coverity: ("Operands don't affect result")
Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/scsi/mvumi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 8906aceda4c4..62df69f1e71e 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -296,7 +296,7 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
 			sgd_getsz(mhba, m_sg, size);
 
 			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
-				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
+				   (dma_addr_t) m_sg->baseaddr_h << 32;
 
 			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
 								phy_addr);
-- 
2.20.1

Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value

[walter harms]

Am 13.08.2019 20:01, schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the top 32 bits of a 64 bit address is being calculated
> by shifting a u32 twice by 16 bits and then being cast into a 64
> bit address.  Shifting a u32 twice by 16 bits always ends up with
> a zero.  Fix this by casting the u32 to a 64 bit address first
> and then shifting it 32 bits.
> 
> Addresses-Coverity: ("Operands don't affect result")
> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/scsi/mvumi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8906aceda4c4..62df69f1e71e 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -296,7 +296,7 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
>  			sgd_getsz(mhba, m_sg, size);
>  
>  			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
> -				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
> +				   (dma_addr_t) m_sg->baseaddr_h << 32;
>  

All the casts make it hard to read, i would propose an alternativ version:
phy_addr = m_sg->baseaddr_h;
phy_addr <<= 32;
phy_addr |= m_sg->baseaddr_l;

JM2C and totaly untested.

re,
 wh

>  			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
>  								phy_addr);

Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value

[Dan Carpenter]

On Tue, Aug 13, 2019 at 07:01:13PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the top 32 bits of a 64 bit address is being calculated
> by shifting a u32 twice by 16 bits and then being cast into a 64
> bit address.  Shifting a u32 twice by 16 bits always ends up with
> a zero.  Fix this by casting the u32 to a 64 bit address first
> and then shifting it 32 bits.
> 
> Addresses-Coverity: ("Operands don't affect result")
> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/scsi/mvumi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8906aceda4c4..62df69f1e71e 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -296,7 +296,7 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
>  			sgd_getsz(mhba, m_sg, size);
>  
>  			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
> -				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
> +				   (dma_addr_t) m_sg->baseaddr_h << 32;

Colin, you've sent this patch before on Feb 16, 2019.  If you shift by
32 then it's undefined behavior on 32 bit systems.  The correct fix is
to move the cast which was what your original patch did actually.

			(((dma_addr_t)m_sg->baseaddr_h << 16) << 16);

My suggestion back then was to introduce a macro:

/*
 * The dma_addr_t type can be either 32 or 64 bit.  Left shifting a 32
 * bit number is undefined so this do two 16 bit left shifts.
 *
 */
#define DMA_LSHIFT_32(val) (((dma_addr_t)(val) << 16) << 16)

regards,
dan carpenter

Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

Hi Colin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc4 next-20190814]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Colin-King/scsi-mvumi-fix-32-bit-shift-of-a-u32-value/20190814-214025
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/scsi/mvumi.c: In function 'mvumi_delete_internal_cmd':
>> drivers/scsi/mvumi.c:299:38: warning: left shift count >= width of type [-Wshift-count-overflow]
           (dma_addr_t) m_sg->baseaddr_h << 32;
                                         ^~

vim +299 drivers/scsi/mvumi.c

   285	
   286	static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
   287							struct mvumi_cmd *cmd)
   288	{
   289		struct mvumi_sgl *m_sg;
   290		unsigned int size;
   291		dma_addr_t phy_addr;
   292	
   293		if (cmd && cmd->frame) {
   294			if (cmd->frame->sg_counts) {
   295				m_sg = (struct mvumi_sgl *) &cmd->frame->payload[0];
   296				sgd_getsz(mhba, m_sg, size);
   297	
   298				phy_addr = (dma_addr_t) m_sg->baseaddr_l |
 > 299					   (dma_addr_t) m_sg->baseaddr_h << 32;
   300	
   301				dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
   302									phy_addr);
   303			}
   304			dma_free_coherent(&mhba->pdev->dev, mhba->ib_max_size,
   305					cmd->frame, cmd->frame_phys);
   306			kfree(cmd);
   307		}
   308	}
   309	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61472 bytes --]