Re: [PATCH 6/7] efi: payload: add support for efi stub boot and fit image.

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi,

Some things worth fixing below.


On Sat, Aug 30, 2025 at 11:55:41PM -0400, chalianis1@gmail.com wrote:
> +static int efi_load_file_image(const char *file,
> +			       struct efi_loaded_image **loaded_image,
> +			       efi_handle_t *h)
>  {
> +	efi_physical_addr_t mem;
>  	void *exe;
> +	char *buf;
>  	size_t size;
>  	efi_handle_t handle;
>  	efi_status_t efiret = EFI_SUCCESS;
>  
> -	exe = efi_read_file(file, &size);
> -	if (!exe)
> -		return -errno;
> +	buf = read_file(file, &size);
> +	if (!buf)
> +		return -ENOMEM;

buf is never freed, neither in the error nor in the success path.

>  
> -	efiret = BS->load_image(false, efi_parent_image, efi_device_path, exe, size,
> -			&handle);
> +	exe = efi_allocate_pages(&mem, size, EFI_ALLOCATE_ANY_PAGES,
> +				 EFI_LOADER_CODE);
> +	if (!exe) {
> +		pr_err("Failed to allocate pages for image\n");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(exe, buf, size);
> +
> +	efiret = BS->load_image(false, efi_parent_image, efi_device_path, exe,
> +				size, &handle);
>  	if (EFI_ERROR(efiret)) {
>  		pr_err("failed to LoadImage: %s\n", efi_strerror(efiret));
>  		goto out;
>  	};
>  
>  	efiret = BS->open_protocol(handle, &efi_loaded_image_protocol_guid,
> -			(void **)loaded_image,
> -			efi_parent_image, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +				   (void **)loaded_image, efi_parent_image,
> +				   NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>  	if (EFI_ERROR(efiret)) {
>  		pr_err("failed to OpenProtocol: %s\n", efi_strerror(efiret));
>  		BS->unload_image(handle);
> @@ -151,8 +163,10 @@ static int efi_load_image(const char *file, struct efi_loaded_image **loaded_ima
>  	}
>  
>  	*h = handle;
> +
> +	return 0;
>  out:
> -	efi_free_file(exe, size);
> +	efi_free_pages(exe, size);
>  	return -efi_errno(efiret);
>  }
>  
> @@ -171,20 +185,16 @@ static bool is_linux_image(enum filetype filetype, const void *base)
>  	return false;
>  }
>  
> +static bool ramdisk_is_fit(struct image_data *data)
> +{
> +	struct stat st;
> +
> +	if (bootm_signed_images_are_forced())
> +		return true;
> +
> +	if (data->initrd_file) {
> +		if (!stat(data->initrd_file, &st) && st.st_size > 0)
> +			return false;
> +	}
> +
> +	return data->os_fit ? (bool)fit_has_image(data->os_fit,
> +			data->fit_config, "ramdisk") : false;
> +}
> +
> +static bool fdt_is_fit(struct image_data *data)
> +{
> +	struct stat st;
> +
> +	if (bootm_signed_images_are_forced())
> +		return true;
> +
> +	if (data->oftree_file) {
> +		if (!stat(data->initrd_file, &st) && st.st_size > 0)
> +			return false;
> +	}
> +
> +	return data->os_fit ? (bool)fit_has_image(data->os_fit,
> +			data->fit_config, "fdt") : false;

fit_has_image() can return an error code. Casting this to bool will
result in 'true' which is not what you want here.

> +}
> +
> +static int efi_load_os(struct efi_image_data *e)
> +{
> +	efi_status_t efiret = EFI_SUCCESS;
> +	efi_physical_addr_t mem;
> +	size_t image_size = 0;
> +	void *image = NULL;
> +	void *vmem = NULL;
> +	int ret = 0;
> +
> +	if (e->data->os_fit) {
> +		image = (void *)e->data->fit_kernel;
> +		image_size = e->data->fit_kernel_size;
> +	} else if (e->data->os_file)
> +		return efi_load_file_image(e->data->os_file,
> +				&e->loaded_image, &e->handle);

If neither of the above is true you continue with image = NULL and
image_size = 0. I think you need a else return -ESOMETHING here.

> +
> +	vmem = efi_allocate_pages(&mem, image_size, EFI_ALLOCATE_ANY_PAGES,
> +				 EFI_LOADER_CODE);
> +	if (!vmem) {
> +		pr_err("Failed to allocate pages for image\n");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(vmem, image, image_size);
> +
> +	efiret = BS->load_image(false, efi_parent_image, efi_device_path, image,
> +				image_size, &e->handle);
> +	if (EFI_ERROR(efiret)) {
> +		ret = -efi_errno(efiret);
> +		pr_err("failed to LoadImage: %s\n", efi_strerror(efiret));
> +		goto out_mem;
> +	};
> +
> +	efiret = BS->open_protocol(e->handle, &efi_loaded_image_protocol_guid,
> +				   (void **)&e->loaded_image, efi_parent_image,
> +				   NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +	if (EFI_ERROR(efiret)) {
> +		ret = -efi_errno(efiret);
> +		pr_err("failed to OpenProtocol: %s\n", efi_strerror(efiret));
> +		goto out_unload;
> +	}
> +
> +	e->image_res.base = mem;
> +	e->image_res.size = image_size;
> +
> +	return 0;
> +
> +out_mem:
> +	efi_free_pages(vmem, image_size);
> +out_unload:
> +	BS->unload_image(e->handle);
> +	return ret;
> +}
> +
> +static int efi_load_fdt(struct efi_image_data *e)
> +{
> +	efi_status_t efiret = EFI_SUCCESS;
> +	efi_physical_addr_t mem;
> +	void *vmem, *tmp = NULL;
> +	const void *of_tree;
> +	unsigned long of_size;
> +	bool from_fit;
> +	int ret;
> +
> +	if (IS_ENABLED(CONFIG_EFI_FDT_FORCE))
> +		return 0;
> +
> +	from_fit = fdt_is_fit(e->data);
> +	if (from_fit) {
> +		ret = fit_open_image(e->data->os_fit, e->data->fit_config,
> +				     "fdt", &of_tree, &of_size);
> +		if (ret) {
> +			pr_err("Cannot open FDT image in FIT image: %pe\n",
> +			       ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +
> +	if (!from_fit) {

else instead?

> +		if (!e->data->oftree_file)
> +			return 0;
> +
> +		pr_info("Loading devicetree from '%s'\n", e->data->oftree_file);
> +		tmp = read_file(e->data->oftree_file, &of_size);
> +		if (!tmp || of_size <= 0) {
> +			pr_err("Failed to read initrd from file: %s\n",
> +				e->data->initrd_file);
> +			return -EINVAL;
> +		}
> +		of_tree = tmp;
> +	}
> +
> +	vmem = efi_allocate_pages(&mem, of_size + CONFIG_FDT_PADDING,
> +				 EFI_ALLOCATE_ANY_PAGES,
> +				 EFI_ACPI_RECLAIM_MEMORY);

Can you replace CONFIG_FDT_PADDING with a #define in this file? The
Kconfig option you introduced becomes visible for every user, but it
only has a meaning in special cases. I think on EFI machines we are not
scarce on memory, so you could allocate a much higher amount of memory,
say 128KiB to reduce the need to make this configurable.

> +	if (!vmem) {
> +		pr_err("Failed to allocate pages for FDT\n");
> +		goto free_file;
> +		return -ENOMEM;

This return is never reached. Also add ret = -ENOMEM before the goto.

> +	}
> +
> +	memcpy(vmem, of_tree, of_size);
> +
> +	efiret = BS->install_configuration_table(&efi_fdt_guid,
> +			(void *)mem);
> +	if (EFI_ERROR(efiret)) {
> +		pr_err("Failed to install FDT %s/n", efi_strerror(efiret));
> +		ret = -efi_errno(efiret);
> +		goto free_mem;
> +	}
> +
> +	e->oftree_res.base = mem;
> +	e->oftree_res.size = of_size + CONFIG_FDT_PADDING;
> +
> +	if (!from_fit && tmp)
> +		free(tmp);

No need to check, free() handles NULL pointers fine.

Some of the comments apply to efi_load_ramdisk() as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |