On Mon, Mar 10, 2025 at 09:12:22AM +0000, Mate Kukri wrote:
> - Use shim loader protocol to verify images in the shim_lock verifier.
This change makes the shim loader protocol mandatory for GRUB under UEFI
Secure Boot. However, I don't think this is a good idea, since the new
protocol is only available starting from shim 16.0. GRUB should remain
compatible with older shim versions, otherwise this could introduce a
disruptive change.
Why not implement a fallback mechanism to use the shim lock protocol
when the loader protocol is unavailable ?
Also see below for an issue:
> - Add API to allow downstream consumers to re-use image handles produced
> by the verifier. This is necessary to avoid having images measured
> twice to the TPM.
> - Register shim loader protocol as an image loader.
>
> Signed-off-by: Mate Kukri <mate.ku...@canonical.com>
> ---
> grub-core/kern/efi/sb.c | 45 ++++++++++++++++--------------
> grub-core/loader/efi/chainloader.c | 21 ++++++++------
> grub-core/loader/efi/linux.c | 30 ++++++--------------
> include/grub/efi/api.h | 5 ++++
> include/grub/efi/efi.h | 19 ++++++++-----
> include/grub/efi/sb.h | 5 ++--
> 6 files changed, 66 insertions(+), 59 deletions(-)
>
> diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c
> index 8d3e41360..fcc71e4e8 100644
> --- a/grub-core/kern/efi/sb.c
> +++ b/grub-core/kern/efi/sb.c
> @@ -31,8 +31,11 @@
> #include <grub/verify.h>
>
> static grub_guid_t shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
> +static grub_guid_t shim_loader_guid = GRUB_EFI_SHIM_IMAGE_LOADER_GUID;
>
> -static bool shim_lock_enabled = false;
> +static grub_efi_loader_t *shim_loader = NULL;
> +
> +static grub_efi_handle_t last_verified_image_handle;
>
> /*
> * Determine whether we're in secure boot mode.
> @@ -95,14 +98,6 @@ grub_efi_get_secureboot (void)
> if (!(attr & GRUB_EFI_VARIABLE_RUNTIME_ACCESS) && *moksbstate == 1)
> {
> secureboot = GRUB_EFI_SECUREBOOT_MODE_DISABLED;
> - /*
> - * TODO: Replace this all with shim's LoadImage protocol, delegating
> policy to it.
> - *
> - * We need to set shim_lock_enabled here because we disabled secure
> boot
> - * validation *inside* shim but not in the firmware, so we set this
> variable
> - * here to trigger that code path, whereas the actual verifier is not
> enabled.
> - */
> - shim_lock_enabled = true;
> goto out;
> }
>
> @@ -183,14 +178,18 @@ shim_lock_verifier_init (grub_file_t io __attribute__
> ((unused)),
> static grub_err_t
> shim_lock_verifier_write (void *context __attribute__ ((unused)), void *buf,
> grub_size_t size)
> {
> - grub_efi_shim_lock_protocol_t *sl = grub_efi_locate_protocol
> (&shim_lock_guid, 0);
> + grub_efi_handle_t image_handle;
>
> - if (!sl)
> - return grub_error (GRUB_ERR_ACCESS_DENIED, N_("shim_lock protocol not
> found"));
> + if (last_verified_image_handle)
> + {
> + shim_loader->unload_image (last_verified_image_handle);
> + last_verified_image_handle = NULL;
> + }
>
> - if (sl->verify (buf, size) != GRUB_EFI_SUCCESS)
> + if (shim_loader->load_image (false, grub_efi_image_handle, NULL, buf,
> size, &image_handle) != GRUB_EFI_SUCCESS)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad shim signature"));
.. and here may cause a NULL pointer dereference if shim_loader is not
provided by the shim.
Thanks,
Michael
>
> + last_verified_image_handle = image_handle;
> return GRUB_ERR_NONE;
> }
>
> @@ -205,11 +204,11 @@ void
> grub_shim_lock_verifier_setup (void)
> {
> struct grub_module_header *header;
> - grub_efi_shim_lock_protocol_t *sl =
> - grub_efi_locate_protocol (&shim_lock_guid, 0);
>
> - /* shim_lock is missing, check if GRUB image is built with
> --disable-shim-lock. */
> - if (!sl)
> + shim_loader = grub_efi_locate_protocol (&shim_loader_guid, 0);
> +
> + /* shim is missing, check if GRUB image is built with --disable-shim-lock.
> */
> + if (!shim_loader)
> {
> FOR_MODULES (header)
> {
> @@ -225,14 +224,18 @@ grub_shim_lock_verifier_setup (void)
> /* Enforce shim_lock_verifier. */
> grub_verifier_register (&shim_lock_verifier);
>
> - shim_lock_enabled = true;
> + /* Register shim loader if supported. */
> + grub_efi_register_loader (shim_loader);
>
> grub_env_set ("shim_lock", "y");
> grub_env_export ("shim_lock");
> }
>
> -bool
> -grub_is_shim_lock_enabled (void)
> +
> +grub_efi_handle_t
> +grub_efi_get_last_verified_image_handle (void)
> {
> - return shim_lock_enabled;
> + grub_efi_handle_t tmp = last_verified_image_handle;
> + last_verified_image_handle = NULL;
> + return tmp;
> }
> diff --git a/grub-core/loader/efi/chainloader.c
> b/grub-core/loader/efi/chainloader.c
> index 11b64ac1b..e77bd863c 100644
> --- a/grub-core/loader/efi/chainloader.c
> +++ b/grub-core/loader/efi/chainloader.c
> @@ -33,6 +33,7 @@
> #include <grub/efi/efi.h>
> #include <grub/efi/disk.h>
> #include <grub/efi/memory.h>
> +#include <grub/efi/sb.h>
> #include <grub/command.h>
> #include <grub/i18n.h>
> #include <grub/net.h>
> @@ -337,16 +338,20 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__
> ((unused)),
> }
> #endif
>
> - status = grub_efi_load_image (0, grub_efi_image_handle, file_path,
> - boot_image, size, &image_handle);
> - if (status != GRUB_EFI_SUCCESS)
> + image_handle = grub_efi_get_last_verified_image_handle ();
> + if (image_handle == NULL)
> {
> - if (status == GRUB_EFI_OUT_OF_RESOURCES)
> - grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of resources");
> - else
> - grub_error (GRUB_ERR_BAD_OS, "cannot load image");
> + status = grub_efi_load_image (0, grub_efi_image_handle, file_path,
> + boot_image, size, &image_handle);
> + if (status != GRUB_EFI_SUCCESS)
> + {
> + if (status == GRUB_EFI_OUT_OF_RESOURCES)
> + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of resources");
> + else
> + grub_error (GRUB_ERR_BAD_OS, "cannot load image");
>
> - goto fail;
> + goto fail;
> + }
> }
>
> /* LoadImage does not set a device handler when the image is
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index 7342f3ee7..b97b3e27b 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -206,11 +206,15 @@ grub_arch_efi_linux_boot_image (grub_addr_t addr,
> grub_size_t size, char *args)
> mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
> mempath[1].header.length = sizeof (grub_efi_device_path_t);
>
> - status = grub_efi_load_image (0, grub_efi_image_handle,
> - (grub_efi_device_path_t *)mempath,
> - (void *)addr, size, &image_handle);
> - if (status != GRUB_EFI_SUCCESS)
> - return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
> + image_handle = grub_efi_get_last_verified_image_handle ();
> + if (image_handle == NULL)
> + {
> + status = grub_efi_load_image (0, grub_efi_image_handle,
> + (grub_efi_device_path_t *)mempath,
> + (void *)addr, size, &image_handle);
> + if (status != GRUB_EFI_SUCCESS)
> + return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
> + }
>
> grub_dprintf ("linux", "linux command line: '%s'\n", args);
>
> @@ -461,22 +465,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
>
> grub_dl_ref (my_mod);
>
> - if (grub_is_shim_lock_enabled () == true)
> - {
> -#if defined(__i386__) || defined(__x86_64__)
> - grub_dprintf ("linux", "shim_lock enabled, falling back to legacy
> Linux kernel loader\n");
> -
> - err = grub_cmd_linux_x86_legacy (cmd, argc, argv);
> -
> - if (err == GRUB_ERR_NONE)
> - return GRUB_ERR_NONE;
> - else
> - goto fail;
> -#else
> - grub_dprintf ("linux", "shim_lock enabled, trying Linux kernel EFI
> stub loader\n");
> -#endif
> - }
> -
> if (argc == 0)
> {
> grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index b686e8afe..9ae908729 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -364,6 +364,11 @@
> { 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23 } \
> }
>
> +#define GRUB_EFI_SHIM_IMAGE_LOADER_GUID \
> + { 0x1f492041, 0xfadb, 0x4e59, \
> + {0x9e, 0x57, 0x7c, 0xaf, 0xe7, 0x3a, 0x55, 0xab } \
> + }
> +
> #define GRUB_EFI_RNG_PROTOCOL_GUID \
> { 0x3152bca5, 0xeade, 0x433d, \
> { 0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44 } \
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 692a1b935..b4d2f2d46 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -150,15 +150,20 @@ EXPORT_FUNC (grub_efi_unload_image) (grub_efi_handle_t
> image_handle);
> typedef struct grub_efi_loader
> {
> grub_efi_status_t (__grub_efi_api *load_image) (grub_efi_boolean_t
> boot_policy,
> - grub_efi_handle_t parent_image_handle,
> - grub_efi_device_path_t *file_path,
> - void *source_buffer,
> - grub_efi_uintn_t source_size,
> - grub_efi_handle_t *image_handle);
> + grub_efi_handle_t
> parent_image_handle,
> + grub_efi_device_path_t
> *file_path,
> + void *source_buffer,
> + grub_efi_uintn_t source_size,
> + grub_efi_handle_t
> *image_handle);
>
> grub_efi_status_t (__grub_efi_api *start_image) (grub_efi_handle_t
> image_handle,
> - grub_efi_uintn_t *exit_data_size,
> - grub_efi_char16_t **exit_data);
> + grub_efi_uintn_t
> *exit_data_size,
> + grub_efi_char16_t
> **exit_data);
> +
> + grub_efi_status_t (__grub_efi_api *exit) (grub_efi_handle_t image_handle,
> + grub_efi_status_t exit_status,
> + grub_efi_uintn_t exit_data_size,
> + grub_efi_char16_t *exit_data);
>
> grub_efi_status_t (__grub_efi_api *unload_image) (grub_efi_handle_t
> image_handle);
> } grub_efi_loader_t;
> diff --git a/include/grub/efi/sb.h b/include/grub/efi/sb.h
> index 49a9ad01c..284e14992 100644
> --- a/include/grub/efi/sb.h
> +++ b/include/grub/efi/sb.h
> @@ -21,6 +21,7 @@
>
> #include <grub/types.h>
> #include <grub/dl.h>
> +#include <grub/efi/api.h>
>
> #define GRUB_EFI_SECUREBOOT_MODE_UNSET 0
> #define GRUB_EFI_SECUREBOOT_MODE_UNKNOWN 1
> @@ -31,8 +32,8 @@
> extern grub_uint8_t
> EXPORT_FUNC (grub_efi_get_secureboot) (void);
>
> -extern bool
> -EXPORT_FUNC (grub_is_shim_lock_enabled) (void);
> +extern grub_efi_handle_t
> +EXPORT_FUNC (grub_efi_get_last_verified_image_handle) (void);
>
> extern void
> grub_shim_lock_verifier_setup (void);
> --
> 2.39.5
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
