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

Marek Vasut
2013-10-19T00:57:20Z

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.