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.g...@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