| Message ID | 1381929675-26165-4-git-send-email-p.marczak@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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? > - tmp_capacity = (long long int)((ums_dev->offset + ums_dev->part_size) > - * SECTOR_SIZE); > - *capacity = ums_dev->mmc->capacity - tmp_capacity; > + 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? > + 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
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?
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). 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.
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
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; + int64_t ums_capacity = (int64_t)ums_dev->part_size * SECTOR_SIZE; + int64_t ums_offset = (int64_t)ums_dev->offset * SECTOR_SIZE; - tmp_capacity = (long long int)((ums_dev->offset + ums_dev->part_size) - * SECTOR_SIZE); - *capacity = ums_dev->mmc->capacity - tmp_capacity; + if (ums_capacity && ((ums_capacity + ums_offset) < mmc_capacity)) + *capacity = ums_capacity; + else + *capacity = mmc_capacity - ums_offset; + + printf("UMS: partition capacity: %#llx blocks\n" + "UMS: partition start block: %#x\n", + *capacity / SECTOR_SIZE, + ums_dev->offset); } static struct ums ums_dev = {
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(-)