[PATCH v2 0/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[PATCH v2 0/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[AKASHI Takahiro]

This is a reworked version of v1[1].

If a (root) file system does not have any partition table, the generated
boot option (BOOTxxxx) by the command,
    => efidebug boot add -b ...
does not work because a device path in the option is in a short-form and
does not contain any valid media device path which should expectedly support
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.

[1] https://lists.denx.de/pipermail/u-boot/2022-April/482697.html

Test:
=====
* Azure CI passed[2]

[2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=4220&view=results

TODO:
=====
The other types of short-form device paths, USB_CLASS and HARD_DRIVE, should
also be implemented in boot manager rather than LoadImage API so as to more
properly comply with the UEFI specification.
See "3.1.2 Load Option Processing".

Change history:
===============
v2 (May, 12, 2022)
* fully re-write the code to implement the feature in boot manager (patch#2)
* try removable media first (patch#1,#2)
* add a test case (patch#3)

v1 (Apr 28, 2022)
* initial patch

AKASHI Takahiro (3):
  efi_loader: disk: add efi_disk_is_removable()
  efi_loader: bootmgr: fix a problem in loading an image from a
    short-path
  test: efi_bootmgr: add a test case for a short-form path

 include/efi_loader.h                          |  3 +
 lib/efi_loader/efi_bootmgr.c                  | 98 ++++++++++++++++++-
 lib/efi_loader/efi_disk.c                     | 27 +++++
 test/py/tests/test_efi_bootmgr/conftest.py    | 25 +++++
 .../test_efi_bootmgr/test_efi_bootmgr.py      | 25 +++++
 5 files changed, 174 insertions(+), 4 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] efi_loader: disk: add efi_disk_is_removable()

[AKASHI Takahiro]

This helper function will be used to determine if the device is
removable media, initially for handling a short-path loading.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h      |  3 +++
 lib/efi_loader/efi_disk.c | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 40fbab816e2d..839184acb7ff 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -659,6 +659,9 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
 /* Call this to signal an event */
 void efi_signal_event(struct efi_event *event);
 
+/* return true if the device is removable */
+bool efi_disk_is_removable(efi_handle_t handle);
+
 /* open file system: */
 struct efi_simple_file_system_protocol *efi_simple_file_system(
 		struct blk_desc *desc, int part, struct efi_device_path *dp);
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index f5b462fb164a..1e82f52dc070 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -73,6 +73,33 @@ static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
 	return EFI_EXIT(EFI_SUCCESS);
 }
 
+/**
+ * efi_disk_is_removable() - check if the device is removable media
+ * @handle:		efi object handle;
+ *
+ * Examine the device and determine if the device is a local block device
+ * and removable media.
+ *
+ * Return:		true if removable, false otherwise
+ */
+bool efi_disk_is_removable(efi_handle_t handle)
+{
+	struct efi_handler *handler;
+	struct efi_block_io *io;
+	efi_status_t ret;
+
+	ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
+	if (ret != EFI_SUCCESS)
+		return false;
+
+	io = handler->protocol_interface;
+
+	if (!io || !io->media)
+		return false;
+
+	return (bool)io->media->removable_media;
+}
+
 enum efi_disk_direction {
 	EFI_DISK_READ,
 	EFI_DISK_WRITE,
-- 
2.33.0

Re: [PATCH v2 1/3] efi_loader: disk: add efi_disk_is_removable()

[Heinrich Schuchardt]

On 5/12/22 04:29, AKASHI Takahiro wrote:
> This helper function will be used to determine if the device is
> removable media, initially for handling a short-path loading.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   include/efi_loader.h      |  3 +++
>   lib/efi_loader/efi_disk.c | 27 +++++++++++++++++++++++++++
>   2 files changed, 30 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 40fbab816e2d..839184acb7ff 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -659,6 +659,9 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type,
>   /* Call this to signal an event */
>   void efi_signal_event(struct efi_event *event);
>
> +/* return true if the device is removable */
> +bool efi_disk_is_removable(efi_handle_t handle);
> +
>   /* open file system: */
>   struct efi_simple_file_system_protocol *efi_simple_file_system(
>   		struct blk_desc *desc, int part, struct efi_device_path *dp);
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index f5b462fb164a..1e82f52dc070 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -73,6 +73,33 @@ static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
>   	return EFI_EXIT(EFI_SUCCESS);
>   }
>
> +/**
> + * efi_disk_is_removable() - check if the device is removable media
> + * @handle:		efi object handle;
> + *
> + * Examine the device and determine if the device is a local block device
> + * and removable media.
> + *
> + * Return:		true if removable, false otherwise
> + */
> +bool efi_disk_is_removable(efi_handle_t handle)
> +{
> +	struct efi_handler *handler;
> +	struct efi_block_io *io;
> +	efi_status_t ret;
> +
> +	ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return false;
> +
> +	io = handler->protocol_interface;
> +
> +	if (!io || !io->media)
> +		return false;
> +
> +	return (bool)io->media->removable_media;

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

> +}
> +
>   enum efi_disk_direction {
>   	EFI_DISK_READ,
>   	EFI_DISK_WRITE,

[PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[AKASHI Takahiro]

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

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
* not try
  "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but
  supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service
  ConnectController must be called for this device with DriverImageHandle
  and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."
(See section 3.1.2 "Load Option Processing".)

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>
---
 lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++--
 1 file changed, 94 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 631a25d76e9e..3608e433503e 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path)
 	return full_path;
 }
 
+static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,
+			       struct efi_device_path *fp,
+			       efi_handle_t *handle, bool removable)
+{
+	struct efi_handler *handler;
+	struct efi_device_path *dp;
+	int i;
+	efi_status_t ret;
+
+	for (i = 0; i < num; i++) {
+		if (removable && !efi_disk_is_removable(fs_handles[i]))
+			continue;
+		if (!removable && efi_disk_is_removable(fs_handles[i]))
+			continue;
+
+		ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,
+					  &handler);
+		if (ret != EFI_SUCCESS)
+			/* unlikely */
+			continue;
+
+		dp = handler->protocol_interface;
+		if (!dp)
+			/* unlikely */
+			continue;
+
+		dp = efi_dp_append(dp, fp);
+		if (!dp)
+			/* unlikely */
+			continue;
+
+		ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,
+					      handle));
+		efi_free_pool(dp);
+		if (ret == EFI_SUCCESS)
+			return ret;
+	}
+
+	return EFI_NOT_FOUND;
+}
+
+/**
+ * try_load_from_short_path
+ * @fp:		file path
+ * @handle:	pointer to handle for newly installed image
+ *
+ * Enumerate all the devices which support file system operations,
+ * prepend its media device path to the file path, @fp, and
+ * try to load the file.
+ * This function should be called when handling a short-form path
+ * which is starting with a file device path.
+ *
+ * Return:	status code
+ */
+static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
+					     efi_handle_t *handle)
+{
+	efi_handle_t *fs_handles;
+	efi_uintn_t num;
+	efi_status_t ret;
+
+	ret = EFI_CALL(efi_locate_handle_buffer(
+					BY_PROTOCOL,
+					&efi_simple_file_system_protocol_guid,
+					NULL,
+					&num, &fs_handles));
+	if (ret != EFI_SUCCESS)
+		return ret;
+	if (!num)
+		return EFI_NOT_FOUND;
+
+	/* removable media first */
+	ret = __try_load(fs_handles, num, fp, handle, true);
+	if (ret == EFI_SUCCESS)
+		goto out;
+
+	/* fixed media */
+	ret = __try_load(fs_handles, num, fp, handle, false);
+	if (ret == EFI_SUCCESS)
+		goto out;
+
+out:
+	return ret;
+}
+
 /**
  * try_load_entry() - try to load image for boot option
  *
@@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
 		log_debug("trying to load \"%ls\" from %pD\n", lo.label,
 			  lo.file_path);
 
-		file_path = expand_media_path(lo.file_path);
-		ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
-					      NULL, 0, handle));
-		efi_free_pool(file_path);
+		if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
+			/* file_path doesn't contain a device path */
+			ret = try_load_from_short_path(lo.file_path, handle);
+		} else {
+			file_path = expand_media_path(lo.file_path);
+			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
+						      NULL, 0, handle));
+			efi_free_pool(file_path);
+		}
 		if (ret != EFI_SUCCESS) {
 			log_warning("Loading %ls '%ls' failed\n",
 				    varname, lo.label);
-- 
2.33.0

Re: [PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[Heinrich Schuchardt]

On 5/12/22 04:29, 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
>
> 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
> * not try
>    "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but
>    supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service
>    ConnectController must be called for this device with DriverImageHandle
>    and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."

We already have the ConnectController() logic in lib/efi_driver/. We
just have to carry it to all block devices (in a future patch set).

> (See section 3.1.2 "Load Option Processing".)
>
> 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>
> ---
>   lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 631a25d76e9e..3608e433503e 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path)
>   	return full_path;
>   }
>

