[U-Boot] [PATCH 3/4] usb: ums: fix bug in partition capacity computation.

[Przemyslaw Marczak <p.marczak@samsung.com>]

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?

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com