[PATCH v3 0/2] efi: Support Shim LoadImage

[PATCH v3 0/2] efi: Support Shim LoadImage

[Gerald Elder-Vass]

Support Shim LoadImage protocol but keep Shim Lock for compatibility

https://gitlab.com/xen-project/people/geraldev/xen/-/pipelines/2023640279

Gerald Elder-Vass (1):
  efi: Support using Shim's LoadImage protocol

Ross Lagerwall (1):
  efi: Add a function to check if Secure Boot mode is enabled

 xen/common/efi/boot.c    | 83 ++++++++++++++++++++++++++++++++++++----
 xen/common/efi/runtime.c |  1 +
 xen/include/xen/efi.h    |  2 +
 3 files changed, 78 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH v4 0/2] efi: Support Shim LoadImage

[Gerald Elder-Vass]

Support Shim LoadImage protocol but keep Shim Lock for compatibility

https://gitlab.com/xen-project/people/geraldev/xen/-/pipelines/2023841743

Gerald Elder-Vass (1):
  efi: Support using Shim's LoadImage protocol

Ross Lagerwall (1):
  efi: Add a function to check if Secure Boot mode is enabled

 xen/common/efi/boot.c    | 83 ++++++++++++++++++++++++++++++++++++----
 xen/common/efi/runtime.c |  1 +
 xen/include/xen/efi.h    |  2 +
 3 files changed, 78 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH v3 1/2] efi: Add a function to check if Secure Boot mode is enabled

[Gerald Elder-Vass]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Also cache it to avoid needing to repeatedly ask the firmware.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
---
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: "Daniel P. Smith" <dpsmith@apertussolutions.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Julien Grall <julien@xen.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

v3:
- Fix build on ARM
---
 xen/common/efi/boot.c    | 24 ++++++++++++++++++++++++
 xen/common/efi/runtime.c |  1 +
 xen/include/xen/efi.h    |  2 ++
 3 files changed, 27 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index e12fa1a7ec04..e7e3dffa7ddc 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
                    " last line will be ignored.\r\n");
 }
 
+static void __init init_secure_boot_mode(void)
+{
+    static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
+    EFI_STATUS status;
+    uint8_t data = 0;
+    UINTN size = sizeof(data);
+    UINT32 attr = 0;
+
+    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
+                                 &size, &data);
+
+    if ( status == EFI_NOT_FOUND ||
+         (status == EFI_SUCCESS &&
+          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) &&
+          size == 1 && data == 0) )
+        /* Platform does not support Secure Boot or it's disabled. */
+        efi_secure_boot = false;
+    else
+        /* Everything else play it safe and assume enabled. */
+        efi_secure_boot = true;
+}
+
 static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     efi_ih = ImageHandle;
@@ -915,6 +937,8 @@ static void __init efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTabl
 
     StdOut = SystemTable->ConOut;
     StdErr = SystemTable->StdErr ?: StdOut;
+
+    init_secure_boot_mode();
 }
 
 static void __init efi_console_set_mode(void)
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 42386c6bde42..30d649ca5c1b 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -41,6 +41,7 @@ void efi_rs_leave(struct efi_rs_state *state);
 unsigned int __read_mostly efi_num_ct;
 const EFI_CONFIGURATION_TABLE *__read_mostly efi_ct;
 
+bool __ro_after_init efi_secure_boot;
 unsigned int __read_mostly efi_version;
 unsigned int __read_mostly efi_fw_revision;
 const CHAR16 *__read_mostly efi_fw_vendor;
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index 623ed2ccdf31..723cb8085270 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -36,6 +36,8 @@ static inline bool efi_enabled(unsigned int feature)
 }
 #endif
 
+extern bool efi_secure_boot;
+
 void efi_init_memory(void);
 bool efi_boot_mem_unused(unsigned long *start, unsigned long *end);
 bool efi_rs_using_pgtables(void);
-- 
2.47.3

[misra] Re: [PATCH v3 1/2] efi: Add a function to check if Secure Boot mode is enabled

[Andrew Cooper]

