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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxx>
> ---
> 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();
>