The logic in this patch looks good to me. Just some nits:

Please, provide a description of the function and its parameters.

> +static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,

Please, use a name not starting with underscores conveying what the
function is good for, e.g. efi_load_from_file_path().

> +			       struct efi_device_path *fp,
> +			       efi_handle_t *handle, bool removable)
> +{
> +	struct efi_handler *handler;
> +	struct efi_device_path *dp;
> +	int i;
> +	efi_status_t ret;
> +
> +	for (i = 0; i < num; i++) {
> +		if (removable && !efi_disk_is_removable(fs_handles[i]))
> +			continue;
> +		if (!removable && efi_disk_is_removable(fs_handles[i]))
> +			continue;
> +
> +		ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,
> +					  &handler);
> +		if (ret != EFI_SUCCESS)
> +			/* unlikely */

If you want to tell the compiler that this path is unlikely:

	        if (unlikely(ret != EFI_SUCCESS))

But as this code is not called often I would simply remove the comment.

> +			continue;
> +
> +		dp = handler->protocol_interface;
> +		if (!dp)
> +			/* unlikely */

ditto

> +			continue;
> +
> +		dp = efi_dp_append(dp, fp);

You are leaking the original dp here. Please, avoid memory leaks.

> +		if (!dp)
> +			/* unlikely */

if (unlikely( ?

> +			continue;
> +
> +		ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,
> +					      handle));
> +		efi_free_pool(dp);
> +		if (ret == EFI_SUCCESS)
> +			return ret;
> +	}
> +
> +	return EFI_NOT_FOUND;
> +}
> +
> +/**
> + * try_load_from_short_path
> + * @fp:		file path
> + * @handle:	pointer to handle for newly installed image
> + *
> + * Enumerate all the devices which support file system operations,
> + * prepend its media device path to the file path, @fp, and
> + * try to load the file.
> + * This function should be called when handling a short-form path
> + * which is starting with a file device path.
> + *
> + * Return:	status code
> + */
> +static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
> +					     efi_handle_t *handle)
> +{
> +	efi_handle_t *fs_handles;
> +	efi_uintn_t num;
> +	efi_status_t ret;
> +
> +	ret = EFI_CALL(efi_locate_handle_buffer(
> +					BY_PROTOCOL,
> +					&efi_simple_file_system_protocol_guid,
> +					NULL,
> +					&num, &fs_handles));
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	if (!num)
> +		return EFI_NOT_FOUND;
> +
> +	/* removable media first */
> +	ret = __try_load(fs_handles, num, fp, handle, true);
> +	if (ret == EFI_SUCCESS)
> +		goto out;
> +
> +	/* fixed media */
> +	ret = __try_load(fs_handles, num, fp, handle, false);
> +	if (ret == EFI_SUCCESS)
> +		goto out;
> +
> +out:
> +	return ret;
> +}
> +
>   /**
>    * try_load_entry() - try to load image for boot option
>    *
> @@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>   		log_debug("trying to load \"%ls\" from %pD\n", lo.label,
>   			  lo.file_path);
>
> -		file_path = expand_media_path(lo.file_path);
> -		ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> -					      NULL, 0, handle));
> -		efi_free_pool(file_path);
> +		if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> +			/* file_path doesn't contain a device path */