On 05/09/2025 11:05 am, Gerald Elder-Vass wrote:
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index e12fa1a7ec04..e7e3dffa7ddc 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
>                     " last line will be ignored.\r\n");
>  }
>  
> +static void __init init_secure_boot_mode(void)
> +{
> +    static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
> +    EFI_STATUS status;
> +    uint8_t data = 0;
> +    UINTN size = sizeof(data);
> +    UINT32 attr = 0;
> +
> +    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
> +                                 &size, &data);

This turns out to be a MISRA R7.4 violation, complaining about casing a
string literal to a non-const pointer.

The real problem here is that the EFI spec.  GetVariable() ought to take
a const CHAR16 *, but doesn't.

We could fix this with:

diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
index a616d1238aa4..56775d553109 100644
--- a/xen/include/efi/efiapi.h
+++ b/xen/include/efi/efiapi.h
@@ -224,7 +224,7 @@ VOID
 typedef
 EFI_STATUS
 (EFIAPI *EFI_GET_VARIABLE) (
-    IN CHAR16                       *VariableName,
+    IN const CHAR16                 *VariableName,
     IN EFI_GUID                     *VendorGuid,
     OUT UINT32                      *Attributes OPTIONAL,
     IN OUT UINTN                    *DataSize,

but I fear this might get some objections.

I don't think we want to be deviating every use of GetVariable() for a
problem ultimately outside of our control.

Another option would be to have a wrapper for GetVariable() which does
the cast once, which lets us deviate in one place only.

Thoughts?

~Andrew

Re: [misra] Re: [PATCH v3 1/2] efi: Add a function to check if Secure Boot mode is enabled

[Jan Beulich]

On 05.09.2025 12:36, Andrew Cooper wrote:
> On 05/09/2025 11:05 am, Gerald Elder-Vass wrote:
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index e12fa1a7ec04..e7e3dffa7ddc 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
>>                     " last line will be ignored.\r\n");
>>  }
>>  
>> +static void __init init_secure_boot_mode(void)
>> +{
>> +    static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
>> +    EFI_STATUS status;
>> +    uint8_t data = 0;
>> +    UINTN size = sizeof(data);
>> +    UINT32 attr = 0;
>> +
>> +    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
>> +                                 &size, &data);
> 
> This turns out to be a MISRA R7.4 violation, complaining about casing a
> string literal to a non-const pointer.
> 
> The real problem here is that the EFI spec.  GetVariable() ought to take
> a const CHAR16 *, but doesn't.
> 
> We could fix this with:
> 
> diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
> index a616d1238aa4..56775d553109 100644
> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h
> @@ -224,7 +224,7 @@ VOID
>  typedef
>  EFI_STATUS
>  (EFIAPI *EFI_GET_VARIABLE) (
> -    IN CHAR16                       *VariableName,
> +    IN const CHAR16                 *VariableName,
>      IN EFI_GUID                     *VendorGuid,
>      OUT UINT32                      *Attributes OPTIONAL,
>      IN OUT UINTN                    *DataSize,
> 
> but I fear this might get some objections.

The interface lacking the const in principle means that we can't rely on
there being implementations which actually do fiddle with the string.
Hence ...

> I don't think we want to be deviating every use of GetVariable() for a
> problem ultimately outside of our control.
> 
> Another option would be to have a wrapper for GetVariable() which does
> the cast once, which lets us deviate in one place only.

... this doesn't look like a viable route to me. (Nor a scalable one,
as down the road we then may need more such wrappers.)

> Thoughts?

Why not instead use

    static CHAR16 __initdata str_SecureBoot[] = L"SecureBoot";

and be done?

Jan

Re: [misra] Re: [PATCH v3 1/2] efi: Add a function to check if Secure Boot mode is enabled

[Andrew Cooper]

