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

[Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>]

>As already said on an earlier version, the use of !EFI_ERROR() here is a
>behavioral change from ...
>
>> @@ -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 )
>
>... checking for EFI_SUCCESS alone here. There's also nothing in the
>description justifying the change. (See the various EFI_WARN_* that exist.)

You're correct! The EFI_WARN_* responses should all be treated as failures, I'll revert that particular change in the patch series

Gerald Elder-Vass

Senior Software Engineer

XenServer

Cambridge, UK

On Mon, Sep 8, 2025 at 9:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 05.09.2025 14:10, Gerald Elder-Vass wrote:
> @@ -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)) )

As already said on an earlier version, the use of !EFI_ERROR() here is a
behavioral change from ...

> @@ -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 )

... checking for EFI_SUCCESS alone here. There's also nothing in the
description justifying the change. (See the various EFI_WARN_* that exist.)

Jan