%s/device path/device part/

In UEFI speak a file_path is a device path.

Best regards

Heinrich

> +			ret = try_load_from_short_path(lo.file_path, handle);
> +		} else {
> +			file_path = expand_media_path(lo.file_path);
> +			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> +						      NULL, 0, handle));
> +			efi_free_pool(file_path);
> +		}
>   		if (ret != EFI_SUCCESS) {
>   			log_warning("Loading %ls '%ls' failed\n",
>   				    varname, lo.label);

Re: [PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[AKASHI Takahiro]

On Thu, May 12, 2022 at 08:50:34AM +0200, Heinrich Schuchardt wrote:
> On 5/12/22 04:29, 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
> > 
> > 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
> > * not try
> >    "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but
> >    supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service
> >    ConnectController must be called for this device with DriverImageHandle
> >    and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."
> 
> We already have the ConnectController() logic in lib/efi_driver/. We
> just have to carry it to all block devices (in a future patch set).
> 
> > (See section 3.1.2 "Load Option Processing".)
> > 
> > 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>
> > ---
> >   lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++--
> >   1 file changed, 94 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 631a25d76e9e..3608e433503e 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path)
> >   	return full_path;
> >   }
> > 
> 
> The logic in this patch looks good to me. Just some nits:
> 
> Please, provide a description of the function and its parameters.

