[PATCH] tools: ensure zeroed padding in external FIT images

[Marek Vasut]

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 at 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 ?