[PATCH v2] tools: ensure zeroed padding in external FIT images
Padding the header of an external FIT image is achieved by truncating the existing temporary FIT file to match the required alignment before appending image data. Reusing an existing file this way means that the padding will likely contain a portion of the original data not overwritten by the new header.
Zero out any data past the end of the new header, and stop at either the end of the desired padding, or the end of the old FIT file, whichever comes first.
Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with external data"") Signed-off-by: Roman Azarenko roman.azarenko@iopsys.eu --- v2: - Use memset() on `fdt` instead of ftruncate() on `fd`, zero only the part that will become padding in the new FIT file. v1: https://lists.denx.de/pipermail/u-boot/2023-August/528213.html
I'm trying to be a bit smart here and only zero out the amount of padding that we actually need, but no more than the total length of the old FIT file (not to cross the boundary of the mmaped region).
See v1 for info on how to reproduce the original issue. --- tools/fit_image.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/fit_image.c b/tools/fit_image.c index 9fe69ea0d9f8..d9da0ce8388f 100644 --- a/tools/fit_image.c +++ b/tools/fit_image.c @@ -497,7 +497,7 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) { void *buf = NULL; int buf_ptr; - int fit_size, new_size; + int fit_size, unpadded_size, new_size, pad_boundary; int fd; struct stat sbuf; void *fdt; @@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) /* Pack the FDT and place the data after it */ fdt_pack(fdt);
- new_size = fdt_totalsize(fdt); - new_size = ALIGN(new_size, align_size); + unpadded_size = fdt_totalsize(fdt); + new_size = ALIGN(unpadded_size, align_size); fdt_set_totalsize(fdt, new_size); + if (unpadded_size < fit_size) { + pad_boundary = new_size < fit_size ? new_size : fit_size; + memset(fdt + unpadded_size, 0, pad_boundary - unpadded_size); + } debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt)); debug("External data size %x\n", buf_ptr); munmap(fdt, sbuf.st_size);
On Fri, 25 Aug 2023 at 02:10, Roman Azarenko roman.azarenko@iopsys.eu wrote:
Padding the header of an external FIT image is achieved by truncating the existing temporary FIT file to match the required alignment before appending image data. Reusing an existing file this way means that the padding will likely contain a portion of the original data not overwritten by the new header.
Zero out any data past the end of the new header, and stop at either the end of the desired padding, or the end of the old FIT file, whichever comes first.
Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with external data"") Signed-off-by: Roman Azarenko roman.azarenko@iopsys.eu
v2:
- Use memset() on `fdt` instead of ftruncate() on `fd`, zero only the part that will become padding in the new FIT file.
v1: https://lists.denx.de/pipermail/u-boot/2023-August/528213.html
I'm trying to be a bit smart here and only zero out the amount of padding that we actually need, but no more than the total length of the old FIT file (not to cross the boundary of the mmaped region).
See v1 for info on how to reproduce the original issue.
tools/fit_image.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/fit_image.c b/tools/fit_image.c index 9fe69ea0d9f8..d9da0ce8388f 100644 --- a/tools/fit_image.c +++ b/tools/fit_image.c @@ -497,7 +497,7 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) { void *buf = NULL; int buf_ptr;
int fit_size, new_size;
int fit_size, unpadded_size, new_size, pad_boundary; int fd; struct stat sbuf; void *fdt;@@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) /* Pack the FDT and place the data after it */ fdt_pack(fdt);
new_size = fdt_totalsize(fdt);new_size = ALIGN(new_size, align_size);
unpadded_size = fdt_totalsize(fdt);new_size = ALIGN(unpadded_size, align_size); fdt_set_totalsize(fdt, new_size);
I didn't know that was allowed...I thought it needed fdt_open_into() ?
if (unpadded_size < fit_size) {pad_boundary = new_size < fit_size ? new_size : fit_size;memset(fdt + unpadded_size, 0, pad_boundary - unpadded_size);} debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt)); debug("External data size %x\n", buf_ptr); munmap(fdt, sbuf.st_size);-- 2.42.0
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
@@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) /* Pack the FDT and place the data after it */ fdt_pack(fdt);
- new_size = fdt_totalsize(fdt); - new_size = ALIGN(new_size, align_size); + unpadded_size = fdt_totalsize(fdt); + new_size = ALIGN(unpadded_size, align_size); fdt_set_totalsize(fdt, new_size);
I didn't know that was allowed...I thought it needed fdt_open_into() ?
The introduction of fdt_set_totalsize() comes from commit ebfe611be91e ("mkimage: fit_image: Add option to make fit header align"). The commit message doesn't describe the choice of this function vs fdt_open_into().
Personally I'm unable to definitively comment on it. I can only blindly guess, that because we're only changing the total length of the fdt struct, and keeping all other fields the same, we don't need to allocate a new fdt struct with a different size.
Hi Roman,
On Mon, 28 Aug 2023 at 02:00, Roman Azarenko roman.azarenko@iopsys.eu wrote:
On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
@@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) /* Pack the FDT and place the data after it */ fdt_pack(fdt);
new_size = fdt_totalsize(fdt);new_size = ALIGN(new_size, align_size);
unpadded_size = fdt_totalsize(fdt);new_size = ALIGN(unpadded_size, align_size); fdt_set_totalsize(fdt, new_size);I didn't know that was allowed...I thought it needed fdt_open_into() ?
The introduction of fdt_set_totalsize() comes from commit ebfe611be91e ("mkimage: fit_image: Add option to make fit header align"). The commit message doesn't describe the choice of this function vs fdt_open_into().
Personally I'm unable to definitively comment on it. I can only blindly guess, that because we're only changing the total length of the fdt struct, and keeping all other fields the same, we don't need to allocate a new fdt struct with a different size.
I am not sure if it would cause problems. I do understand that you didn't write the code. You could copy the people who did (and those that reviewed it) for their input.
But I think it should change to call fdt_open_into()...if you look at that function it does extra things. Unfortunately, the function has no comment in libfdt.h.
Could you add another patch before or after this one?
Regards, Simon
On Mon, 28 Aug 2023 at 02:00, Roman Azarenko
roman.azarenko@iopsys.eu wrote:
On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
@@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) /* Pack the FDT and place the data after it */ fdt_pack(fdt);
- new_size = fdt_totalsize(fdt); - new_size = ALIGN(new_size, align_size); + unpadded_size = fdt_totalsize(fdt); + new_size = ALIGN(unpadded_size, align_size); fdt_set_totalsize(fdt, new_size);
I didn't know that was allowed...I thought it needed fdt_open_into() ?
The introduction of fdt_set_totalsize() comes from commit ebfe611be91e ("mkimage: fit_image: Add option to make fit header align"). The commit message doesn't describe the choice of this function vs fdt_open_into().
Personally I'm unable to definitively comment on it. I can only blindly guess, that because we're only changing the total length of the fdt struct, and keeping all other fields the same, we don't need to allocate a new fdt struct with a different size.
I am not sure if it would cause problems. I do understand that you didn't write the code. You could copy the people who did (and those that reviewed it) for their input.
@Kever, @Punit, @Tom, could you please comment on this remark by Simon? Thank you!
Could you add another patch before or after this one?
I'll look into this as well, and see if I can figure it out and then post v3 of the patchset. Thanks!
On Thu, Aug 31, 2023 at 09:39:52AM +0000, Roman Azarenko wrote:
On Mon, 28 Aug 2023 at 02:00, Roman Azarenko
roman.azarenko@iopsys.eu wrote:
On Fri, 2023-08-25 at 12:06 -0600, Simon Glass wrote:
@@ -564,9 +564,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) /* Pack the FDT and place the data after it */ fdt_pack(fdt);
- new_size = fdt_totalsize(fdt); - new_size = ALIGN(new_size, align_size); + unpadded_size = fdt_totalsize(fdt); + new_size = ALIGN(unpadded_size, align_size); fdt_set_totalsize(fdt, new_size);
I didn't know that was allowed...I thought it needed fdt_open_into() ?
The introduction of fdt_set_totalsize() comes from commit ebfe611be91e ("mkimage: fit_image: Add option to make fit header align"). The commit message doesn't describe the choice of this function vs fdt_open_into().
Personally I'm unable to definitively comment on it. I can only blindly guess, that because we're only changing the total length of the fdt struct, and keeping all other fields the same, we don't need to allocate a new fdt struct with a different size.
I am not sure if it would cause problems. I do understand that you didn't write the code. You could copy the people who did (and those that reviewed it) for their input.
@Kever, @Punit, @Tom, could you please comment on this remark by Simon? Thank you!
I assume that we're given an already well-ordered device tree at this point and so that's why we don't use fdt_open_into to move things and just handle it ourselves at that point in the tool.
On Fri, Aug 25, 2023 at 10:10:14AM +0200, Roman Azarenko wrote:
Padding the header of an external FIT image is achieved by truncating the existing temporary FIT file to match the required alignment before appending image data. Reusing an existing file this way means that the padding will likely contain a portion of the original data not overwritten by the new header.
Zero out any data past the end of the new header, and stop at either the end of the desired padding, or the end of the old FIT file, whichever comes first.
Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with external data"") Signed-off-by: Roman Azarenko roman.azarenko@iopsys.eu Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (3)
-
Roman Azarenko -
Simon Glass -
Tom Rini