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

login
register
mail settings
Submitter Przemyslaw Marczak
Date Oct. 16, 2013, 1:21 p.m.
Message ID <1381929675-26165-4-git-send-email-p.marczak@samsung.com>
Download mbox | patch
Permalink /patch/283943/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Przemyslaw Marczak - Oct. 16, 2013, 1:21 p.m.
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(-)
Marek Vasut - Oct. 17, 2013, 5:41 p.m.
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
Przemyslaw Marczak - Oct. 18, 2013, 3:05 p.m.
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?
Marek Vasut - Oct. 19, 2013, 12:57 a.m.
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.
Przemyslaw Marczak - Oct. 22, 2013, 11:04 a.m.
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

Patch

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 = {