On 05/09/2025 11:44 am, Jan Beulich wrote:
> On 05.09.2025 12:36, Andrew Cooper wrote:
>> On 05/09/2025 11:05 am, Gerald Elder-Vass wrote:
>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>> index e12fa1a7ec04..e7e3dffa7ddc 100644
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
>>>                     " last line will be ignored.\r\n");
>>>  }
>>>  
>>> +static void __init init_secure_boot_mode(void)
>>> +{
>>> +    static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
>>> +    EFI_STATUS status;
>>> +    uint8_t data = 0;
>>> +    UINTN size = sizeof(data);
>>> +    UINT32 attr = 0;
>>> +
>>> +    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
>>> +                                 &size, &data);
>> This turns out to be a MISRA R7.4 violation, complaining about casing a
>> string literal to a non-const pointer.
>>
>> The real problem here is that the EFI spec.  GetVariable() ought to take
>> a const CHAR16 *, but doesn't.
>>
>> We could fix this with:
>>
>> diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
>> index a616d1238aa4..56775d553109 100644
>> --- a/xen/include/efi/efiapi.h
>> +++ b/xen/include/efi/efiapi.h
>> @@ -224,7 +224,7 @@ VOID
>>  typedef
>>  EFI_STATUS
>>  (EFIAPI *EFI_GET_VARIABLE) (
>> -    IN CHAR16                       *VariableName,
>> +    IN const CHAR16                 *VariableName,
>>      IN EFI_GUID                     *VendorGuid,
>>      OUT UINT32                      *Attributes OPTIONAL,
>>      IN OUT UINTN                    *DataSize,
>>
>> but I fear this might get some objections.
> The interface lacking the const in principle means that we can't rely on
> there being implementations which actually do fiddle with the string.

Well, the IN and absence of OUT does mean this in practice.

> Hence ...
>
>> I don't think we want to be deviating every use of GetVariable() for a
>> problem ultimately outside of our control.
>>
>> Another option would be to have a wrapper for GetVariable() which does
>> the cast once, which lets us deviate in one place only.
> ... this doesn't look like a viable route to me. (Nor a scalable one,
> as down the road we then may need more such wrappers.)
>
>> Thoughts?
> Why not instead use
>
>     static CHAR16 __initdata str_SecureBoot[] = L"SecureBoot";
>
> and be done?

I suppose, but that's still awkward to use.

~Andrew

[PATCH v3 2/2] efi: Support using Shim's LoadImage protocol

[Gerald Elder-Vass]

The existing Verify functionality of the Shim lock protocol is
deprecated and will be removed, the alternative it to 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.

If the LoadImage protocol isn't available then fall back to the Shim
Lock (Verify) interface.

Log when the kernel is not verified and fail if this occurs
when secure boot mode is enabled.

Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
---
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: "Daniel P. Smith" <dpsmith@apertussolutions.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Julien Grall <julien@xen.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>

v3:
- Use Shim Image by default, fall back to Shim Lock
---
 xen/common/efi/boot.c | 59 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 8 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index e7e3dffa7ddc..1f63473d264d 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
@@ -1047,6 +1056,46 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
     return gop_mode;
 }
 
+static void __init efi_verify_kernel(EFI_HANDLE ImageHandle)
+{
+    static EFI_GUID __initdata shim_image_guid = SHIM_IMAGE_LOADER_GUID;
+    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
+    SHIM_IMAGE_LOADER *shim_loader;
+    EFI_HANDLE loaded_kernel;
+    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
+    EFI_STATUS status;
+    bool verified = false;
+
+    /* Look for LoadImage first */
+    if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_image_guid, NULL,
+                                           (void **)&shim_loader)) )
+    {
+        status = shim_loader->LoadImage(false, ImageHandle, NULL,
+                                        (void *)kernel.ptr, kernel.size,
+                                        &loaded_kernel);
+        if ( !EFI_ERROR(status) )
+            verified = true;
+
+        /* LoadImage performed verification, now clean up with UnloadImage */
+        shim_loader->UnloadImage(loaded_kernel);
+    }
+
+    /* else fall back to Shim Lock */
+    if ( !verified &&
+         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
+                                           (void **)&shim_lock)) &&
+         !EFI_ERROR(shim_lock->Verify(kernel.ptr, kernel.size)) )
+        verified = true;
+
+    if ( !verified )
+    {
+        PrintStr(L"Kernel was not verified\n");
+
+        if ( efi_secure_boot )
+            blexit(L"Failed to verify kernel");
+    }
+}
+
 static void __init efi_tables(void)
 {
     unsigned int i;
@@ -1334,13 +1383,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
                                       EFI_SYSTEM_TABLE *SystemTable)
 {
     static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL;
-    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
     unsigned int i;
     CHAR16 *file_name, *cfg_file_name = NULL, *options = NULL;
     UINTN gop_mode = ~0;
-    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     union string section = { NULL }, name;
     bool base_video = false;
@@ -1591,12 +1638,8 @@ 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 )
+        efi_verify_kernel(ImageHandle);
 
     efi_arch_edd();
 
