From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaehoon Chung Date: Tue, 20 Sep 2016 11:04:40 +0900 Subject: [U-Boot] [PATCH] mmc: cat u8 to u64 to avoid unexpected error In-Reply-To: <20160919113020.GU29602@bill-the-cat> References: <1473755277-23489-1-git-send-email-haibo.chen@nxp.com> <20160918175354.GS8156@bill-the-cat> <06cf3c53-ffa9-9cd5-75da-8abebb882d04@samsung.com> <20160919113020.GU29602@bill-the-cat> Message-ID: <83bb4eea-91eb-ca64-79e7-e81ab67add94@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 09/19/2016 08:30 PM, Tom Rini wrote: > On Mon, Sep 19, 2016 at 03:31:54PM +0900, Jaehoon Chung wrote: >> On 09/19/2016 02:53 AM, Tom Rini wrote: >>> On Tue, Sep 13, 2016 at 04:27:57PM +0800, Haibo Chen wrote: >>> >>>> Suspicious implicit sign extension exist. ext_csd[] is defined >>>> as "u8", capacity is defined as u64, so u8 is promoted to signed >>>> int first int the "|" expersion, then the sign extended to u64. >>>> if the tmp sign value is largeer than 0x7fffffff, after the sign >>>> extension, the upper bits of the result will all be 1. >>>> Thanks to coverity >>>> >>>> e.g. >>>> u8 data_8; >>>> u64 data_64; >>>> >>>> data_8 = 0x80; >>>> data_64 = data_8 << 24; //0xffffffff80000000 >>>> data_64 = ((u64)data_8) << 24; //0x80000000 >>>> >>>> Signed-off-by: Haibo Chen >>> >>> Please add a 'Reported-by: Coverity' and you can include the CID if you >>> like. >> >> I think cid doesn't need to change type. > > I mean the coverity CID :) In the public coverity project it's 149300 Ah! I misunderstood CID as cid register. :) > >> >>> >>>> --- >>>> drivers/mmc/mmc.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c >>>> index 43ea0bb..c1d1dc6 100644 >>>> --- a/drivers/mmc/mmc.c >>>> +++ b/drivers/mmc/mmc.c >>>> @@ -1176,10 +1176,10 @@ static int mmc_startup(struct mmc *mmc) >>>> * ext_csd's capacity is valid if the value is more >>>> * than 2GB >>>> */ >>>> - capacity = ext_csd[EXT_CSD_SEC_CNT] << 0 >>>> - | ext_csd[EXT_CSD_SEC_CNT + 1] << 8 >>>> - | ext_csd[EXT_CSD_SEC_CNT + 2] << 16 >>>> - | ext_csd[EXT_CSD_SEC_CNT + 3] << 24; >>>> + capacity = ((u64)ext_csd[EXT_CSD_SEC_CNT]) << 0 >>>> + | ((u64)ext_csd[EXT_CSD_SEC_CNT + 1]) << 8 >>>> + | ((u64)ext_csd[EXT_CSD_SEC_CNT + 2]) << 16 >>>> + | ((u64)ext_csd[EXT_CSD_SEC_CNT + 3]) << 24; >>>> capacity *= MMC_MAX_BLOCK_LEN; >>>> if ((capacity >> 20) > 2 * 1024) >>>> mmc->capacity_user = capacity; >>> >>> Can't we just move capacity down to a u8 instead? Thanks! >> >> Maybe not to move down to a u8..because it's displayed the real capacity for storage. > > We could update those lines too? It's just that one case right there, > yes? If it's possible to calculate the correct capacity? Best Regards, Jaehoon Chung > >> And i wonder that coverity didn't report about the line 1294? > > It does, along with 1256. > > Thanks! >