From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Fri, 18 Oct 2013 17:05:45 +0200 Subject: [U-Boot] [PATCH 3/4] usb: ums: fix bug in partition capacity computation. In-Reply-To: <201310171941.20573.marex@denx.de> References: <1381929675-26165-1-git-send-email-p.marczak@samsung.com> <1381929675-26165-4-git-send-email-p.marczak@samsung.com> <201310171941.20573.marex@denx.de> Message-ID: <52614E49.8010109@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 Hi Marek, On 10/17/2013 07:41 PM, Marek Vasut wrote: > Dear Przemyslaw Marczak, > >> Before this change ums disk capacity was miscalculated because >> of integer overflow. >> >> Signed-off-by: Przemyslaw Marczak >> Cc: Marek Vasut >> --- >> board/samsung/common/ums.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c >> index 1f28590..6c4e6c4 100644 >> --- a/board/samsung/common/ums.c >> +++ b/board/samsung/common/ums.c >> @@ -37,11 +37,19 @@ static int ums_write_sector(struct ums *ums_dev, >> >> static void ums_get_capacity(struct ums *ums_dev, long long int *capacity) >> { >> - long long int tmp_capacity; >> + int64_t mmc_capacity = (int64_t)ums_dev->mmc->capacity; > > Why are these casts here? > >> + int64_t ums_capacity = (int64_t)ums_dev->part_size * SECTOR_SIZE; >> + int64_t ums_offset = (int64_t)ums_dev->offset * SECTOR_SIZE; > > And here all around? And why are these values signed, can there ever be negative > value in them? > I tried to fix it without changes in ums driver because it works fine. Of course capacity can't be a negative value. When we set some offset and some part size we have an integer overflow at this line, just before cast to long long int: >> - tmp_capacity = (long long int)((ums_dev->offset + ums_dev->part_size) >> - * SECTOR_SIZE); >> - *capacity = ums_dev->mmc->capacity - tmp_capacity; In the best case of overflow - ums partition capacity will have the same value as mmc cap, but if offset was set, then the partition size will be exceeded. >> + if (ums_capacity && ((ums_capacity + ums_offset) < mmc_capacity)) >> + *capacity = ums_capacity; >> + else >> + *capacity = mmc_capacity - ums_offset; > > Urgh, what exactly does this code achieve again? This code above avoids situation when tmp_capacity value is bigger than real mmc capacity. I don't check next the offset but this is also the reason why I put printf here. I assume that developer should know how to define UMS_START_BLOCK and UMS_PART_SIZE if no, some information will be printed. > >> + printf("UMS: partition capacity: %#llx blocks\n" >> + "UMS: partition start block: %#x\n", >> + *capacity / SECTOR_SIZE, >> + ums_dev->offset); >> } >> >> static struct ums ums_dev = { > > Best regards, > Marek Vasut > In summary I will change signed variables to unsigned here and few in the ums gadget driver. Moreover now I think that it will be better to replace part_size from the struct ums_dev with part_blk_num and compute its value at ums_init function. And then pointer to ums_get_capacity is not needed in ums structure. What do you think about this? -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com