[PATCH 0/2] efi_loader: use short-form DP for load options

[PATCH 0/2] efi_loader: use short-form DP for load options

[Heinrich Schuchardt]

The GUID of partitions is sufficient for identification and will stay
constant in the lifetime of a boot option. The preceding path of the
device-path may change due to changes in the enumeration of devices.

Create short-form device-paths in the 'efidebug boot add' command.

Fix the support of short form device-paths in the implementation of
LoadImage().

Heinrich Schuchardt (2):
  efi_loader: support booting via short-form device-path
  efi_loader: use short-form DP for load options

 cmd/efidebug.c                   | 15 +++++++++++----
 include/efi_loader.h             |  3 ++-
 lib/efi_loader/efi_boottime.c    | 12 ++++++------
 lib/efi_loader/efi_device_path.c | 21 +++++++++++++--------
 4 files changed, 32 insertions(+), 19 deletions(-)

--
2.34.1

[PATCH 1/2] efi_loader: support booting via short-form device-path

[Heinrich Schuchardt]

The boot manager must support loading from boot options using a short-form
device-path, e.g. one where the first element is a hard drive media path.

See '3.1.2 Load Options Processing' in UEFI specification version 2.9.

Fixes: 0e074d12393b ("efi_loader: carve out efi_load_image_from_file()")
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 82128ac1d5..173e636d60 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1940,7 +1940,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;
+	struct efi_device_path *dp, *rem;
 	struct efi_load_file_protocol *load_file_protocol = NULL;
 	efi_uintn_t buffer_size;
 	uint64_t addr, pages;
@@ -1951,18 +1951,18 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
 	*size = 0;

 	dp = file_path;
-	ret = EFI_CALL(efi_locate_device_path(
-		       &efi_simple_file_system_protocol_guid, &dp, &device));
+	device = efi_dp_find_obj(dp, &rem);
+	ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
+				  NULL);
 	if (ret == EFI_SUCCESS)
 		return efi_load_image_from_file(file_path, buffer, size);

-	ret = EFI_CALL(efi_locate_device_path(
-		       &efi_guid_load_file_protocol, &dp, &device));
+	ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
 	if (ret == EFI_SUCCESS) {
 		guid = &efi_guid_load_file_protocol;
 	} else if (!boot_policy) {
 		guid = &efi_guid_load_file2_protocol;
-		ret = EFI_CALL(efi_locate_device_path(guid, &dp, &device));
+		ret = efi_search_protocol(device, guid, NULL);
 	}
 	if (ret != EFI_SUCCESS)
 		return EFI_NOT_FOUND;
--
2.34.1

[PATCH 2/2] efi_loader: use short-form DP for load options

[Heinrich Schuchardt]

The GUID of partitions is sufficient for identification and will stay
constant in the lifetime of a boot option. The preceding path of the
device-path may change due to changes in the enumeration of devices.
Therefore it is preferable to use the short-form of device-paths in load
options. Adjust the 'efidebug boot add' command accordingly.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/efidebug.c                   | 15 +++++++++++----
 include/efi_loader.h             |  3 ++-
 lib/efi_loader/efi_device_path.c | 21 +++++++++++++--------
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 401d13cc4c..f62a4345fd 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -747,7 +747,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
 					 const char *file)

 {
-	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
+	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp;
 	struct efi_device_path *initrd_dp = NULL;
 	efi_status_t ret;
 	const struct efi_initrd_dp id_dp = {
@@ -771,9 +771,12 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
 		printf("Cannot create device path for \"%s %s\"\n", part, file);
 		goto out;
 	}
+	short_fp = efi_dp_shorten(tmp_fp);
+	if (!short_fp)
+		short_fp = tmp_fp;

 	initrd_dp = efi_dp_append((const struct efi_device_path *)&id_dp,
-				  tmp_fp);
+				  short_fp);

 out:
 	efi_free_pool(tmp_dp);
@@ -807,6 +810,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 	size_t label_len, label_len16;
 	u16 *label;
 	struct efi_device_path *device_path = NULL, *file_path = NULL;
+	struct efi_device_path *fp_free = NULL;
 	struct efi_device_path *final_fp = NULL;
 	struct efi_device_path *initrd_dp = NULL;
 	struct efi_load_option lo;
@@ -849,13 +853,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,

 			/* file path */
 			ret = efi_dp_from_name(argv[3], argv[4], argv[5],
-					       &device_path, &file_path);
+					       &device_path, &fp_free);
 			if (ret != EFI_SUCCESS) {
 				printf("Cannot create device path for \"%s %s\"\n",
 				       argv[3], argv[4]);
 				r = CMD_RET_FAILURE;
 				goto out;
 			}
