| Message ID | 20230823121417.270511-1-roman.azarenko@iopsys.eu |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | tools: ensure zeroed padding in external FIT images | expand |
On 8/23/23 14:14, Roman Azarenko wrote: > Padding the header of an external FIT image is achieved by growing the > existing temporary FIT file to match the given alignment requirement > 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. > > Truncate the temporary FIT file to unpadded header length first, so > that the subsequent truncate operation extends the file with null > bytes, making padding predictably zeroed. > > Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with external data"") > Signed-off-by: Roman Azarenko <roman.azarenko@iopsys.eu> > --- > This fix was prompted by an observation that there is unexpected data > in the padding between the FIT header and the first image in a > generated external FIT file. > > Having non-zero data in padding doesn't have any negative practical > consequences, these FIT files work fine just like any other. However > I'm not aware of any reason of making the padding depend on the > contents of an internal/intermediate FIT file. In my opinion, padding > should be predictably zeroed. > > Steps to reproduce: > > 1. Generate a random binary blob representing a fake rootfs image: > > $ dd if=/dev/urandom of=rootfs bs=1M count=1 > > 2. Define a minimalistic FIT source file: > > /dts-v1/; > / { > description = "Padding demo"; > #address-cells = <0x01>; > images { > rootfs { > data = /incbin/("rootfs"); > type = "filesystem"; > compression = "none"; > hash-1 { > algo = "sha256"; > }; > }; > }; > > configurations { > default = "config"; > > config { > rootfs = "rootfs"; > }; > }; > }; > > 3. Build an external FIT image with 4096 bytes of padding: > > $ mkimage -f source.dts -E -B 1000 output.dtb > > Observe how the header is padded to 4096 bytes (0x1000) with a part of > an existing dummy rootfs image, which happened to be in that place in > the internal/intermediate FIT file. > --- > tools/fit_image.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/tools/fit_image.c b/tools/fit_image.c > index 9fe69ea0d9f8..7a7b68f96ed8 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; > int fd; > struct stat sbuf; > void *fdt; > @@ -564,14 +564,15 @@ 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); > 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); > > - if (ftruncate(fd, new_size)) { > + /* Truncate to unpadded size first to ensure zero-filled padding */ > + if (ftruncate(fd, unpadded_size) || ftruncate(fd, new_size)) { > debug("%s: Failed to truncate file: %s\n", __func__, > strerror(errno)); > ret = -EIO; Can we use memset and zero out the trailing part of the buffer instead ?
Hello Marek, On Wed, 2023-08-23 at 20:06 +0200, Marek Vasut wrote: > Can we use memset and zero out the trailing part of the buffer instead > ? That is what I started with, but at the time the ftruncate() approach seemed simpler. Now that you suggested memset yourself, I took a stab at it again. I think I got to the point where I'm happy with the logic, and will submit v2 tomorrow to give some more time if anyone else has more comments on v1. Thank you!
On 8/24/23 12:17, Roman Azarenko wrote: > Hello Marek, > > On Wed, 2023-08-23 at 20:06 +0200, Marek Vasut wrote: >> Can we use memset and zero out the trailing part of the buffer instead >> ? > > That is what I started with, but at the time the ftruncate() approach > seemed simpler. > > Now that you suggested memset yourself, I took a stab at it again. I > think I got to the point where I'm happy with the logic, and will submit > v2 tomorrow to give some more time if anyone else has more comments on > v1. Nice, thanks!
diff --git a/tools/fit_image.c b/tools/fit_image.c index 9fe69ea0d9f8..7a7b68f96ed8 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; int fd; struct stat sbuf; void *fdt; @@ -564,14 +564,15 @@ 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); 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); - if (ftruncate(fd, new_size)) { + /* Truncate to unpadded size first to ensure zero-filled padding */ + if (ftruncate(fd, unpadded_size) || ftruncate(fd, new_size)) { debug("%s: Failed to truncate file: %s\n", __func__, strerror(errno)); ret = -EIO;
Padding the header of an external FIT image is achieved by growing the existing temporary FIT file to match the given alignment requirement 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. Truncate the temporary FIT file to unpadded header length first, so that the subsequent truncate operation extends the file with null bytes, making padding predictably zeroed. Fixes: 7946a814a319 ("Revert "mkimage: fit: Do not tail-pad fitImage with external data"") Signed-off-by: Roman Azarenko <roman.azarenko@iopsys.eu> --- This fix was prompted by an observation that there is unexpected data in the padding between the FIT header and the first image in a generated external FIT file. Having non-zero data in padding doesn't have any negative practical consequences, these FIT files work fine just like any other. However I'm not aware of any reason of making the padding depend on the contents of an internal/intermediate FIT file. In my opinion, padding should be predictably zeroed. Steps to reproduce: 1. Generate a random binary blob representing a fake rootfs image: $ dd if=/dev/urandom of=rootfs bs=1M count=1 2. Define a minimalistic FIT source file: /dts-v1/; / { description = "Padding demo"; #address-cells = <0x01>; images { rootfs { data = /incbin/("rootfs"); type = "filesystem"; compression = "none"; hash-1 { algo = "sha256"; }; }; }; configurations { default = "config"; config { rootfs = "rootfs"; }; }; }; 3. Build an external FIT image with 4096 bytes of padding: $ mkimage -f source.dts -E -B 1000 output.dtb Observe how the header is padded to 4096 bytes (0x1000) with a part of an existing dummy rootfs image, which happened to be in that place in the internal/intermediate FIT file. --- tools/fit_image.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)