On Tue, Sep 20, 2016 at 11:04:40AM +0900, Jaehoon Chung wrote: > 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 <http://www.coverity.com> > >>>> > >>>> 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 <haibo.c...@nxp.com> > >>> > >>> 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?
... I think? I hadn't had my coffee yet when I did a quick compile test this morning but it looks like all of the uses of capacity would fit into a u8. Someone should check and make a formal patch :) -- Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
