[PATCH v2] disk: efi_loader: Improve disk image detection in efi_bootmgr

[Ilias Apalodimas]

Hi Javier

> On Sat Aug 9, 2025 at 12:27 AM EEST, Javier Tia wrote:
> Refines the disk image detection mechanism in the EFI boot manager.
> Instead of relying on file extensions, the updated code uses the
> partition driver lookup function to determine if a downloaded file is a
> valid disk image.
>
> The change enhances the robustness of the boot manager by providing a
> more accurate disk image detection. Extend prepare_loaded_image() to
> accept an optional reuse_blk parameter. When a partition driver is
> detected, the existing temporary device is passed to
> prepare_loaded_image() instead of being destroyed and recreated.
>
> The partition driver lookup function is also made public to enable its
> use in the EFI boot manager. The function documentation is updated
> accordingly.
>
> Signed-off-by: Javier Tia <javier.tia at linaro.org>
>
>  	struct part_driver *drv =
>  		ll_entry_start(struct part_driver, part_driver);
> @@ -855,3 +855,4 @@ int part_get_bootable(struct blk_desc *desc)
>
>  	return 0;
>  }
> +

You don't need a new line here

[...]

>   * @blk:	pointer to created blk udevice
> + * @reuse_blk:	optional existing block device to reuse (can be NULL)
>   * Return:	status code
>   */
>  static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
>  					 struct efi_device_path **dp,
> -					 struct udevice **blk)
> +					 struct udevice **blk,
> +					 struct udevice *reuse_blk)
>  {
>  	u64 pages;
>  	efi_status_t ret;
>  	struct udevice *ramdisk_blk;
>
> -	ramdisk_blk = mount_image(label, addr, size);
> -	if (!ramdisk_blk)
> -		return EFI_LOAD_ERROR;
> +	if (reuse_blk) {
> +		/* Reuse existing block device */
> +		ramdisk_blk = reuse_blk;
> +	} else {
> +		/* Create new block device */
> +		ramdisk_blk = mount_image(label, addr, size);
> +		if (!ramdisk_blk)
> +			return EFI_LOAD_ERROR;
> +	}

Why do we need this? With the new code the image is always going to be mounted beforehand.
The function is only used locally as well. So can't we get rid of the 'else' and always
expect a loaded image. The only difference is that the  *reuse_blk can't be NULL

>
>  	ret = fill_default_file_path(ramdisk_blk, dp);
>  	if (ret != EFI_SUCCESS) {
> @@ -387,7 +396,8 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
>  	return EFI_SUCCESS;
>
>  err:
> -	if (blkmap_destroy(ramdisk_blk->parent))
> +	/* Only destroy if we created it (not reused) */
> +	if (!reuse_blk && blkmap_destroy(ramdisk_blk->parent))
>  		log_err("Destroying blkmap failed\n");
>
>  	return ret;
> @@ -407,7 +417,7 @@ static efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>  	if (!ctx)
>  		return ret;
>
> -	/* cleanup for iso or img image */
> +	/* cleanup for disk image */
>  	if (ctx->ramdisk_blk_dev) {
>  		ret = efi_add_memory_map(ctx->image_addr, ctx->image_size,
>  					 EFI_CONVENTIONAL_MEMORY);
> @@ -452,6 +462,20 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>  	EFI_EXIT(ret);
>  }
>
> +/**
> + * cleanup_temp_blkdev() - Clean up temporary block device
> + *
> + * @temp_bdev: temporary block device to clean up
> + */
> +static void cleanup_temp_blkdev(struct udevice *temp_bdev)
> +{
> +	if (!temp_bdev)
> +		return;
> +
> +	if (blkmap_destroy(temp_bdev->parent))
> +		log_err("Destroying temporary blkmap failed\n");
> +}
> +
>  /**
>   * try_load_from_uri_path() - Handle the URI device path
>   *
> @@ -466,7 +490,6 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>  {
>  	char *s;
>  	int err;
> -	int uri_len;
>  	efi_status_t ret;
>  	void *source_buffer;
>  	efi_uintn_t source_size;
> @@ -516,21 +539,43 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>  	image_size = ALIGN(image_size, SZ_2M);
>
>  	/*
> -	 * If the file extension is ".iso" or ".img", mount it and try to load
> -	 * the default file.
> -	 * If the file is PE-COFF image, load the downloaded file.
> +	 * Check if the downloaded file is a disk image or PE-COFF image.
> +	 * Use partition driver lookup for disk image detection.
>  	 */
> -	uri_len = strlen(uridp->uri);
> -	if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> -	    !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> +	struct udevice *temp_bdev;
> +	struct part_driver *part_drv = NULL;

Mixed declarations are not allowed. You need to move these up with the rest.

> +
> +	/* Create temporary block device from the downloaded image for testing */
> +	temp_bdev = mount_image(lo_label, image_addr, image_size);
> +	if (!temp_bdev) {
> +		log_debug("mount_image() failed, will attempt PE-COFF detection\n");
> +	}

Get rid of this and move the log_debug() in the 'else if'

> +	if (temp_bdev) {
> +		struct blk_desc *desc = dev_get_uclass_plat(temp_bdev);
> +		if (!desc) {
> +			/* Clean up temporary device when dev_get_uclass_plat() returns NULL */
> +			cleanup_temp_blkdev(temp_bdev);
> +		} else {
> +			/*
> +			 * Use part_driver_lookup_type for comprehensive partition detection
> +			 */
> +			part_drv = part_driver_lookup_type(desc);
> +		}
> +	}
> +
> +	if (part_drv) {
> +		/* Reuse the temporary device instead of destroying and recreating */
>  		ret = prepare_loaded_image(lo_label, image_addr, image_size,
> -					   &loaded_dp, &blk);
> +					   &loaded_dp, &blk, temp_bdev);
>  		if (ret != EFI_SUCCESS)
>  			goto err;
>
>  		source_buffer = NULL;
>  		source_size = 0;
>  	} else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
> +		/* Clean up temporary device if it was created but not a valid disk image */
> +		cleanup_temp_blkdev(temp_bdev);

Is this really needed ? Since we ended up here part_drv is NULL which means dev_get_uclass_plat()
returned NULL and the cleanup has already executed.

> +
>  		/*
>  		 * loaded_dp must exist until efi application returns,
>  		 * will be freed in return_to_efibootmgr event callback.
> @@ -545,7 +590,10 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>  		source_buffer = (void *)image_addr;
>  		source_size = image_size;
>  	} else {
> -		log_err("Error: file type is not supported\n");
> +		/* Clean up temporary device if it was created but not a valid disk image */
> +		cleanup_temp_blkdev(temp_bdev);
> +
> +		log_err("Error: disk image is not supported\n");
>  		ret = EFI_UNSUPPORTED;
>  		goto err;
>  	}

Thanks
/Ilias