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-v...@cloud.com>
> Signed-off-by: Kevin Lampis <kevin.lam...@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();
>