+			file_path = efi_dp_shorten(fp_free);
+			if (!file_path)
+				file_path = fp_free;
 			fp_size += efi_dp_size(file_path) +
 				sizeof(struct efi_device_path);
 			argc -= 5;
@@ -927,7 +934,7 @@ out:
 	efi_free_pool(final_fp);
 	efi_free_pool(initrd_dp);
 	efi_free_pool(device_path);
-	efi_free_pool(file_path);
+	efi_free_pool(fp_free);
 	free(lo.label);

 	return r;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index e390d323a9..c7d6b7c7f3 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -725,7 +725,8 @@ extern void *efi_bounce_buffer;
 #define EFI_LOADER_BOUNCE_BUFFER_SIZE (64 * 1024 * 1024)
 #endif

-
+/* shorten device path */
+struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp);
 struct efi_device_path *efi_dp_next(const struct efi_device_path *dp);
 int efi_dp_match(const struct efi_device_path *a,
 		 const struct efi_device_path *b);
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index dc787b4d3d..ddd5f132ec 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -122,20 +122,25 @@ int efi_dp_match(const struct efi_device_path *a,
 	}
 }

-/*
+/**
+ * efi_dp_shorten() - shorten device-path
+ *
  * We can have device paths that start with a USB WWID or a USB Class node,
  * and a few other cases which don't encode the full device path with bus
  * hierarchy:
  *
- *   - MESSAGING:USB_WWID
- *   - MESSAGING:USB_CLASS
- *   - MEDIA:FILE_PATH
- *   - MEDIA:HARD_DRIVE
- *   - MESSAGING:URI
+ * * MESSAGING:USB_WWID
+ * * MESSAGING:USB_CLASS
+ * * MEDIA:FILE_PATH
+ * * MEDIA:HARD_DRIVE
+ * * MESSAGING:URI
  *
  * See UEFI spec (section 3.1.2, about short-form device-paths)
+ *
+ * @dp:		original devie-path
+ * @Return:	shortened device-path or NULL
  */