I don't think we need a description here because, as you can see,
__try_load() is an inner function and intended to be called only from
try_load_from_short_path(). The purpose of extracting it is to avoid
a duplicated code to go though a list of handles *twice*,
for removable media and for fixed media.

The description of try_load_from_short_path() covers both.

> > +static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,
> 
> Please, use a name not starting with underscores conveying what the
> function is good for, e.g. efi_load_from_file_path().
> 
> > +			       struct efi_device_path *fp,
> > +			       efi_handle_t *handle, bool removable)
> > +{
> > +	struct efi_handler *handler;
> > +	struct efi_device_path *dp;
> > +	int i;
> > +	efi_status_t ret;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (removable && !efi_disk_is_removable(fs_handles[i]))
> > +			continue;
> > +		if (!removable && efi_disk_is_removable(fs_handles[i]))
> > +			continue;
> > +
> > +		ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,
> > +					  &handler);
> > +		if (ret != EFI_SUCCESS)
> > +			/* unlikely */
> 
> If you want to tell the compiler that this path is unlikely:
> 
> 	        if (unlikely(ret != EFI_SUCCESS))
> 
> But as this code is not called often I would simply remove the comment.
> 
> > +			continue;
> > +
> > +		dp = handler->protocol_interface;
> > +		if (!dp)
> > +			/* unlikely */
> 
> ditto
> 
> > +			continue;
> > +
> > +		dp = efi_dp_append(dp, fp);
> 
> You are leaking the original dp here. Please, avoid memory leaks.

Actually, not.
The original dp comes from "handler->protocol_interface" and it should
exist while the associated handle, (fs_handles[i]), exists.

-Takahiro Akashi

