Re: [PATCH v2] loader/efi/chainloader: Add efidriver command

[Mate Kukri]

I think this is not really compatible with shim signing, which is how
UEFI GRUB is usually used in the Linux world.
Are you trying to get an MS UEFI CA signed dtbloader and use that with GRUB?

The planned shim loader protocol changes will break that when used
with shim, because all LoadImage calls would go through shim's hook at
that point.
Can this dtb selection functionality not be implemented in GRUB instead?

On Tue, Sep 24, 2024 at 6:31 PM Nikita Travkin via Grub-devel
<grub-devel@gnu.org> wrote:
>
> Sometimes it's useful to load EFI drivers. Since loading a driver is
> the same as starting it and UEFI handles cleanup after on it's own, we
> can piggy back on existing chainloader command and just start the image
> immediately instead of defering to "boot". Conveniently this also means
> that grub can also run plain efi applications with the new command,
> though the command is still called "efidriver" since it's the primary
> intended usecase.
>
> Refactor chainloader to split up image loading and starting from the
> command and loader functions and add a new "efidriver" command that
> executes those steps immediately.
>
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> ---
> This patch adds EFI driver loading support to grub.
>
> Similar feature already exists in systemd-boot in a form of a dedicated
> directory that it loads drivers from.
>
> A few specific examples where it can be necessary:
>
> Loading devicetree
> ------------------
>
> Linux can't rely on ACPI on most currently existing
> Windows-on-Snapdragon devices and instead makes use of devicetree. This
> means that something has to load said devicetree and provide it to the
> kerne. GRUB has a "devicetree" command for that but it has a couple of
> shortcomings:
>
>  - One has to know beforehand what dtb they want to load;
>  - This command doesn't verify the dtb and can't be used with UEFI
>    secure-boot.
>
> Instead dtb could be loaded by a dedicated efi driver such as
> dtbloader [1]. Such driver could dynamically detect the device and load
> appropriate dtb and since it's a normal efi driver, it's verified with
> secure-boot, and itself can implement some security model to safely load
> dtbs.
>
> This would allow distro image makers to create generic os/installer
> images instead of adding device-specific hacks or asking the users for
> manual action to load the dtb.
>
> Peforming pre-boot firmware hacks
> ---------------------------------
>
> Most currently shipping WoS devices boot UEFI (and thus Linux) in EL1
> which means Linux can't use KVM and virtualization. Windows has a custom
> api to switch to EL2. To work around that, custom efi driver,
> slbounce[2] can be used. This driver hooks into UEFI boot services and
> performs EL1->EL2 transition upon EBS.
>
> Adding "efidriver" command to GRUB would make it easy to "dual boot"
> EL1 and EL2 modes for users since at the moment Linux can't use some
> hardware while running in EL2.
>
> [1] https://github.com/TravMurav/dtbloader
> [2] https://github.com/TravMurav/slbounce
> ---
> Changes in v2:
> - Align the changes with correct code style (Avnish)
> - Suppress EFI_ABORTED when loading efi drivers
> - Link to v1: 
> https://lore.kernel.org/r/20240922-cmd-efidriver-v1-1-750b77bfca0a@trvn.ru
> ---
>  grub-core/loader/efi/chainloader.c | 73 
> +++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/grub-core/loader/efi/chainloader.c 
> b/grub-core/loader/efi/chainloader.c
> index 869307bf3..77185e3aa 100644
> --- a/grub-core/loader/efi/chainloader.c
> +++ b/grub-core/loader/efi/chainloader.c
> @@ -19,6 +19,7 @@
>
>  /* TODO: support load options.  */
>
> +#include <stdbool.h>
>  #include <grub/loader.h>
>  #include <grub/file.h>
>  #include <grub/err.h>
> @@ -64,9 +65,8 @@ grub_chainloader_unload (void *context)
>  }
>
>  static grub_err_t
> -grub_chainloader_boot (void *context)
> +start_efi_image (grub_efi_handle_t image_handle, bool driver)
>  {
> -  grub_efi_handle_t image_handle = (grub_efi_handle_t) context;
>    grub_efi_boot_services_t *b;
>    grub_efi_status_t status;
>    grub_efi_uintn_t exit_data_size;
> @@ -74,7 +74,9 @@ grub_chainloader_boot (void *context)
>
>    b = grub_efi_system_table->boot_services;
>    status = b->start_image (image_handle, &exit_data_size, &exit_data);
> -  if (status != GRUB_EFI_SUCCESS)
> +  /* EFI_ABORTED is a normal return code for "initializing" drivers
> +     which indicates that "the driver did what it wanted and unloaded" */
> +  if (status != GRUB_EFI_SUCCESS && !(driver && status == GRUB_EFI_ABORTED))
>      {
>        if (exit_data)
>         {
> @@ -97,9 +99,20 @@ grub_chainloader_boot (void *context)
>    if (exit_data)
>      b->free_pool (exit_data);
>
> +  return grub_errno;
> +}
> +
> +static grub_err_t
> +grub_chainloader_boot (void *context)
> +{
> +  grub_efi_handle_t image_handle = (grub_efi_handle_t) context;
> +  grub_err_t ret;
> +
> +  ret = start_efi_image (image_handle, false);
> +
>    grub_loader_unset ();
>
> -  return grub_errno;
> +  return ret;
>  }
>
>  static grub_err_t
> @@ -209,8 +222,7 @@ make_file_path (grub_efi_device_path_t *dp, const char 
> *filename)
>  }
>
>  static grub_err_t
> -grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
> -                     int argc, char *argv[])
> +load_efi_image (int argc, char *argv[], grub_efi_handle_t *image_ret)
>  {
>    grub_file_t file = 0;
>    grub_ssize_t size;
> @@ -400,8 +412,8 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ 
> ((unused)),
>    b->free_pages (address, pages);
>    grub_free (file_path);
>
> -  grub_loader_set_ex (grub_chainloader_boot, grub_chainloader_unload, 
> image_handle, 0);
> -  return 0;
> +  *image_ret = image_handle;
> +  return GRUB_ERR_NONE;
>
>   fail:
>
> @@ -425,16 +437,53 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ 
> ((unused)),
>    return grub_errno;
>  }
>
> -static grub_command_t cmd;
> +static grub_err_t
> +grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
> +                     int argc, char *argv[])
> +{
> +  grub_efi_handle_t image_handle = NULL;
> +  grub_err_t ret;
> +
> +  ret = load_efi_image (argc, argv, &image_handle);
> +  if (ret != GRUB_ERR_NONE)
> +    return ret;
> +
> +  grub_loader_set_ex (grub_chainloader_boot, grub_chainloader_unload, 
> image_handle, 0);
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_efidriver (grub_command_t cmd __attribute__ ((unused)),
> +                   int argc, char *argv[])
> +{
> +  grub_efi_handle_t image_handle = NULL;
> +  grub_err_t ret;
> +
> +  ret = load_efi_image (argc, argv, &image_handle);
> +  if (ret != GRUB_ERR_NONE)
> +    return ret;
> +
> +  ret = start_efi_image (image_handle, true);
> +
> +  grub_dl_unref (my_mod);
> +
> +  return ret;
> +}
> +
> +static grub_command_t cmd_chainloader;
> +static grub_command_t cmd_efidriver;
>
>  GRUB_MOD_INIT(chainloader)
>  {
> -  cmd = grub_register_command ("chainloader", grub_cmd_chainloader,
> -                              0, N_("Load another boot loader."));
> +  cmd_chainloader = grub_register_command ("chainloader", 
> grub_cmd_chainloader,
> +                                          0, N_("Load another boot 
> loader."));
> +  cmd_efidriver = grub_register_command ("efidriver", grub_cmd_efidriver,
> +                                        0, N_("Load a efi driver."));
>    my_mod = mod;
>  }
>
>  GRUB_MOD_FINI(chainloader)
>  {
> -  grub_unregister_command (cmd);
> +  grub_unregister_command (cmd_chainloader);
> +  grub_unregister_command (cmd_efidriver);
>  }
>
> ---
> base-commit: 9c34d56c2dafcd2737db0e3e49df63bce4d8b504
> change-id: 20240922-cmd-efidriver-5b644a784851
>
> Best regards,
> --
> Nikita Travkin <nikita@trvn.ru>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel