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 p.marczak@samsung.com Cc: Marek Vasut marex@denx.de
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?