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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

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

On Mon, Sep 01, 2025 at 03:33:54PM +0000, 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.

But IIUC shim lock protocol isn't removed yet, and some people may still
rely on it. Better to try locating new protocol first, but keep using
old one if new is not found. IMO removal of old protocol support should
only follow removal it from shim and maybe even a bit longer, like wait
for expiration of old shim versions (like revocation of old versions via
SBAT, or maybe expiration of old MS 3rd party cert).

> 
> 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>
> ---
>  xen/common/efi/boot.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 453b1ba099cd..67e7220d8fa3 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -36,8 +36,8 @@
>  
>  #define SMBIOS3_TABLE_GUID \
>    { 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    \
> @@ -66,9 +66,12 @@ typedef EFI_STATUS
>      IN const VOID *Buffer,
>      IN UINT32 Size);
>  
> -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;
>  
> @@ -1333,13 +1336,13 @@ 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;
> +    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 +1593,24 @@ 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 )
> +    if ( kernel.ptr && !kernel_verified )
> +    {
>          PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> +    status = efi_bs->LocateProtocol(&((EFI_GUID) SHIM_IMAGE_LOADER_GUID),
> +                                     NULL, (void **)&shim_loader);

Something is wrong with indentation / brackets here - it's all under "if
( kernel.ptr && !kernel_verified )". Rebase fail?


> +    if ( EFI_ERROR(status) )
> +        PrintErrMesg(L"Error LocateProtocol IMAGE_LOADER", status);
> +
> +    if ( kernel.ptr ) {
> +        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);
> +    }
>  
>      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 --]