Re: [PATCH] efi_loader: fix a problem in loading an image from a short-path

On Fri, Apr 29, 2022 at 12:57:15PM +0200, Heinrich Schuchardt wrote:

On 4/28/22 06:50, AKASHI Takahiro wrote:

...

Booting from a short-form device path which starts with the first element being a File Path Media Device Path failed because it doesn't contain any valid device with simple file system protocol and efi_dp_find_obj() in efi_load_image_from_path() will return NULL. For instance, /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi -> shortened version: /\helloworld.efi

Thanks for enabling this.

Please, enhance test/py/tests/test_efi_bootmgr to test booting from a filename only device path.

Okay, but you should have added tests in your patch.

...

With this patch applied, all the media devices with simple file system protocol are enumerated and the boot manager attempts to boot temporarily generated device paths one-by-one.

This new implementation is still a bit incompatible with the UEFI specification in terms of:

• not creating real boot options

The UEFI specification has:

"These new boot options must not be saved to non volatile storage, and may not be added to BootOrder."

Is there really something missing?

When the boot manager attempts to boot a short-form File Path Media Device Path, it will enumerate all removable media devices, followed by all fixed media devices, creating boot options for each device. ^^^^^^^^^^^^^^^^^^^^^

...

• not distinguishing removable media and fix media

struct blk_desc has field 'removable' which is mirrored in struct efi_disk_obj.

Instead of relying on such an internal structure, we can and should use a public interface, efi_block_io(->media.removable_media).

mmc_bind() always sets removable to 1 irrespective of the device being eMMC or SD-card. I guess this would need to be fixed.

...

(See section 3.1.3 "Boot Options".)

But it still gives us a closer and better solution than the current.

Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path") Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

---

include/efi_loader.h | 3 ++ lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++---- lib/efi_loader/efi_file.c | 35 ++++++++++---- 3 files changed, 107 insertions(+), 18 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index ba79a9afb404..9730c1375a55 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event); struct efi_simple_file_system_protocol *efi_simple_file_system( struct blk_desc *desc, int part, struct efi_device_path *dp);

+/* open file from simple file system */ +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,

• 			 struct efi_device_path *fp);
  

  /* open file from device-path: */ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 5bcb8253edba..39b0e8f7ade0 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1868,19 +1868,21 @@ out: }

/**

• • efi_load_image_from_file() - load an image from file system

• • __efi_load_image_from_file() - load an image from file system
  • Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
  • callers obligation to update the memory type as needed.
• • @v: simple file system

Please, use a meaningful variable name.

I will fully re-write this patch. While efi boot manager should be responsible for handling a short-form device path starting with a file path, my current implementation is made in efi_load_image_from_path() hence LoadImage API.

Instead, the logic of enumerating file_system_protocol devices will be added to try_load_entry()/efi_bootmgr_load().

-Takahiro Akashi

...

• @file_path: the path of the image to load
• @buffer: buffer containing the loaded image
• @size: size of the loaded image
• Return: status code

*/ static -efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,

• 		      void **buffer, efi_uintn_t *size)
  

+efi_status_t __efi_load_image_from_file(struct efi_simple_file_system_protocol *v,

• 			struct efi_device_path *file_path,
  

• 			void **buffer, efi_uintn_t *size)
  

  { struct efi_file_handle *f; efi_status_t ret;

@@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, efi_uintn_t bs;

/* Open file */

• f = efi_file_from_path(file_path);

• if (v)

• f = efi_file_from_fs(v, file_path);
  

• else

• /* file_path should have a device path */
  

• f = efi_file_from_path(file_path);
  

  if (!f) return EFI_NOT_FOUND;

@@ -1921,6 +1927,64 @@ error: return ret; }

+/**

• • efi_load_image_from_file() - load an image from file system
• • Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
• • callers obligation to update the memory type as needed.
• • @file_path: the path of the image to load
• • @buffer: buffer containing the loaded image
• • @size: size of the loaded image
• • Return: status code
• */

+static +efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,

• 		      void **buffer, efi_uintn_t *size)
  

