Message ID | 20210416215821.3575989-1-sean.anderson@seco.com |
---|---|
State | Changes Requested |
Delegated to: | Tom Rini |
Headers | show |
Series | fastboot: Fix overflow when calculating chunk size | expand |
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
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 >
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); >
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); >>
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
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.
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);
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(-)