> 
> > +		if (!dp)
> > +			/* unlikely */
> 
> if (unlikely( ?
> 
> > +			continue;
> > +
> > +		ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,
> > +					      handle));
> > +		efi_free_pool(dp);
> > +		if (ret == EFI_SUCCESS)
> > +			return ret;
> > +	}
> > +
> > +	return EFI_NOT_FOUND;
> > +}
> > +
> > +/**
> > + * try_load_from_short_path
> > + * @fp:		file path
> > + * @handle:	pointer to handle for newly installed image
> > + *
> > + * Enumerate all the devices which support file system operations,
> > + * prepend its media device path to the file path, @fp, and
> > + * try to load the file.
> > + * This function should be called when handling a short-form path
> > + * which is starting with a file device path.
> > + *
> > + * Return:	status code
> > + */
> > +static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
> > +					     efi_handle_t *handle)
> > +{
> > +	efi_handle_t *fs_handles;
> > +	efi_uintn_t num;
> > +	efi_status_t ret;
> > +
> > +	ret = EFI_CALL(efi_locate_handle_buffer(
> > +					BY_PROTOCOL,
> > +					&efi_simple_file_system_protocol_guid,
> > +					NULL,
> > +					&num, &fs_handles));
> > +	if (ret != EFI_SUCCESS)
> > +		return ret;
> > +	if (!num)
> > +		return EFI_NOT_FOUND;
> > +
> > +	/* removable media first */
> > +	ret = __try_load(fs_handles, num, fp, handle, true);
> > +	if (ret == EFI_SUCCESS)
> > +		goto out;
> > +
> > +	/* fixed media */
> > +	ret = __try_load(fs_handles, num, fp, handle, false);
> > +	if (ret == EFI_SUCCESS)
> > +		goto out;
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> >   /**
> >    * try_load_entry() - try to load image for boot option
> >    *
> > @@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> >   		log_debug("trying to load \"%ls\" from %pD\n", lo.label,
> >   			  lo.file_path);
> > 
> > -		file_path = expand_media_path(lo.file_path);
> > -		ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > -					      NULL, 0, handle));
> > -		efi_free_pool(file_path);
> > +		if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> > +			/* file_path doesn't contain a device path */
> 
> %s/device path/device part/
> 
> In UEFI speak a file_path is a device path.
> 
> Best regards
> 
> Heinrich
> 
> > +			ret = try_load_from_short_path(lo.file_path, handle);
> > +		} else {
> > +			file_path = expand_media_path(lo.file_path);
> > +			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > +						      NULL, 0, handle));
> > +			efi_free_pool(file_path);
> > +		}
> >   		if (ret != EFI_SUCCESS) {
> >   			log_warning("Loading %ls '%ls' failed\n",
> >   				    varname, lo.label);

Re: [PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[Heinrich Schuchardt]

Am 12. Mai 2022 09:25:46 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>On Thu, May 12, 2022 at 08:50:34AM +0200, Heinrich Schuchardt wrote:
>> On 5/12/22 04:29, 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
>> > 
>> > 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
>> > * not try
>> >    "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but
>> >    supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service
>> >    ConnectController must be called for this device with DriverImageHandle
>> >    and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."
>> 
>> We already have the ConnectController() logic in lib/efi_driver/. We
>> just have to carry it to all block devices (in a future patch set).
>> 
>> > (See section 3.1.2 "Load Option Processing".)
>> > 
>> > 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>
>> > ---
>> >   lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++--
>> >   1 file changed, 94 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> > index 631a25d76e9e..3608e433503e 100644
>> > --- a/lib/efi_loader/efi_bootmgr.c
>> > +++ b/lib/efi_loader/efi_bootmgr.c
>> > @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path)
>> >   	return full_path;
>> >   }
>> > 
>> 
>> The logic in this patch looks good to me. Just some nits:
>> 
>> Please, provide a description of the function and its parameters.
>
>I don't think we need a description here because, as you can see,
>__try_load() is an inner function and intended to be called only from
>try_load_from_short_path(). The purpose of extracting it is to avoid
>a duplicated code to go though a list of handles *twice*,
>for removable media and for fixed media.
>
>The description of try_load_from_short_path() covers both.

Please, provide a description to let me accept the patch.

Best regards

Heinrich 

