[XEN PATCH v2] efi: Use Shim's LoadImage to verify the Dom0 kernel

[XEN PATCH v2] efi: Use Shim's LoadImage to verify the Dom0 kernel

[Gerald Elder-Vass]

The existing Verify functionality of the Shim lock protocol is
deprecated and will be removed, instead we must use the LoadImage
interface to perform the verification.

When the loading is successful we won't be using the newly loaded image
(as of yet) so we must then immediately unload the image to clean up.

Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
---
Changes since v1:
- Re-instated SHIM_LOCK_PROTOCOL as a fallback option if IMAGE_LOADER is
  not found
- Fixed indentation and error messages
---
 xen/common/efi/boot.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 453b1ba099cd..273da3d9e3e3 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -38,6 +38,8 @@
   { 0xf2fd1544U, 0x9794, 0x4a2c, {0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94} }
 #define SHIM_LOCK_PROTOCOL_GUID \
   { 0x605dab50U, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
+#define SHIM_IMAGE_LOADER_GUID \
+  { 0x1f492041U, 0xfadb, 0x4e59, {0x9e, 0x57, 0x7c, 0xaf, 0xe7, 0x3a, 0x55, 0xab} }
 #define APPLE_PROPERTIES_PROTOCOL_GUID \
   { 0x91bd12feU, 0xf6c3, 0x44fb, {0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} }
 #define EFI_SYSTEM_RESOURCE_TABLE_GUID    \
@@ -70,6 +72,13 @@ typedef struct {
     EFI_SHIM_LOCK_VERIFY Verify;
 } EFI_SHIM_LOCK_PROTOCOL;
 
+typedef struct _SHIM_IMAGE_LOADER {
+    EFI_IMAGE_LOAD LoadImage;
+    EFI_IMAGE_START StartImage;
+    EFI_EXIT Exit;
+    EFI_IMAGE_UNLOAD UnloadImage;
+} SHIM_IMAGE_LOADER;
+
 struct _EFI_APPLE_PROPERTIES;
 
 typedef EFI_STATUS
@@ -1336,10 +1345,12 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
+    EFI_HANDLE loaded_kernel;
     unsigned int i;
     CHAR16 *file_name, *cfg_file_name = NULL, *options = NULL;
     UINTN gop_mode = ~0;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
+    SHIM_IMAGE_LOADER *shim_loader;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     union string section = { NULL }, name;
     bool base_video = false;
@@ -1590,12 +1601,32 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
      * device tree through the efi_check_dt_boot function, in this stage
      * verify it.
      */
-    if ( kernel.ptr &&
-         !kernel_verified &&
-         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
-                                           (void **)&shim_lock)) &&
-         (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
-        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
+    if ( kernel.ptr && !kernel_verified )
+    {
+
+        if ( !EFI_ERROR(efi_bs->LocateProtocol(&((EFI_GUID) SHIM_IMAGE_LOADER_GUID),
+                                               NULL, (void **)&shim_loader)) )
+        {
+            status = shim_loader->LoadImage(false, ImageHandle, NULL, (void *)kernel.ptr, kernel.size, &loaded_kernel);
+            if ( EFI_ERROR(status) )
+                PrintErrMesg(L"LoadImage failed", status);
+
+            // LoadImage performs verification, now unload it to clean up
+            status = shim_loader->UnloadImage(loaded_kernel);
+            if ( EFI_ERROR(status) )
+                PrintErrMesg(L"UnloadImage failed", status);
+        }
+        else
+        {
+            status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, (void **)&shim_lock);
+            if ( EFI_ERROR(status) )
+                PrintErrMesg(L"Failed to locate SHIM_LOCK protocol", status);
+
+            status = shim_lock->Verify(kernel.ptr, kernel.size);
+            if ( EFI_ERROR(status) )
+                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
+        }
+    }
 
     efi_arch_edd();
 
-- 
2.47.3

Re: [XEN PATCH v2] efi: Use Shim's LoadImage to verify the Dom0 kernel

[Jan Beulich]