-- 
2.47.3

Re: [PATCH v3 2/2] efi: Support using Shim's LoadImage protocol

[Marek Marczykowski-Górecki]

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

On Fri, Sep 05, 2025 at 10:05:32AM +0000, Gerald Elder-Vass wrote:
> The existing Verify functionality of the Shim lock protocol is
> deprecated and will be removed, the alternative it to 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.
> 
> If the LoadImage protocol isn't available then fall back to the Shim
> Lock (Verify) interface.
> 
> Log when the kernel is not verified and fail if this occurs
> when secure boot mode is enabled.
> 
> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
> Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
> ---
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Julien Grall <julien@xen.org>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> 
> v3:
> - Use Shim Image by default, fall back to Shim Lock
> ---
>  xen/common/efi/boot.c | 59 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index e7e3dffa7ddc..1f63473d264d 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
> @@ -1047,6 +1056,46 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>      return gop_mode;
>  }
>  
> +static void __init efi_verify_kernel(EFI_HANDLE ImageHandle)
> +{
> +    static EFI_GUID __initdata shim_image_guid = SHIM_IMAGE_LOADER_GUID;
> +    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
> +    SHIM_IMAGE_LOADER *shim_loader;
> +    EFI_HANDLE loaded_kernel;
> +    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> +    EFI_STATUS status;
> +    bool verified = false;
> +
> +    /* Look for LoadImage first */
> +    if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_image_guid, NULL,
> +                                           (void **)&shim_loader)) )
> +    {
> +        status = shim_loader->LoadImage(false, ImageHandle, NULL,
> +                                        (void *)kernel.ptr, kernel.size,
> +                                        &loaded_kernel);
> +        if ( !EFI_ERROR(status) )
> +            verified = true;
> +
> +        /* LoadImage performed verification, now clean up with UnloadImage */
> +        shim_loader->UnloadImage(loaded_kernel);

Is UnloadImage really appropriate even if LoadImage failed?

> +    }
> +
> +    /* else fall back to Shim Lock */
> +    if ( !verified &&
> +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                                           (void **)&shim_lock)) &&
> +         !EFI_ERROR(shim_lock->Verify(kernel.ptr, kernel.size)) )
> +        verified = true;
> +
> +    if ( !verified )
> +    {
> +        PrintStr(L"Kernel was not verified\n");
> +
> +        if ( efi_secure_boot )
> +            blexit(L"Failed to verify kernel");

Better be more explicit why it's fatal, like "Refusing to boot
unverified kernel with UEFI SecureBoot enabled".

> +    }
> +}
> +
>  static void __init efi_tables(void)
>  {
>      unsigned int i;
> @@ -1334,13 +1383,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>                                        EFI_SYSTEM_TABLE *SystemTable)
>  {
>      static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL;
> -    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
>      EFI_LOADED_IMAGE *loaded_image;
>      EFI_STATUS status;
>      unsigned int i;
>      CHAR16 *file_name, *cfg_file_name = NULL, *options = NULL;
>      UINTN gop_mode = ~0;
> -    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      union string section = { NULL }, name;
>      bool base_video = false;
> @@ -1591,12 +1638,8 @@ 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 )
> +        efi_verify_kernel(ImageHandle);
>  
>      efi_arch_edd();
>  
> -- 
> 2.47.3
> 

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

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

Re: [PATCH v3 2/2] efi: Support using Shim's LoadImage protocol

[Gerald Elder-Vass]

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

 <forgot to use reply all!>

>  Is UnloadImage really appropriate even if LoadImage failed?
Yes, LoadImage can "fail" in a number of ways, some of which do load
the image into the EFI_HANDLE (e.g. image verification failure -
image is loaded then the verification fails and returns an error)
So UnloadImage is needed in many cases, I've intentionally ignored
its return value as it will only unload an image if there is an image to
unload.

> Better be more explicit why it's fatal, like "Refusing to boot
>unverified kernel with UEFI SecureBoot enabled".
I agree! Will update!