-static struct efi_device_path *shorten_path(struct efi_device_path *dp)
+struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
 {
 	while (dp) {
 		/*
@@ -189,7 +194,7 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
 				}
 			}

-			obj_dp = shorten_path(efi_dp_next(obj_dp));
+			obj_dp = efi_dp_shorten(efi_dp_next(obj_dp));
 		} while (short_path && obj_dp);
 	}

--
2.34.1

Re: [PATCH 2/2] efi_loader: use short-form DP for load options

[AKASHI Takahiro]

On Sat, Feb 26, 2022 at 12:56:51PM +0100, Heinrich Schuchardt wrote:
> The GUID of partitions is sufficient for identification and will stay
> constant in the lifetime of a boot option. The preceding path of the
> device-path may change due to changes in the enumeration of devices.
> Therefore it is preferable to use the short-form of device-paths in load
> options. Adjust the 'efidebug boot add' command accordingly.

Since a "long" device path is still valid, I think that a user should
be allowed to use a long device path especially if he or she wants to
limit an interface for loading any image.
Please add an option to efidebug to select a short or long path.

Furthermore, I would like to ask you to add a test, as you always
require me to do so, for loading from a short path.
Otherwise, the feature will never be exercised in CI loop.

-Takahiro Akashi


> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  cmd/efidebug.c                   | 15 +++++++++++----
>  include/efi_loader.h             |  3 ++-
>  lib/efi_loader/efi_device_path.c | 21 +++++++++++++--------
>  3 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 401d13cc4c..f62a4345fd 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -747,7 +747,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
>  					 const char *file)
> 
>  {
> -	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
> +	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp;
>  	struct efi_device_path *initrd_dp = NULL;
>  	efi_status_t ret;
>  	const struct efi_initrd_dp id_dp = {
> @@ -771,9 +771,12 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
>  		printf("Cannot create device path for \"%s %s\"\n", part, file);
>  		goto out;
>  	}
> +	short_fp = efi_dp_shorten(tmp_fp);
> +	if (!short_fp)
> +		short_fp = tmp_fp;
> 
>  	initrd_dp = efi_dp_append((const struct efi_device_path *)&id_dp,
> -				  tmp_fp);
> +				  short_fp);
> 
>  out:
>  	efi_free_pool(tmp_dp);
> @@ -807,6 +810,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	size_t label_len, label_len16;
>  	u16 *label;
>  	struct efi_device_path *device_path = NULL, *file_path = NULL;
> +	struct efi_device_path *fp_free = NULL;
>  	struct efi_device_path *final_fp = NULL;
>  	struct efi_device_path *initrd_dp = NULL;
>  	struct efi_load_option lo;
> @@ -849,13 +853,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> 
>  			/* file path */
>  			ret = efi_dp_from_name(argv[3], argv[4], argv[5],
> -					       &device_path, &file_path);
> +					       &device_path, &fp_free);
>  			if (ret != EFI_SUCCESS) {
>  				printf("Cannot create device path for \"%s %s\"\n",
>  				       argv[3], argv[4]);
>  				r = CMD_RET_FAILURE;
>  				goto out;
>  			}
> +			file_path = efi_dp_shorten(fp_free);
> +			if (!file_path)
> +				file_path = fp_free;
>  			fp_size += efi_dp_size(file_path) +
>  				sizeof(struct efi_device_path);
>  			argc -= 5;
> @@ -927,7 +934,7 @@ out:
>  	efi_free_pool(final_fp);
>  	efi_free_pool(initrd_dp);
>  	efi_free_pool(device_path);
> -	efi_free_pool(file_path);
> +	efi_free_pool(fp_free);
>  	free(lo.label);
> 
>  	return r;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index e390d323a9..c7d6b7c7f3 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -725,7 +725,8 @@ extern void *efi_bounce_buffer;
>  #define EFI_LOADER_BOUNCE_BUFFER_SIZE (64 * 1024 * 1024)
>  #endif
> 
> -
> +/* shorten device path */
> +struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp);
>  struct efi_device_path *efi_dp_next(const struct efi_device_path *dp);
>  int efi_dp_match(const struct efi_device_path *a,
>  		 const struct efi_device_path *b);
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index dc787b4d3d..ddd5f132ec 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -122,20 +122,25 @@ int efi_dp_match(const struct efi_device_path *a,
>  	}
>  }
> 
> -/*
> +/**
> + * efi_dp_shorten() - shorten device-path
> + *
>   * We can have device paths that start with a USB WWID or a USB Class node,
>   * and a few other cases which don't encode the full device path with bus
>   * hierarchy:
>   *
> - *   - MESSAGING:USB_WWID
> - *   - MESSAGING:USB_CLASS
> - *   - MEDIA:FILE_PATH
> - *   - MEDIA:HARD_DRIVE
> - *   - MESSAGING:URI
> + * * MESSAGING:USB_WWID
> + * * MESSAGING:USB_CLASS
> + * * MEDIA:FILE_PATH
> + * * MEDIA:HARD_DRIVE
> + * * MESSAGING:URI
>   *
>   * See UEFI spec (section 3.1.2, about short-form device-paths)
> + *
> + * @dp:		original devie-path
> + * @Return:	shortened device-path or NULL
>   */
> -static struct efi_device_path *shorten_path(struct efi_device_path *dp)
> +struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
>  {
>  	while (dp) {
>  		/*
> @@ -189,7 +194,7 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
>  				}
>  			}
> 
> -			obj_dp = shorten_path(efi_dp_next(obj_dp));
> +			obj_dp = efi_dp_shorten(efi_dp_next(obj_dp));
>  		} while (short_path && obj_dp);
>  	}
> 
> --
> 2.34.1
> 

Re: [PATCH 2/2] efi_loader: use short-form DP for load options

[Heinrich Schuchardt]

On 3/2/22 10:44, AKASHI Takahiro wrote:
> On Sat, Feb 26, 2022 at 12:56:51PM +0100, Heinrich Schuchardt wrote:
>> The GUID of partitions is sufficient for identification and will stay
>> constant in the lifetime of a boot option. The preceding path of the
>> device-path may change due to changes in the enumeration of devices.
>> Therefore it is preferable to use the short-form of device-paths in load
>> options. Adjust the 'efidebug boot add' command accordingly.
>
> Since a "long" device path is still valid, I think that a user should
> be allowed to use a long device path especially if he or she wants to
> limit an interface for loading any image.
> Please add an option to efidebug to select a short or long path.
>
> Furthermore, I would like to ask you to add a test, as you always
> require me to do so, for loading from a short path.
> Otherwise, the feature will never be exercised in CI loop.
>
> -Takahiro Akashi

Dear Takahiro,

thanks for reviewing. In the next round I will alternative options
-b/-B, -i/-I to choose between long and short device path.

I will create a Python test exposes a initrd via LoadFile2 protocol and
then use initrddump.efi as binary payload and to check the content of
the initrd.

Best regards

Heinrich

>
>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   cmd/efidebug.c                   | 15 +++++++++++----
>>   include/efi_loader.h             |  3 ++-
>>   lib/efi_loader/efi_device_path.c | 21 +++++++++++++--------
>>   3 files changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>> index 401d13cc4c..f62a4345fd 100644
>> --- a/cmd/efidebug.c
>> +++ b/cmd/efidebug.c
>> @@ -747,7 +747,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
>>   					 const char *file)
>>
>>   {
>> -	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
>> +	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp;
>>   	struct efi_device_path *initrd_dp = NULL;
>>   	efi_status_t ret;
>>   	const struct efi_initrd_dp id_dp = {
>> @@ -771,9 +771,12 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
>>   		printf("Cannot create device path for \"%s %s\"\n", part, file);
>>   		goto out;
>>   	}
>> +	short_fp = efi_dp_shorten(tmp_fp);
>> +	if (!short_fp)
>> +		short_fp = tmp_fp;
>>
>>   	initrd_dp = efi_dp_append((const struct efi_device_path *)&id_dp,
>> -				  tmp_fp);
>> +				  short_fp);
>>
>>   out:
>>   	efi_free_pool(tmp_dp);
>> @@ -807,6 +810,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>>   	size_t label_len, label_len16;
>>   	u16 *label;
>>   	struct efi_device_path *device_path = NULL, *file_path = NULL;
>> +	struct efi_device_path *fp_free = NULL;
>>   	struct efi_device_path *final_fp = NULL;
>>   	struct efi_device_path *initrd_dp = NULL;
>>   	struct efi_load_option lo;
>> @@ -849,13 +853,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>>
>>   			/* file path */
>>   			ret = efi_dp_from_name(argv[3], argv[4], argv[5],
>> -					       &device_path, &file_path);
>> +					       &device_path, &fp_free);
>>   			if (ret != EFI_SUCCESS) {
>>   				printf("Cannot create device path for \"%s %s\"\n",
>>   				       argv[3], argv[4]);
>>   				r = CMD_RET_FAILURE;
>>   				goto out;
>>   			}
>> +			file_path = efi_dp_shorten(fp_free);
>> +			if (!file_path)
>> +				file_path = fp_free;
>>   			fp_size += efi_dp_size(file_path) +
>>   				sizeof(struct efi_device_path);
>>   			argc -= 5;
>> @@ -927,7 +934,7 @@ out:
>>   	efi_free_pool(final_fp);
>>   	efi_free_pool(initrd_dp);
>>   	efi_free_pool(device_path);
>> -	efi_free_pool(file_path);
>> +	efi_free_pool(fp_free);
>>   	free(lo.label);
>>
>>   	return r;
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index e390d323a9..c7d6b7c7f3 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -725,7 +725,8 @@ extern void *efi_bounce_buffer;
>>   #define EFI_LOADER_BOUNCE_BUFFER_SIZE (64 * 1024 * 1024)
>>   #endif
>>
>> -
>> +/* shorten device path */
>> +struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp);
>>   struct efi_device_path *efi_dp_next(const struct efi_device_path *dp);
>>   int efi_dp_match(const struct efi_device_path *a,
>>   		 const struct efi_device_path *b);
>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>> index dc787b4d3d..ddd5f132ec 100644
>> --- a/lib/efi_loader/efi_device_path.c
>> +++ b/lib/efi_loader/efi_device_path.c
>> @@ -122,20 +122,25 @@ int efi_dp_match(const struct efi_device_path *a,
>>   	}
>>   }
>>
>> -/*
>> +/**
>> + * efi_dp_shorten() - shorten device-path
>> + *
>>    * We can have device paths that start with a USB WWID or a USB Class node,
>>    * and a few other cases which don't encode the full device path with bus
>>    * hierarchy:
>>    *
>> - *   - MESSAGING:USB_WWID
>> - *   - MESSAGING:USB_CLASS
>> - *   - MEDIA:FILE_PATH
>> - *   - MEDIA:HARD_DRIVE
>> - *   - MESSAGING:URI
>> + * * MESSAGING:USB_WWID
>> + * * MESSAGING:USB_CLASS
>> + * * MEDIA:FILE_PATH
>> + * * MEDIA:HARD_DRIVE
>> + * * MESSAGING:URI
>>    *
>>    * See UEFI spec (section 3.1.2, about short-form device-paths)
>> + *
>> + * @dp:		original devie-path
>> + * @Return:	shortened device-path or NULL
>>    */
>> -static struct efi_device_path *shorten_path(struct efi_device_path *dp)
>> +struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
>>   {
>>   	while (dp) {
>>   		/*
>> @@ -189,7 +194,7 @@ static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
>>   				}
>>   			}
>>
>> -			obj_dp = shorten_path(efi_dp_next(obj_dp));
>> +			obj_dp = efi_dp_shorten(efi_dp_next(obj_dp));
>>   		} while (short_path && obj_dp);
>>   	}
>>
>> --
>> 2.34.1
>>