[PATCH] fastboot: Fix overflow when calculating chunk size

[PATCH] fastboot: Fix overflow when calculating chunk size

[Sean Anderson]

If a chunk was larger than 4GiB, then chunk_data_sz would overflow and
blkcnt would not be calculated correctly. Upgrade it to a u64 and cast
its multiplicands as well. Also fix bytes_written while we're at it.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 lib/image-sparse.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/image-sparse.c b/lib/image-sparse.c
index 187ac28cd3..52c8dcc08c 100644
--- a/lib/image-sparse.c
+++ b/lib/image-sparse.c
@@ -55,10 +55,10 @@ int write_sparse_image(struct sparse_storage *info,
 	lbaint_t blk;
 	lbaint_t blkcnt;
 	lbaint_t blks;
-	uint32_t bytes_written = 0;
+	uint64_t bytes_written = 0;
 	unsigned int chunk;
 	unsigned int offset;
-	unsigned int chunk_data_sz;
+	uint64_t chunk_data_sz;
 	uint32_t *fill_buf = NULL;
 	uint32_t fill_val;
 	sparse_header_t *sparse_header;
@@ -132,7 +132,7 @@ int write_sparse_image(struct sparse_storage *info,
 				 sizeof(chunk_header_t));
 		}
 
-		chunk_data_sz = sparse_header->blk_sz * chunk_header->chunk_sz;
+		chunk_data_sz = ((u64)sparse_header->blk_sz) * chunk_header->chunk_sz;
 		blkcnt = chunk_data_sz / info->blksz;
 		switch (chunk_header->chunk_type) {
 		case CHUNK_TYPE_RAW:
@@ -162,7 +162,7 @@ int write_sparse_image(struct sparse_storage *info,
 				return -1;
 			}
 			blk += blks;
-			bytes_written += blkcnt * info->blksz;
+			bytes_written += ((u64)blkcnt) * info->blksz;
 			total_blocks += chunk_header->chunk_sz;
 			data += chunk_data_sz;
 			break;
@@ -222,7 +222,7 @@ int write_sparse_image(struct sparse_storage *info,
 				blk += blks;
 				i += j;
 			}
-			bytes_written += blkcnt * info->blksz;
+			bytes_written += ((u64)blkcnt) * info->blksz;
 			total_blocks += chunk_data_sz / sparse_header->blk_sz;
 			free(fill_buf);
 			break;
@@ -253,7 +253,7 @@ int write_sparse_image(struct sparse_storage *info,
 
 	debug("Wrote %d blocks, expected to write %d blocks\n",
 	      total_blocks, sparse_header->total_blks);
-	printf("........ wrote %u bytes to '%s'\n", bytes_written, part_name);
+	printf("........ wrote %llu bytes to '%s'\n", bytes_written, part_name);
 
 	if (total_blocks != sparse_header->total_blks) {
 		info->mssg("sparse image write failure", response);
-- 
2.25.1

[PATCH] fastboot: Fix overflow when calculating chunk size

[Heiko Schocher]

Hello Sean,

On 16.04.21 23:58, Sean Anderson wrote:
> If a chunk was larger than 4GiB, then chunk_data_sz would overflow and
> blkcnt would not be calculated correctly. Upgrade it to a u64 and cast
> its multiplicands as well. Also fix bytes_written while we're at it.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  lib/image-sparse.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Wow chunk size larger than 4G!

But patch seems valid, so

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH] fastboot: Fix overflow when calculating chunk size

[Sean Anderson]

On 4/19/21 12:31 AM, Heiko Schocher wrote:
 > Hello Sean,
 >
 > On 16.04.21 23:58, Sean Anderson wrote:
 >> If a chunk was larger than 4GiB, then chunk_data_sz would overflow and
 >> blkcnt would not be calculated correctly. Upgrade it to a u64 and cast
 >> its multiplicands as well. Also fix bytes_written while we're at it.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >>   lib/image-sparse.c | 12 ++++++------
 >>   1 file changed, 6 insertions(+), 6 deletions(-)
 >
 > Wow chunk size larger than 4G!

Normally filesystems like ext4 put data all around the disk (i.e. the
used-block bitmap). However, if you use a log filesystem (like F2FS),
then all the data is at the beginning of the disk. In that case, it is
fairly easy to end up with a 4GiB or greater "don't care" chunk taking
up most of the disk.

--Sean

 >
 > But patch seems valid, so
 >
 > Reviewed-by: Heiko Schocher <hs@denx.de>
 >
 > bye,
 > Heiko
 >

[PATCH] fastboot: Fix overflow when calculating chunk size

[Sean Anderson]

Hi Lukasz,

Can this make it into 2020.07? Thanks,

--Sean

On 4/16/21 5:58 PM, Sean Anderson wrote:
> If a chunk was larger than 4GiB, then chunk_data_sz would overflow and
> blkcnt would not be calculated correctly. Upgrade it to a u64 and cast
> its multiplicands as well. Also fix bytes_written while we're at it.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>   lib/image-sparse.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
> index 187ac28cd3..52c8dcc08c 100644
> --- a/lib/image-sparse.c
> +++ b/lib/image-sparse.c
> @@ -55,10 +55,10 @@ int write_sparse_image(struct sparse_storage *info,
>   	lbaint_t blk;
>   	lbaint_t blkcnt;
>   	lbaint_t blks;
> -	uint32_t bytes_written = 0;
> +	uint64_t bytes_written = 0;
>   	unsigned int chunk;
>   	unsigned int offset;
> -	unsigned int chunk_data_sz;
> +	uint64_t chunk_data_sz;
>   	uint32_t *fill_buf = NULL;
>   	uint32_t fill_val;
>   	sparse_header_t *sparse_header;
> @@ -132,7 +132,7 @@ int write_sparse_image(struct sparse_storage *info,
>   				 sizeof(chunk_header_t));
>   		}
>   
> -		chunk_data_sz = sparse_header->blk_sz * chunk_header->chunk_sz;
> +		chunk_data_sz = ((u64)sparse_header->blk_sz) * chunk_header->chunk_sz;
>   		blkcnt = chunk_data_sz / info->blksz;
>   		switch (chunk_header->chunk_type) {
>   		case CHUNK_TYPE_RAW:
> @@ -162,7 +162,7 @@ int write_sparse_image(struct sparse_storage *info,
>   				return -1;
>   			}
>   			blk += blks;
> -			bytes_written += blkcnt * info->blksz;
> +			bytes_written += ((u64)blkcnt) * info->blksz;
>   			total_blocks += chunk_header->chunk_sz;
>   			data += chunk_data_sz;
>   			break;
> @@ -222,7 +222,7 @@ int write_sparse_image(struct sparse_storage *info,
>   				blk += blks;
>   				i += j;
>   			}
> -			bytes_written += blkcnt * info->blksz;
> +			bytes_written += ((u64)blkcnt) * info->blksz;
>   			total_blocks += chunk_data_sz / sparse_header->blk_sz;
>   			free(fill_buf);
>   			break;
> @@ -253,7 +253,7 @@ int write_sparse_image(struct sparse_storage *info,
>   
>   	debug("Wrote %d blocks, expected to write %d blocks\n",
>   	      total_blocks, sparse_header->total_blks);
> -	printf("........ wrote %u bytes to '%s'\n", bytes_written, part_name);
> +	printf("........ wrote %llu bytes to '%s'\n", bytes_written, part_name);
>   
>   	if (total_blocks != sparse_header->total_blks) {
>   		info->mssg("sparse image write failure", response);
> 

Re: [PATCH] fastboot: Fix overflow when calculating chunk size

[Sean Anderson]

On 5/13/21 11:54 AM, Sean Anderson wrote:
> Hi Lukasz,
> 
> Can this make it into 2020.07? Thanks,

ping? Should Tom pick this up instead?

--Sean

> 
> --Sean
> 
> On 4/16/21 5:58 PM, Sean Anderson wrote:
>> If a chunk was larger than 4GiB, then chunk_data_sz would overflow and
>> blkcnt would not be calculated correctly. Upgrade it to a u64 and cast
>> its multiplicands as well. Also fix bytes_written while we're at it.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>>   lib/image-sparse.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
>> index 187ac28cd3..52c8dcc08c 100644
>> --- a/lib/image-sparse.c
>> +++ b/lib/image-sparse.c
>> @@ -55,10 +55,10 @@ int write_sparse_image(struct sparse_storage *info,
>>       lbaint_t blk;
>>       lbaint_t blkcnt;
>>       lbaint_t blks;
>> -    uint32_t bytes_written = 0;
>> +    uint64_t bytes_written = 0;
>>       unsigned int chunk;
>>       unsigned int offset;
>> -    unsigned int chunk_data_sz;
>> +    uint64_t chunk_data_sz;
>>       uint32_t *fill_buf = NULL;
>>       uint32_t fill_val;
>>       sparse_header_t *sparse_header;
>> @@ -132,7 +132,7 @@ int write_sparse_image(struct sparse_storage *info,
>>                    sizeof(chunk_header_t));
>>           }
>> -        chunk_data_sz = sparse_header->blk_sz * chunk_header->chunk_sz;
>> +        chunk_data_sz = ((u64)sparse_header->blk_sz) * chunk_header->chunk_sz;
>>           blkcnt = chunk_data_sz / info->blksz;
>>           switch (chunk_header->chunk_type) {
>>           case CHUNK_TYPE_RAW:
>> @@ -162,7 +162,7 @@ int write_sparse_image(struct sparse_storage *info,
>>                   return -1;
>>               }
>>               blk += blks;
>> -            bytes_written += blkcnt * info->blksz;
>> +            bytes_written += ((u64)blkcnt) * info->blksz;
>>               total_blocks += chunk_header->chunk_sz;
>>               data += chunk_data_sz;
>>               break;
>> @@ -222,7 +222,7 @@ int write_sparse_image(struct sparse_storage *info,
>>                   blk += blks;
>>                   i += j;
>>               }
>> -            bytes_written += blkcnt * info->blksz;
>> +            bytes_written += ((u64)blkcnt) * info->blksz;
>>               total_blocks += chunk_data_sz / sparse_header->blk_sz;
>>               free(fill_buf);
>>               break;
>> @@ -253,7 +253,7 @@ int write_sparse_image(struct sparse_storage *info,
>>       debug("Wrote %d blocks, expected to write %d blocks\n",
>>             total_blocks, sparse_header->total_blks);
>> -    printf("........ wrote %u bytes to '%s'\n", bytes_written, part_name);
>> +    printf("........ wrote %llu bytes to '%s'\n", bytes_written, part_name);
>>       if (total_blocks != sparse_header->total_blks) {
>>           info->mssg("sparse image write failure", response);
>>

Re: [PATCH] fastboot: Fix overflow when calculating chunk size

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

Hi Sean,

> On 5/13/21 11:54 AM, Sean Anderson wrote:
> > Hi Lukasz,
> > 
> > Can this make it into 2020.07? Thanks,  
> 
> ping? Should Tom pick this up instead?
> 

Yes, Tom please pick it up - as I will not prepare PR sooner than June.

> --Sean
> 
> > 
> > --Sean
> > 
> > On 4/16/21 5:58 PM, Sean Anderson wrote:  
> >> If a chunk was larger than 4GiB, then chunk_data_sz would overflow
> >> and blkcnt would not be calculated correctly. Upgrade it to a u64
> >> and cast its multiplicands as well. Also fix bytes_written while
> >> we're at it.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> ---
> >>
> >>   lib/image-sparse.c | 12 ++++++------
> >>   1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
> >> index 187ac28cd3..52c8dcc08c 100644
> >> --- a/lib/image-sparse.c
> >> +++ b/lib/image-sparse.c
> >> @@ -55,10 +55,10 @@ int write_sparse_image(struct sparse_storage
> >> *info, lbaint_t blk;
> >>       lbaint_t blkcnt;
> >>       lbaint_t blks;
> >> -    uint32_t bytes_written = 0;
> >> +    uint64_t bytes_written = 0;
> >>       unsigned int chunk;
> >>       unsigned int offset;
> >> -    unsigned int chunk_data_sz;
> >> +    uint64_t chunk_data_sz;
> >>       uint32_t *fill_buf = NULL;
> >>       uint32_t fill_val;
> >>       sparse_header_t *sparse_header;
> >> @@ -132,7 +132,7 @@ int write_sparse_image(struct sparse_storage
> >> *info, sizeof(chunk_header_t));
> >>           }
> >> -        chunk_data_sz = sparse_header->blk_sz *
> >> chunk_header->chunk_sz;
> >> +        chunk_data_sz = ((u64)sparse_header->blk_sz) *
> >> chunk_header->chunk_sz; blkcnt = chunk_data_sz / info->blksz;
> >>           switch (chunk_header->chunk_type) {
> >>           case CHUNK_TYPE_RAW:
> >> @@ -162,7 +162,7 @@ int write_sparse_image(struct sparse_storage
> >> *info, return -1;
> >>               }
> >>               blk += blks;
> >> -            bytes_written += blkcnt * info->blksz;
> >> +            bytes_written += ((u64)blkcnt) * info->blksz;
> >>               total_blocks += chunk_header->chunk_sz;
> >>               data += chunk_data_sz;
> >>               break;
> >> @@ -222,7 +222,7 @@ int write_sparse_image(struct sparse_storage
> >> *info, blk += blks;
> >>                   i += j;
> >>               }
> >> -            bytes_written += blkcnt * info->blksz;
> >> +            bytes_written += ((u64)blkcnt) * info->blksz;
> >>               total_blocks += chunk_data_sz /
> >> sparse_header->blk_sz; free(fill_buf);
> >>               break;
> >> @@ -253,7 +253,7 @@ int write_sparse_image(struct sparse_storage
> >> *info, debug("Wrote %d blocks, expected to write %d blocks\n",
> >>             total_blocks, sparse_header->total_blks);
> >> -    printf("........ wrote %u bytes to '%s'\n", bytes_written,
> >> part_name);
> >> +    printf("........ wrote %llu bytes to '%s'\n", bytes_written,
> >> part_name); if (total_blocks != sparse_header->total_blks) {
> >>           info->mssg("sparse image write failure", response);
> >>  



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] fastboot: Fix overflow when calculating chunk size

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]

On Fri, Apr 16, 2021 at 05:58:21PM -0400, Sean Anderson wrote:

> If a chunk was larger than 4GiB, then chunk_data_sz would overflow and
> blkcnt would not be calculated correctly. Upgrade it to a u64 and cast
> its multiplicands as well. Also fix bytes_written while we're at it.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Heiko Schocher <hs@denx.de>
> ---
> 
>  lib/image-sparse.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
> index 187ac28cd3..52c8dcc08c 100644
> --- a/lib/image-sparse.c
> +++ b/lib/image-sparse.c
> @@ -55,10 +55,10 @@ int write_sparse_image(struct sparse_storage *info,
>  	lbaint_t blk;
>  	lbaint_t blkcnt;
>  	lbaint_t blks;
> -	uint32_t bytes_written = 0;
> +	uint64_t bytes_written = 0;
>  	unsigned int chunk;
>  	unsigned int offset;
> -	unsigned int chunk_data_sz;
> +	uint64_t chunk_data_sz;
>  	uint32_t *fill_buf = NULL;
>  	uint32_t fill_val;
>  	sparse_header_t *sparse_header;
> @@ -132,7 +132,7 @@ int write_sparse_image(struct sparse_storage *info,
>  				 sizeof(chunk_header_t));
>  		}
>  
> -		chunk_data_sz = sparse_header->blk_sz * chunk_header->chunk_sz;
> +		chunk_data_sz = ((u64)sparse_header->blk_sz) * chunk_header->chunk_sz;
>  		blkcnt = chunk_data_sz / info->blksz;
>  		switch (chunk_header->chunk_type) {
>  		case CHUNK_TYPE_RAW:
> @@ -162,7 +162,7 @@ int write_sparse_image(struct sparse_storage *info,
>  				return -1;
>  			}
>  			blk += blks;
> -			bytes_written += blkcnt * info->blksz;
> +			bytes_written += ((u64)blkcnt) * info->blksz;
>  			total_blocks += chunk_header->chunk_sz;
>  			data += chunk_data_sz;
>  			break;
> @@ -222,7 +222,7 @@ int write_sparse_image(struct sparse_storage *info,
>  				blk += blks;
>  				i += j;
>  			}
> -			bytes_written += blkcnt * info->blksz;
> +			bytes_written += ((u64)blkcnt) * info->blksz;
>  			total_blocks += chunk_data_sz / sparse_header->blk_sz;
>  			free(fill_buf);
>  			break;
> @@ -253,7 +253,7 @@ int write_sparse_image(struct sparse_storage *info,
>  
>  	debug("Wrote %d blocks, expected to write %d blocks\n",
>  	      total_blocks, sparse_header->total_blks);
> -	printf("........ wrote %u bytes to '%s'\n", bytes_written, part_name);
> +	printf("........ wrote %llu bytes to '%s'\n", bytes_written, part_name);
>  
>  	if (total_blocks != sparse_header->total_blks) {
>  		info->mssg("sparse image write failure", response);

This results in things like:
            pico-dwarf-imx7d: all +506 bss +48 rodata +2 text +456
               u-boot: add: 1/0, grow: 1/0 bytes: 452/0 (452)
                 function                                   old     new   delta
                 __aeabi_uldivmod                             -     392    +392
                 write_sparse_image                         712     772     +60

Which I believe means that some of the division above needs to be
converted to use do_div().  Since I can't easily confirm the changes,
can you please check in to it?  Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]