>
>> > +static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,
>> 
>> Please, use a name not starting with underscores conveying what the
>> function is good for, e.g. efi_load_from_file_path().
>> 
>> > +			       struct efi_device_path *fp,
>> > +			       efi_handle_t *handle, bool removable)
>> > +{
>> > +	struct efi_handler *handler;
>> > +	struct efi_device_path *dp;
>> > +	int i;
>> > +	efi_status_t ret;
>> > +
>> > +	for (i = 0; i < num; i++) {
>> > +		if (removable && !efi_disk_is_removable(fs_handles[i]))
>> > +			continue;
>> > +		if (!removable && efi_disk_is_removable(fs_handles[i]))
>> > +			continue;
>> > +
>> > +		ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,
>> > +					  &handler);
>> > +		if (ret != EFI_SUCCESS)
>> > +			/* unlikely */
>> 
>> If you want to tell the compiler that this path is unlikely:
>> 
>> 	        if (unlikely(ret != EFI_SUCCESS))
>> 
>> But as this code is not called often I would simply remove the comment.
>> 
>> > +			continue;
>> > +
>> > +		dp = handler->protocol_interface;
>> > +		if (!dp)
>> > +			/* unlikely */
>> 
>> ditto
>> 
>> > +			continue;
>> > +
>> > +		dp = efi_dp_append(dp, fp);
>> 
>> You are leaking the original dp here. Please, avoid memory leaks.
>
>Actually, not.
>The original dp comes from "handler->protocol_interface" and it should
>exist while the associated handle, (fs_handles[i]), exists.
>
>-Takahiro Akashi
>
>> 
>> > +		if (!dp)
>> > +			/* unlikely */
>> 
>> if (unlikely( ?
>> 
>> > +			continue;
>> > +
>> > +		ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,
>> > +					      handle));
>> > +		efi_free_pool(dp);
>> > +		if (ret == EFI_SUCCESS)
>> > +			return ret;
>> > +	}
>> > +
>> > +	return EFI_NOT_FOUND;
>> > +}
>> > +
>> > +/**
>> > + * try_load_from_short_path
>> > + * @fp:		file path
>> > + * @handle:	pointer to handle for newly installed image
>> > + *
>> > + * Enumerate all the devices which support file system operations,
>> > + * prepend its media device path to the file path, @fp, and
>> > + * try to load the file.
>> > + * This function should be called when handling a short-form path
>> > + * which is starting with a file device path.
>> > + *
>> > + * Return:	status code
>> > + */
>> > +static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
>> > +					     efi_handle_t *handle)
>> > +{
>> > +	efi_handle_t *fs_handles;
>> > +	efi_uintn_t num;
>> > +	efi_status_t ret;
>> > +
>> > +	ret = EFI_CALL(efi_locate_handle_buffer(
>> > +					BY_PROTOCOL,
>> > +					&efi_simple_file_system_protocol_guid,
>> > +					NULL,
>> > +					&num, &fs_handles));
>> > +	if (ret != EFI_SUCCESS)
>> > +		return ret;
>> > +	if (!num)
>> > +		return EFI_NOT_FOUND;
>> > +
>> > +	/* removable media first */
>> > +	ret = __try_load(fs_handles, num, fp, handle, true);
>> > +	if (ret == EFI_SUCCESS)
>> > +		goto out;
>> > +
>> > +	/* fixed media */
>> > +	ret = __try_load(fs_handles, num, fp, handle, false);
>> > +	if (ret == EFI_SUCCESS)
>> > +		goto out;
>> > +
>> > +out:
>> > +	return ret;
>> > +}
>> > +
>> >   /**
>> >    * try_load_entry() - try to load image for boot option
>> >    *
>> > @@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>> >   		log_debug("trying to load \"%ls\" from %pD\n", lo.label,
>> >   			  lo.file_path);
>> > 
>> > -		file_path = expand_media_path(lo.file_path);
>> > -		ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
>> > -					      NULL, 0, handle));
>> > -		efi_free_pool(file_path);
>> > +		if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
>> > +			/* file_path doesn't contain a device path */
>> 
>> %s/device path/device part/
>> 
>> In UEFI speak a file_path is a device path.
>> 
>> Best regards
>> 
>> Heinrich
>> 
>> > +			ret = try_load_from_short_path(lo.file_path, handle);
>> > +		} else {
>> > +			file_path = expand_media_path(lo.file_path);
>> > +			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
>> > +						      NULL, 0, handle));
>> > +			efi_free_pool(file_path);
>> > +		}
>> >   		if (ret != EFI_SUCCESS) {
>> >   			log_warning("Loading %ls '%ls' failed\n",
>> >   				    varname, lo.label);

[PATCH v2 3/3] test: efi_bootmgr: add a test case for a short-form path

[AKASHI Takahiro]

A short-form path starting with a file device path will be tested in
a new test case.

This type of short-form path will be created with "efidebug boot add -b",
in particular, when a file system has no partition table.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/py/tests/test_efi_bootmgr/conftest.py    | 25 +++++++++++++++++++
 .../test_efi_bootmgr/test_efi_bootmgr.py      | 25 +++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
index a0a754afbe1b..5cd7252671fa 100644
--- a/test/py/tests/test_efi_bootmgr/conftest.py
+++ b/test/py/tests/test_efi_bootmgr/conftest.py
@@ -38,3 +38,28 @@ def efi_bootmgr_data(u_boot_config):
                shell=True)
 
     return image_path
+
+@pytest.fixture(scope='session')
+def efi_bootmgr_data2(u_boot_config):
+    """Set up a file system without a partition table to be used
+       in UEFI bootmanager tests
+
+    Args:
+        u_boot_config -- U-boot configuration.
+
+    Return:
+        A path to disk image to be used for testing
+    """
+    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
+    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr_data2.img'
+
+    shutil.rmtree(mnt_point, ignore_errors=True)
+    os.mkdir(mnt_point, mode = 0o755)
+
+    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi',
+                    mnt_point + '/helloworld.efi')
+
+    check_call(f'virt-make-fs --size=+1M --type=vfat {mnt_point} {image_path}',
+               shell=True)
+
+    return image_path
diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
index 75a6e7c96296..ab3d53a2dc95 100644
--- a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
+++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
@@ -41,3 +41,28 @@ def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
 
     u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
     u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
