Re: [PATCH v3 1/5] boot: fit: tell callers when a devicetree buffer must be freed

[David Lechner <dlechner@baylibre.com>]

On 10/26/26 7:18 AM, Carlo Caione wrote:
> The FIT devicetree loader can return either data inside the FIT or a
> newly allocated buffer. Callers which only need a temporary result
> cannot tell whether it is safe to free.
> 
> Report when the returned buffer must be freed. This lets the new
> fdt fit command copy the result to its chosen address without leaking
> memory or freeing part of the original FIT. Also release temporary
> allocations when they are no longer needed, including on error.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>  boot/image-fdt.c |  3 ++-
>  boot/image-fit.c | 68 +++++++++++++++++++++++++++++++++++++++++++++-----------
>  include/image.h  |  5 ++++-
>  3 files changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 956a3d97c42..e65114ff835 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -468,7 +468,8 @@ static int select_fdt(struct bootm_headers *images, const char *select, u8 arch,
>  				fdt_noffset = boot_get_fdt_fit(images, fdt_addr,
>  							       &fit_uname_fdt,
>  							       &fit_uname_config,
> -							       arch, &load, &len);
> +							       arch, &load, &len,
> +							       NULL);
>  
>  				if (fdt_noffset < 0)
>  					return -ENOENT;
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index 60e47db5fb0..4f4e7b4e06a 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -2242,10 +2242,13 @@ static inline int fit_image_load_storage(struct bootm_headers *images,
>  }
>  #endif
>  
> -int fit_image_load(struct bootm_headers *images, ulong addr,
> -		   const char **fit_unamep, const char **fit_uname_configp,
> -		   int arch, int ph_type, int bootstage_id,
> -		   enum fit_load_op load_op, ulong *datap, ulong *lenp)
> +/* Track load-time copies without changing the public fit_image_load() API. */
> +static int fit_image_load_alloc(struct bootm_headers *images, ulong addr,
> +				const char **fit_unamep,
> +				const char **fit_uname_configp,
> +				int arch, int ph_type, int bootstage_id,
> +				enum fit_load_op load_op, ulong *datap,
> +				ulong *lenp, bool *ownedp)
>  {
>  	int image_type = image_ph_type(ph_type);
>  	int cfg_noffset, noffset;
> @@ -2255,6 +2258,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  	const void *fit;
>  	void *buf;
>  	void *loadbuf;
> +	bool load_owned = false;
>  	size_t size;
>  	int type_ok, os_ok;
>  	ulong load, load_end, data, len;
> @@ -2262,6 +2266,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  	const char *prop_name;
>  	int ret;
>  
> +	if (ownedp)
> +		*ownedp = false;
>  	fit = map_sysmem(addr, 0);
>  	fit_uname = fit_unamep ? *fit_unamep : NULL;
>  	fit_uname_config = fit_uname_configp ? *fit_uname_configp : NULL;
> @@ -2487,6 +2493,9 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  		log_debug("decompressing image\n");
>  		if (load == data) {
>  			loadbuf = aligned_alloc(8, max_decomp_len);
> +			if (!loadbuf)
> +				return -ENOMEM;

Checking for failed allocation seems like separate change that should get
it's own patch.

and Fixes: 8fbcc0e0e839 ("boot: Assure FDT is always at 8-byte aligned address")


> +			load_owned = true;

I would rename this to loadbuf_owned since it is the loadbuf pointer that
is being freed later.

>  			load = map_to_sysmem(loadbuf);
>  		} else {
>  			loadbuf = map_sysmem(load, max_decomp_len);
> @@ -2494,13 +2503,17 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  		if (image_decomp(comp, load, data, image_type,
>  				loadbuf, buf, len, max_decomp_len, &load_end)) {
>  			printf("Error decompressing %s\n", prop_name);
> -
> +			if (load_owned)
> +				free(loadbuf);

Freeing on error also technically fixes a leak in the existing code, so
could be considered for a separate patch as well.

>  			return -ENOEXEC;
>  		}
>  		len = load_end - load;
>  	} else if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
>  		   (load & 7)) {
>  		loadbuf = aligned_alloc(8, len);
> +		if (!loadbuf)
> +			return -ENOMEM;
> +		load_owned = true;
>  		load = map_to_sysmem(loadbuf);
>  		memcpy(loadbuf, buf, len);
>  	} else if (load != data) {
> @@ -2517,6 +2530,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  	if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
>  	    fdt_check_header(loadbuf)) {
>  		puts("Subimage data is not a FDT\n");
> +		if (load_owned)
> +			free(loadbuf);
>  		return -ENOEXEC;
>  	}
>  
> @@ -2526,6 +2541,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  
>  	*datap = load;
>  	*lenp = len;
> +	if (ownedp)
> +		*ownedp = load_owned;
>  	if (fit_unamep)
>  		*fit_unamep = (char *)fit_uname;
>  	if (fit_uname_configp)
> @@ -2535,6 +2552,16 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
>  	return noffset;
>  }
>  
> +int fit_image_load(struct bootm_headers *images, ulong addr,
> +		   const char **fit_unamep, const char **fit_uname_configp,
> +		   int arch, int ph_type, int bootstage_id,
> +		   enum fit_load_op load_op, ulong *datap, ulong *lenp)
> +{
> +	return fit_image_load_alloc(images, addr, fit_unamep, fit_uname_configp,
> +				    arch, ph_type, bootstage_id, load_op, datap,
> +				    lenp, NULL);
> +}
> +
>  int boot_get_setup_fit(struct bootm_headers *images, uint8_t arch,
>  		       ulong *setup_start, ulong *setup_len)
>  {
> @@ -2631,7 +2658,7 @@ out:
>  
>  int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  		     const char **fit_unamep, const char **fit_uname_configp,
> -		     int arch, ulong *datap, ulong *lenp)
> +		     int arch, ulong *datap, ulong *lenp, bool *ownedp)
>  {
>  	int fdt_noffset, cfg_noffset, count;
>  	const void *fit;
> @@ -2639,7 +2666,8 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  	const char *fit_uname_config = NULL;
>  	char *fit_uname_config_copy = NULL;
>  	char *next_config = NULL;
> -	ulong load, len;
> +	ulong load = 0, len = 0;
> +	bool load_owned = false;
>  #ifdef CONFIG_OF_LIBFDT_OVERLAY
>  	ulong ovload, ovlen, ovcopylen, need;
>  	const char *uconfig;
> @@ -2651,6 +2679,8 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  #endif
>  
>  	fit_uname = fit_unamep ? *fit_unamep : NULL;
> +	if (ownedp)
> +		*ownedp = false;
>  
>  	if (fit_uname_configp && *fit_uname_configp) {
>  		fit_uname_config_copy = strdup(*fit_uname_configp);
> @@ -2664,11 +2694,11 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  			fit_uname_config = fit_uname_config_copy;
>  	}
>  
> -	fdt_noffset = fit_image_load(images,
> -		addr, &fit_uname, &fit_uname_config,
> -		arch, IH_TYPE_FLATDT,
> -		BOOTSTAGE_ID_FIT_FDT_START,
> -		FIT_LOAD_OPTIONAL, &load, &len);
> +	fdt_noffset = fit_image_load_alloc(images, addr, &fit_uname,
> +					   &fit_uname_config, arch, IH_TYPE_FLATDT,
> +					   BOOTSTAGE_ID_FIT_FDT_START,
> +					   FIT_LOAD_OPTIONAL, &load, &len,
> +					   &load_owned);
>  
>  	if (fdt_noffset < 0)
>  		goto out;
> @@ -2699,6 +2729,10 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  	err = boot_get_fdt_fit_into_buffer(map_sysmem(load, len), len,
>  					   CONFIG_SYS_FDT_PAD, 0, &base_buf,
>  					   &base_buf_size);
> +	if (load_owned) {
> +		free(map_sysmem(load, len));
> +		load_owned = false;
> +	}
>  	if (err < 0) {
>  		if (err != -ENOMEM)
>  			printf("Required FDT copy for applying DTOs failed: %s\n",
> @@ -2809,9 +2843,17 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  
>  out:
>  #ifdef CONFIG_OF_LIBFDT_OVERLAY
> -	if (fdt_noffset >= 0 && base_buf)
> +	if (fdt_noffset >= 0 && base_buf) {
>  		load = map_to_sysmem(base_buf);
> +		load_owned = true;
> +	}
>  #endif
> +	if (fdt_noffset < 0 && load_owned) {
> +		free(map_sysmem(load, len));
> +		load_owned = false;
> +	}
> +	if (ownedp)
> +		*ownedp = load_owned;
>  	if (datap)
>  		*datap = load;
>  	if (lenp)
> diff --git a/include/image.h b/include/image.h
> index d991b4c2bc3..4ef5344c718 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -721,12 +721,15 @@ int boot_get_setup_fit(struct bootm_headers *images, uint8_t arch,
>   * @param arch		Expected architecture (IH_ARCH_...)
>   * @param datap		Returns address of loaded image
>   * @param lenp		Returns length of loaded image
> + * @param ownedp	Returns true if the loaded image is separately allocated
> + *			and must be freed by the caller; may be NULL if the caller
> + *			does not need this information

It's a bit long, but caller_must_free_datap would be a more clear name.

Also, if I understand correctly, caller would need to call:

	free(map_sysmem(load, len))

Which isn't obvious at all, so should be documented.

>   *
>   * Return: node offset of base image, or -ve error code on error
>   */
>  int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
>  		     const char **fit_unamep, const char **fit_uname_configp,
> -		     int arch, ulong *datap, ulong *lenp);
> +		     int arch, ulong *datap, ulong *lenp, bool *ownedp);
>  
>  /**
>   * fit_image_load() - load an image from a FIT
>