[PATCH v2 08/11] efi_loader: move file-system helpers to efi_app_common

[Simon Glass]

Hi Heinrich,

On 2026-06-21T08:19:04, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
> efi_loader: move file-system helpers to efi_app_common
>
> Move the shared file-system utilities to efi_app_common:
>
> * open_file_system() - open the simple file system protocol, first
>   trying the partition the image was loaded from, then the UEFI system
>   partition.
> * get_config_table()  - look up an EFI configuration table by GUID.
> * save_file()         - write a buffer to a named file, prompting for
>   confirmation if the file already exists.
>
> get_config_table() supersedes the private get_dtb() in dtbdump and
> get_dbg_info() in dbginfodump; both call-sites are updated to use it.
>
> dtbdump's do_save() and initrddump's do_save() are simplified to use
> the common save_file(), eliminating duplicate file-open/write/close
> sequences.
>
> smbiosdump's private get_config_table(), open_file_system() and
> save_file() are removed; the common versions take their place.
> [...]
>
> lib/efi_loader/dbginfodump.c    |  22 +------
>  lib/efi_loader/dtbdump.c        | 120 ++------------------------------------
>  lib/efi_loader/efi_app_common.c | 105 +++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_app_common.h |  27 +++++++++
>  lib/efi_loader/initrddump.c     |  69 ++--------------------
>  lib/efi_loader/smbiosdump.c     | 125 ----------------------------------------
>  6 files changed, 141 insertions(+), 327 deletions(-)

> diff --git a/lib/efi_loader/efi_app_common.c b/lib/efi_loader/efi_app_common.c
> @@ -199,3 +203,104 @@ efi_status_t efi_input(u16 *buffer, efi_uintn_t buffer_size)
> +void *get_config_table(const efi_guid_t *guid)
> +{
> +     size_t i;
> +
> +     for (i = 0; i < systable->nr_tables; ++i) {
> +             if (!memcmp(guid, &systable->tables[i].guid, 16))
> +                     return systable->tables[i].table;
> +     }

Please use sizeof(efi_guid_t) rather than the literal 16 - that is
what get_dtb() and get_dbg_info() did before the move. The 16 was
inherited from the smbiosdump copy.

> diff --git a/lib/efi_loader/efi_app_common.c b/lib/efi_loader/efi_app_common.c
> @@ -199,3 +203,104 @@ efi_status_t efi_input(u16 *buffer, efi_uintn_t buffer_size)
> +efi_status_t save_file(u16 *filename, void *buf, efi_uintn_t size)
> +{
> +     efi_uintn_t ret;

ret should be efi_status_t to match the return type.

> diff --git a/lib/efi_loader/efi_app_common.c b/lib/efi_loader/efi_app_common.c
> @@ -199,3 +203,104 @@ efi_status_t efi_input(u16 *buffer, efi_uintn_t buffer_size)
> +             if (ret != EFI_SUCCESS) {
> +                     root->close(root);
> +                     error(u"Aborted by user\r\n");
> +                     return ret;
> +             }

The old smbiosdump save_file() did bs->free_pool(buf) on this abort
path while the caller also freed buf unconditionally - a double-free.
Dropping the inner free here quietly fixes that. Worth a sentence in
the commit message so the change in ownership (caller now always owns
buf) is explicit.

> diff --git a/lib/efi_loader/efi_app_common.h b/lib/efi_loader/efi_app_common.h
> @@ -101,4 +101,31 @@ efi_status_t efi_input_yn(void);
> +/**
> + * save_file() - save buffer to a file on the EFI system partition
> + *
> + * @filename:   file name
> + * @buf:        buffer to write
> + * @size:       size of the buffer
> + * Return:      status code
> + */
> +efi_status_t save_file(u16 *filename, void *buf, efi_uintn_t size);

The description is misleading - open_file_system() tries the loaded
image's own partition first and only falls back to the ESP. Please
reword along the lines of 'save buffer to a file on the loaded image's
partition, falling back to the EFI system partition'.

Regards,
Simon