Hello Marek,
On 10/19/2013 02:57 AM, Marek Vasut wrote:
Dear Przemyslaw Marczak,
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?
I think the first screaming thing here is ... why is this all multiplied by SECTOR_SIZE before doing the comparisons and stuffs ? You can do that later (that does mean do it later, yes).
You're right, but actually we don't need to use real card capacity but only sector count. Patch v2 will include this.
Try this:
u64 mmc_cap = ums_dev->mmc->capacity / SECTOR_SIZE; u64 ums_start = ums_dev->offset; u64 ums_end = ums_start + ums_dev->part_size;
/* Start past MMC size. */ if (ums_start >= mmc_cap) return -EINVAL;
/* End past MMC size. */ if (ums_end > mmc_cap) { puts("UMS region larger than MMC device, capping\n"); ums_end = mmc_cap; }
*capacity = (ums_end - ums_start) * SECTOR_SIZE;
Does this work? You'd need to add debug.
It will only work if UMS_PART_SIZE and UMS_START_BLOCK are set correctly. In default case when both values are defined as 0 - function returns null capacity but we don't want this.
Patch v2 will include cases for default, valid and bad definitions of UMS_PART_SIZE and UMS_START_BLOCK. I will also remove unnecessary code around capacity validation from ums gadget driver. Next patch set will be send soon.
Regards