Re: [PATCH v2 1/8] efi_loader: allow concatenation with contained end node

On Tue, 28 May 2024 at 17:43, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

Allow appending a device-path to a device-path that contains an end node as separator. We need this feature for creating boot options specifying kernel, initrd, and dtb.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

v2: update efi_dp_concat() instead of new function efi_dp_merge()

---

cmd/eficonfig.c | 6 +++--- cmd/efidebug.c | 4 ++-- include/efi_loader.h | 2 +- lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_device_path.c | 20 +++++++++++++------- lib/efi_loader/efi_device_path_utilities.c | 2 +- 8 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 0ba92c60e03..b13d9a3d2d9 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -531,7 +531,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ dp = efi_dp_shorten(dp_volume); if (!dp) dp = dp_volume;

•   dp = efi_dp_concat(dp, &fp->dp, false);
  

•   dp = efi_dp_concat(dp, &fp->dp, 0);
    free(buf);
  
    return dp;
  

@@ -1485,7 +1485,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,

•                                     dp, false);
  

•                                     dp, 0);
            efi_free_pool(dp);
    }
  
  

@@ -1496,7 +1496,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo } final_dp_size = efi_dp_size(dp) + sizeof(END); if (initrd_dp) {

•           final_dp = efi_dp_concat(dp, initrd_dp, true);
  

•           final_dp = efi_dp_concat(dp, initrd_dp, 1);
            final_dp_size += efi_dp_size(initrd_dp) + sizeof(END);
    } else {
            final_dp = efi_dp_dup(dp);
  

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index c2c525f2351..762027daf8a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -697,7 +697,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, short_fp = tmp_fp;

    initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,

•                             short_fp, false);
  

•                             short_fp, 0);
  
  

out: efi_free_pool(tmp_dp); @@ -917,7 +917,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }

•   final_fp = efi_dp_concat(file_path, initrd_dp, true);
  

•   final_fp = efi_dp_concat(file_path, initrd_dp, 1);
    if (!final_fp) {
            printf("Cannot create final device path\n");
            r = CMD_RET_FAILURE;
  

diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..ddf2e41a95c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -946,7 +946,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2,

•                                 bool split_end_node);
  

•                                 size_t split_end_node);
  

struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path); efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, efi_uintn_t *size); diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index b7910f78fb6..a87006b3c0e 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -150,7 +150,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) msg_path = file_path; } else { file_path = efi_dp_concat(bootefi_device_path,

•                                     bootefi_image_path, false);
  

•                                     bootefi_image_path, 0);
            msg_path = bootefi_image_path;
            log_debug("Loaded from disk\n");
    }
  

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 7da3139f917..b0bf21cf841 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -130,7 +130,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles, if (!dp) continue;

•           dp = efi_dp_concat(dp, fp, false);
  

•           dp = efi_dp_concat(dp, fp, 0);
            if (!dp)
                    continue;
  
  

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1951291747c..630c5f52c4f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1816,7 +1816,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);

•           dp = efi_dp_concat(device_path, file_path, false);
  

•           dp = efi_dp_concat(device_path, file_path, 0);
            if (!dp) {
                    ret = EFI_OUT_OF_RESOURCES;
                    goto failure;
  

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index aec224d8466..c8c8d54f733 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -276,10 +276,11 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)

• @dp1: First device path
• @dp2: Second device path

• • @split_end_node: If true the two device paths will be concatenated and

• •              separated by an end node (DEVICE_PATH_SUB_TYPE_END).
    

• •             If false the second device path will be concatenated to the
    

• •             first one as-is.
    

• • @split_end_node:
• • • 0 to concatenate
• • • 1 to concatenate with end node added as separator
• • • size of dp1 excluding last end node to concatenate with end node as
• • separator in case dp1 contains an end node
  • Return:
  • concatenated device path or NULL. Caller must free the returned value

@@ -287,7 +288,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2,

•                          bool split_end_node)
  

•                          size_t split_end_node)
  

{ struct efi_device_path *ret; size_t end_size; @@ -301,10 +302,15 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, ret = efi_dp_dup(dp1); } else { /* both dp1 and dp2 are non-null */

•           unsigned sz1 = efi_dp_size(dp1);
  

•           unsigned sz2 = efi_dp_size(dp2);
  

•           size_t sz1;
  

•           size_t sz2 = efi_dp_size(dp2);
            void *p;
  
  

•           if (split_end_node < sizeof(struct efi_device_path))
  

Can we be more explicit here pls? Someone might misuse this in the future split_end_node < =1 ? And we can document we can use values up to sizeof(struct efi_device_path) if we ever need extra functionality

•                   sz1 = efi_dp_size(dp1);
  

•           else
  

•                   sz1 = split_end_node;
  

•           if (split_end_node)
                    end_size = 2 * sizeof(END);
            else
  

diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..ac250bbfcc9 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);

•   return EFI_EXIT(efi_dp_concat(src1, src2, false));
  

•   return EFI_EXIT(efi_dp_concat(src1, src2, 0));
  

}

/*

2.43.0

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org