+
+@pytest.mark.boardspec('sandbox')
+@pytest.mark.buildconfigspec('cmd_efidebug')
+@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr')
+def test_efi_bootmgr_short(u_boot_console, efi_bootmgr_data2):
+    """ Unit test for UEFI bootmanager with a short-form path
+    In this test case,
+    - File system has no partition table
+    - UEFI load option has a short-form path starting with a file device path
+
+    Args:
+        u_boot_console -- U-Boot console
+        efi_bootmgr_data2 -- Path to the disk image used for testing.
+    """
+    u_boot_console.run_command(cmd = f'host bind 0 {efi_bootmgr_data2}')
+
+    u_boot_console.run_command(cmd = 'efidebug boot add ' \
+        '-b 0001 TEST2 host 0:0 helloworld.efi')
+    response = u_boot_console.run_command(cmd = 'efidebug boot dump')
+    assert 'file_path: /helloworld.efi' in response
+    u_boot_console.run_command(cmd = 'efidebug boot next 0001')
+    response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
+    assert 'Hello, world!' in response
+
+    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
-- 
2.33.0

Re: [PATCH v2 3/3] test: efi_bootmgr: add a test case for a short-form path

[Heinrich Schuchardt]

On 5/12/22 04:29, AKASHI Takahiro wrote:
> A short-form path starting with a file device path will be tested in
> a new test case.
>
> This type of short-form path will be created with "efidebug boot add -b",
> in particular, when a file system has no partition table.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   test/py/tests/test_efi_bootmgr/conftest.py    | 25 +++++++++++++++++++
>   .../test_efi_bootmgr/test_efi_bootmgr.py      | 25 +++++++++++++++++++
>   2 files changed, 50 insertions(+)
>
> diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
> index a0a754afbe1b..5cd7252671fa 100644
> --- a/test/py/tests/test_efi_bootmgr/conftest.py
> +++ b/test/py/tests/test_efi_bootmgr/conftest.py
> @@ -38,3 +38,28 @@ def efi_bootmgr_data(u_boot_config):
>                  shell=True)
>
>       return image_path
> +
> +@pytest.fixture(scope='session')
> +def efi_bootmgr_data2(u_boot_config):
> +    """Set up a file system without a partition table to be used
> +       in UEFI bootmanager tests

Why do you want a file system without partition table? This is nothing
the UEFI specification expects to support. Your test should cover having
a partition table and a device path which is file only.

Best regards

Heinrich

> +
> +    Args:
> +        u_boot_config -- U-boot configuration.
> +
> +    Return:
> +        A path to disk image to be used for testing
> +    """
> +    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
> +    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr_data2.img'
> +
> +    shutil.rmtree(mnt_point, ignore_errors=True)
> +    os.mkdir(mnt_point, mode = 0o755)
> +
> +    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi',
> +                    mnt_point + '/helloworld.efi')
> +
> +    check_call(f'virt-make-fs --size=+1M --type=vfat {mnt_point} {image_path}',
> +               shell=True)
> +
> +    return image_path
> diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> index 75a6e7c96296..ab3d53a2dc95 100644
> --- a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> @@ -41,3 +41,28 @@ def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
>
>       u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
>       u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
> +
> +@pytest.mark.boardspec('sandbox')
> +@pytest.mark.buildconfigspec('cmd_efidebug')
> +@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr')
> +def test_efi_bootmgr_short(u_boot_console, efi_bootmgr_data2):
> +    """ Unit test for UEFI bootmanager with a short-form path
> +    In this test case,
> +    - File system has no partition table
> +    - UEFI load option has a short-form path starting with a file device path
> +
> +    Args:
> +        u_boot_console -- U-Boot console
> +        efi_bootmgr_data2 -- Path to the disk image used for testing.
> +    """
> +    u_boot_console.run_command(cmd = f'host bind 0 {efi_bootmgr_data2}')
> +
> +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
> +        '-b 0001 TEST2 host 0:0 helloworld.efi')

This will not create a file only device path. So this does not test
patch 1 and 2.

You could use setenv -e -i to create the boot option.

Best regards

Heinrich