Thanks!

*Gerald Elder-Vass*
Senior Software Engineer

XenServer
Cambridge, UK


www.cloud.com


On Fri, Sep 5, 2025 at 12:14 PM Marek Marczykowski-Górecki <
marmarek@invisiblethingslab.com> wrote:

> On Fri, Sep 05, 2025 at 10:05:32AM +0000, Gerald Elder-Vass wrote:
> > The existing Verify functionality of the Shim lock protocol is
> > deprecated and will be removed, the alternative it to 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.
> >
> > If the LoadImage protocol isn't available then fall back to the Shim
> > Lock (Verify) interface.
> >
> > Log when the kernel is not verified and fail if this occurs
> > when secure boot mode is enabled.
> >
> > Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
> > Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
> > ---
> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > CC: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > CC: Anthony PERARD <anthony.perard@vates.tech>
> > CC: Michal Orzel <michal.orzel@amd.com>
> > CC: Julien Grall <julien@xen.org>
> > CC: "Roger Pau Monné" <roger.pau@citrix.com>
> > CC: Stefano Stabellini <sstabellini@kernel.org>
> >
> > v3:
> > - Use Shim Image by default, fall back to Shim Lock
> > ---
> >  xen/common/efi/boot.c | 59 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 51 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index e7e3dffa7ddc..1f63473d264d 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
> > @@ -1047,6 +1056,46 @@ static UINTN __init
> efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> >      return gop_mode;
> >  }
> >
> > +static void __init efi_verify_kernel(EFI_HANDLE ImageHandle)
> > +{
> > +    static EFI_GUID __initdata shim_image_guid = SHIM_IMAGE_LOADER_GUID;
> > +    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
> > +    SHIM_IMAGE_LOADER *shim_loader;
> > +    EFI_HANDLE loaded_kernel;
> > +    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> > +    EFI_STATUS status;
> > +    bool verified = false;
> > +
> > +    /* Look for LoadImage first */
> > +    if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_image_guid, NULL,
> > +                                           (void **)&shim_loader)) )
> > +    {
> > +        status = shim_loader->LoadImage(false, ImageHandle, NULL,
> > +                                        (void *)kernel.ptr, kernel.size,
> > +                                        &loaded_kernel);
> > +        if ( !EFI_ERROR(status) )
> > +            verified = true;
> > +
> > +        /* LoadImage performed verification, now clean up with
> UnloadImage */
> > +        shim_loader->UnloadImage(loaded_kernel);
>
> Is UnloadImage really appropriate even if LoadImage failed?
>
> > +    }
> > +
> > +    /* else fall back to Shim Lock */
> > +    if ( !verified &&
> > +         !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> > +                                           (void **)&shim_lock)) &&
> > +         !EFI_ERROR(shim_lock->Verify(kernel.ptr, kernel.size)) )
> > +        verified = true;
> > +
> > +    if ( !verified )
> > +    {
> > +        PrintStr(L"Kernel was not verified\n");
> > +
> > +        if ( efi_secure_boot )
> > +            blexit(L"Failed to verify kernel");
>
> Better be more explicit why it's fatal, like "Refusing to boot
> unverified kernel with UEFI SecureBoot enabled".
>
> > +    }
> > +}
> > +
> >  static void __init efi_tables(void)
> >  {
> >      unsigned int i;
> > @@ -1334,13 +1383,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
> >                                        EFI_SYSTEM_TABLE *SystemTable)
> >  {
> >      static EFI_GUID __initdata loaded_image_guid =
> LOADED_IMAGE_PROTOCOL;
> > -    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
> >      EFI_LOADED_IMAGE *loaded_image;
> >      EFI_STATUS status;
> >      unsigned int i;
> >      CHAR16 *file_name, *cfg_file_name = NULL, *options = NULL;
> >      UINTN gop_mode = ~0;
> > -    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> >      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
> >      union string section = { NULL }, name;
> >      bool base_video = false;
> > @@ -1591,12 +1638,8 @@ 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 )
> > +        efi_verify_kernel(ImageHandle);
> >
> >      efi_arch_edd();
> >
> > --
> > 2.47.3
> >
>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab
>

[-- Attachment #2: Type: text/html, Size: 10414 bytes --]