+{

• efi_uintn_t no_handles;
• efi_handle_t *handles;
• struct efi_handler *handler;
• struct efi_simple_file_system_protocol *fs;
• int i;
• efi_status_t ret;
• /* if a file_path contains a device path */
• if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH))

• return __efi_load_image_from_file(NULL, file_path, buffer, size);
  

• /* no explicit device specified */
• ret = EFI_CALL(efi_locate_handle_buffer(

• 			BY_PROTOCOL,
  

• 			&efi_simple_file_system_protocol_guid,
  

• 			NULL,
  

• 			&no_handles,
  

• 			&handles));
  

• if (ret != EFI_SUCCESS)

• return ret;
  

• if (!no_handles)

• return EFI_NOT_FOUND;
  

• for (i = 0; i < no_handles; i++) {

• ret = efi_search_protocol(handles[i],
  

• 			  &efi_simple_file_system_protocol_guid,
  

• 			  &handler);
  

• if (ret != EFI_SUCCESS)
  

• 	/* unlikely */
  

• 	continue;
  

• fs = handler->protocol_interface;
  

• if (!fs)
  

• 	continue;
  

• ret = __efi_load_image_from_file(fs, file_path, buffer, size);
  

• if (ret == EFI_SUCCESS)
  

• 	return ret;
  

• }
• return EFI_NOT_FOUND;

+}

• /**
  • efi_load_image_from_path() - load an image using a file path

@@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy, { efi_handle_t device; efi_status_t ret;

• struct efi_device_path *dp, *rem;

• struct efi_device_path *rem; struct efi_load_file_protocol *load_file_protocol = NULL; efi_uintn_t buffer_size; uint64_t addr, pages;

@@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool boot_policy, *buffer = NULL; *size = 0;

• dp = file_path;
• device = efi_dp_find_obj(dp, NULL, &rem);
• ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,

• 		  NULL);
  

• /* try first for simple file system protocols */
• ret = efi_load_image_from_file(file_path, buffer, size); if (ret == EFI_SUCCESS)

• return efi_load_image_from_file(file_path, buffer, size);
  

• return ret;
  

• /* TODO: does this really make sense? */

• device = efi_dp_find_obj(file_path, NULL, &rem);

• if (!device)

• return EFI_NOT_FOUND;
  
  

  ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL); if (ret == EFI_SUCCESS) {

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index 7a7077e6d032..2d6a432b168b 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -1083,16 +1083,16 @@ static const struct efi_file_handle efi_file_handle_protocol = { /**

• efi_file_from_path() - open file via device path

• • @fp: device path

• • @v: simple file system
• • @fp: file path
  • Return: EFI_FILE_PROTOCOL for the file or NULL
  */

-struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,

• 			 struct efi_device_path *fp)
  

  {

• struct efi_simple_file_system_protocol *v; struct efi_file_handle *f; efi_status_t ret;

• v = efi_fs_from_path(fp); if (!v) return NULL;

@@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) if (ret != EFI_SUCCESS) return NULL;

• /* Skip over device-path nodes before the file path. */
• while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))

• fp = efi_dp_next(fp);
  

• /*
  • Step through the nodes of the directory path until the actual file
  • node is reached which is the final node in the device path.

@@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) return f; }

+/**

• • efi_file_from_path() - open file via device path
• • @fp: device path

'fp' does not resonate 'device path'. How about using dp?

...

• • Return: EFI_FILE_PROTOCOL for the file or NULL
• */

+struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) +{

• struct efi_simple_file_system_protocol *v;

Please, provide a meaningful variable name.

Best regards

Heinrich

...

• v = efi_fs_from_path(fp);
• if (!v)

• return NULL;
  

• /* Skip over device-path nodes before the file path. */
• while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))

• fp = efi_dp_next(fp);
  

• if (!fp)

• return NULL;
  

• return efi_file_from_fs(v, fp);

+}

• static efi_status_t EFIAPI efi_open_volume(struct efi_simple_file_system_protocol *this, struct efi_file_handle **root)