> +    response = u_boot_console.run_command(cmd = 'efidebug boot dump')
> +    assert 'file_path: /helloworld.efi' in response
> +    u_boot_console.run_command(cmd = 'efidebug boot next 0001')
> +    response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
> +    assert 'Hello, world!' in response
> +
> +    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')

Re: [PATCH v2 3/3] test: efi_bootmgr: add a test case for a short-form path

[AKASHI Takahiro]

On Thu, May 12, 2022 at 09:02:36AM +0200, Heinrich Schuchardt wrote:
> On 5/12/22 04:29, AKASHI Takahiro wrote:
> > A short-form path starting with a file device path will be tested in
> > a new test case.
> > 
> > This type of short-form path will be created with "efidebug boot add -b",
> > in particular, when a file system has no partition table.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   test/py/tests/test_efi_bootmgr/conftest.py    | 25 +++++++++++++++++++
> >   .../test_efi_bootmgr/test_efi_bootmgr.py      | 25 +++++++++++++++++++
> >   2 files changed, 50 insertions(+)
> > 
> > diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py
> > index a0a754afbe1b..5cd7252671fa 100644
> > --- a/test/py/tests/test_efi_bootmgr/conftest.py
> > +++ b/test/py/tests/test_efi_bootmgr/conftest.py
> > @@ -38,3 +38,28 @@ def efi_bootmgr_data(u_boot_config):
> >                  shell=True)
> > 
> >       return image_path
> > +
> > +@pytest.fixture(scope='session')
> > +def efi_bootmgr_data2(u_boot_config):
> > +    """Set up a file system without a partition table to be used
> > +       in UEFI bootmanager tests
> 
> Why do you want a file system without partition table? This is nothing
> the UEFI specification expects to support.

As we discussed several times in the past, a partition table in the file
system is not mandatory in U-Boot's UEFI subsystem.

> Your test should cover having
> a partition table and a device path which is file only.
> 
> Best regards
> 
> Heinrich
> 
> > +
> > +    Args:
> > +        u_boot_config -- U-boot configuration.
> > +
> > +    Return:
> > +        A path to disk image to be used for testing
> > +    """
> > +    mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
> > +    image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr_data2.img'
> > +
> > +    shutil.rmtree(mnt_point, ignore_errors=True)
> > +    os.mkdir(mnt_point, mode = 0o755)
> > +
> > +    shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi',
> > +                    mnt_point + '/helloworld.efi')
> > +
> > +    check_call(f'virt-make-fs --size=+1M --type=vfat {mnt_point} {image_path}',
> > +               shell=True)
> > +
> > +    return image_path
> > diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> > index 75a6e7c96296..ab3d53a2dc95 100644
> > --- a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> > +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py
> > @@ -41,3 +41,28 @@ def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
> > 
> >       u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
> >       u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
> > +
> > +@pytest.mark.boardspec('sandbox')
> > +@pytest.mark.buildconfigspec('cmd_efidebug')
> > +@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr')
> > +def test_efi_bootmgr_short(u_boot_console, efi_bootmgr_data2):
> > +    """ Unit test for UEFI bootmanager with a short-form path
> > +    In this test case,
> > +    - File system has no partition table
> > +    - UEFI load option has a short-form path starting with a file device path
> > +
> > +    Args:
> > +        u_boot_console -- U-Boot console
> > +        efi_bootmgr_data2 -- Path to the disk image used for testing.
> > +    """
> > +    u_boot_console.run_command(cmd = f'host bind 0 {efi_bootmgr_data2}')
> > +
> > +    u_boot_console.run_command(cmd = 'efidebug boot add ' \
> > +        '-b 0001 TEST2 host 0:0 helloworld.efi')
> 
> This will not create a file only device path. So this does not test
> patch 1 and 2.

It does.
See the output from "efidebug boot dump" in the log.

> You could use setenv -e -i to create the boot option.
> 
> Best regards
> 
> Heinrich
> 
> > +    response = u_boot_console.run_command(cmd = 'efidebug boot dump')
> > +    assert 'file_path: /helloworld.efi' in response

Furthermore, here is the check for a device path only with a file.

-Takahiro Akashi

> > +    u_boot_console.run_command(cmd = 'efidebug boot next 0001')
> > +    response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
> > +    assert 'Hello, world!' in response
> > +
> > +    u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
>