[PATCH] bootstd: Avoid freeing a non-allocated buffer

[Heinrich Schuchardt]

On 11/16/23 02:35, Simon Glass wrote:
> EFI applications can be very large and thus used to cause boot failures
> when malloc() space was exhausted.
>
> A recent changed fixed this by using the kernel_addr_r environment var
> as the address of the buffer. However, it still frees the buffer when
> the bootflow is discarded.
>
> Fix this by introducing a flag to indicate whether the buffer was
> allocated, or not.
>
> Note that kernel_addr_r is not the last word here. It might be better
> to use lmb to place images. But there is a lot of refactoring to do

We need a discussion about memory management. We currently have:

* malloc
* EFI - AllocatePages(), AllocatePool()
* lmb
* preassigned addresses like $kernel_addr_r, $fdt_addr_r

EFI manages memory on page level. It needs to provide a complete memory
map to the OS with flags for different types of memory. Currently it is
not aware of where files are loaded and might allocate those regions for
its own usage.

lmb does not track any allocations but tries to recreate a memory map on
the fly whenever a file is loaded.

It would be preferable to allocate memory for files and require to
explicitly unload a file before reusing the same memory area.

We should start a separate thread on this topic.

> before we can remove the environment variables. The distro scripts rely
> on them so it is safe for bootstd to do so too.
>
> Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> Reported by: Simon Glass <sjg at chromium.org>
> Reported by: Shantur Rathore <i at shantur.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

> ---
>
>   boot/bootflow.c     | 3 ++-
>   boot/bootmeth_efi.c | 1 +
>   include/bootflow.h  | 5 ++++-
>   3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 6922e7e0c4e7..1ea2966ae9a5 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
>   	free(bflow->name);
>   	free(bflow->subdir);
>   	free(bflow->fname);
> -	free(bflow->buf);
> +	if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
> +		free(bflow->buf);
>   	free(bflow->os_name);
>   	free(bflow->fdt_fname);
>   	free(bflow->bootmeth_priv);
> diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> index ae936c8daa18..9ba7734911e1 100644
> --- a/boot/bootmeth_efi.c
> +++ b/boot/bootmeth_efi.c
> @@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow, ulong addr)
>   	if (ret)
>   		return log_msg_ret("read", ret);
>   	bflow->buf = map_sysmem(addr, bflow->size);
> +	bflow->flags |= BOOTFLOWF_STATIC_BUF;
>
>   	set_efi_bootdev(desc, bflow);
>
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 44d3741eacae..fede8f22a2b8 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -43,9 +43,12 @@ enum bootflow_state_t {
>    *	and it is using the prior-stage FDT, which is the U-Boot control FDT.
>    *	This is only possible with the EFI bootmeth (distro-efi) and only when
>    *	CONFIG_OF_HAS_PRIOR_STAGE is enabled
> + * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set, rather
> + *	than being allocated by malloc().
>    */
>   enum bootflow_flags_t {
>   	BOOTFLOWF_USE_PRIOR_FDT	= 1 << 0,
> +	BOOTFLOWF_STATIC_BUF	= 1 << 1,
>   };
>
>   /**
> @@ -72,7 +75,7 @@ enum bootflow_flags_t {
>    * @fname: Filename of bootflow file (allocated)
>    * @logo: Logo to display for this bootflow (BMP format)
>    * @logo_size: Size of the logo in bytes
> - * @buf: Bootflow file contents (allocated)
> + * @buf: Bootflow file contents (allocated unless @flags & BOOTFLOWF_STATIC_BUF)
>    * @size: Size of bootflow file in bytes
>    * @err: Error number received (0 if OK)
>    * @os_name: Name of the OS / distro being booted, or NULL if not known