On 02.09.2025 16:44, Gerald Elder-Vass wrote:
> The existing Verify functionality of the Shim lock protocol is
> deprecated and will be removed, instead we must use the LoadImage
> interface to perform the verification.
> 
> When the loading is successful we won't be using the newly loaded image
> (as of yet) so we must then immediately unload the image to clean up.
> 
> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
> Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
> ---
> Changes since v1:
> - Re-instated SHIM_LOCK_PROTOCOL as a fallback option if IMAGE_LOADER is
>   not found
> - Fixed indentation and error messages
> ---
>  xen/common/efi/boot.c | 43 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 453b1ba099cd..273da3d9e3e3 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -38,6 +38,8 @@
>    { 0xf2fd1544U, 0x9794, 0x4a2c, {0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94} }
>  #define SHIM_LOCK_PROTOCOL_GUID \
>    { 0x605dab50U, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
> +#define SHIM_IMAGE_LOADER_GUID \
> +  { 0x1f492041U, 0xfadb, 0x4e59, {0x9e, 0x57, 0x7c, 0xaf, 0xe7, 0x3a, 0x55, 0xab} }
>  #define APPLE_PROPERTIES_PROTOCOL_GUID \
>    { 0x91bd12feU, 0xf6c3, 0x44fb, {0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} }
>  #define EFI_SYSTEM_RESOURCE_TABLE_GUID    \
> @@ -70,6 +72,13 @@ typedef struct {
>      EFI_SHIM_LOCK_VERIFY Verify;
>  } EFI_SHIM_LOCK_PROTOCOL;
>  
> +typedef struct _SHIM_IMAGE_LOADER {
> +    EFI_IMAGE_LOAD LoadImage;
> +    EFI_IMAGE_START StartImage;
> +    EFI_EXIT Exit;
> +    EFI_IMAGE_UNLOAD UnloadImage;
> +} SHIM_IMAGE_LOADER;
> +
>  struct _EFI_APPLE_PROPERTIES;
>  
>  typedef EFI_STATUS
> @@ -1336,10 +1345,12 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>      static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;

Please put the new GUID here, rather than using a compound literal below.

>      EFI_LOADED_IMAGE *loaded_image;
>      EFI_STATUS status;
> +    EFI_HANDLE loaded_kernel;

This variable wants to move into the more narrow scope you introduce.

>      unsigned int i;
>      CHAR16 *file_name, *cfg_file_name = NULL, *options = NULL;
>      UINTN gop_mode = ~0;
>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> +    SHIM_IMAGE_LOADER *shim_loader;

Same for this new one as well as shim_lock, as it looks.

> @@ -1590,12 +1601,32 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>       * device tree through the efi_check_dt_boot function, in this stage
>       * verify it.
>       */
> -    if ( kernel.ptr &&
> -         !kernel_verified &&
> -         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> -                                           (void **)&shim_lock)) &&
> -         (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> -        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> +    if ( kernel.ptr && !kernel_verified )
> +    {
> +

Nit: Stray blank line.

> +        if ( !EFI_ERROR(efi_bs->LocateProtocol(&((EFI_GUID) SHIM_IMAGE_LOADER_GUID),
> +                                               NULL, (void **)&shim_loader)) )
> +        {
> +            status = shim_loader->LoadImage(false, ImageHandle, NULL, (void *)kernel.ptr, kernel.size, &loaded_kernel);

Please pay attention to line length (see ./CODING_STYLE, also for other style
aspects, like ...

> +            if ( EFI_ERROR(status) )
> +                PrintErrMesg(L"LoadImage failed", status);
> +
> +            // LoadImage performs verification, now unload it to clean up

... comment format).

> +            status = shim_loader->UnloadImage(loaded_kernel);
> +            if ( EFI_ERROR(status) )
> +                PrintErrMesg(L"UnloadImage failed", status);

Does this really need to be fatal?

> +        }
> +        else
> +        {
> +            status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, (void **)&shim_lock);
> +            if ( EFI_ERROR(status) )
> +                PrintErrMesg(L"Failed to locate SHIM_LOCK protocol", status);

This is a behavioral change not justified in the description. Imo, if
the original code was wrong, that would want to be a separate change
anyway, so right here you want to retain original behavior. Simply
consider the case of a shim-free boot, where neither of the two
protocols would be available.

Jan

> +            status = shim_lock->Verify(kernel.ptr, kernel.size);
> +            if ( EFI_ERROR(status) )
> +                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> +        }
> +    }
>  
>      efi_arch_edd();
>  

Re: [XEN PATCH v2] efi: Use Shim's LoadImage to verify the Dom0 kernel

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Tue, Sep 02, 2025 at 05:00:52PM +0200, Jan Beulich wrote:
> On 02.09.2025 16:44, Gerald Elder-Vass wrote:
> > +        else
> > +        {
> > +            status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, (void **)&shim_lock);
> > +            if ( EFI_ERROR(status) )
> > +                PrintErrMesg(L"Failed to locate SHIM_LOCK protocol", status);
> 
> This is a behavioral change not justified in the description. Imo, if
> the original code was wrong, that would want to be a separate change
> anyway, so right here you want to retain original behavior. Simply
> consider the case of a shim-free boot, where neither of the two
> protocols would be available.

Yes, as commented by Yann on v1, this change as is seems to break
shim-free boot (well, technically UKI is shim-free and remain working,
but you know what I mean). That needs to remain working, even if only in
SecureBoot-free case.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]