Re: [PATCH 2/3] efi: Protect against unnecessary image unloading

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

>> @@ -1078,11 +1078,12 @@ static void __init efi_verify_kernel(EFI_HANDLE ImageHandle)
>>              verified = true;
>> 
>>          /*
>> -         * Always unload the image.  We only needed LoadImage() to perform
>> -         * verification anyway, and in the case of a failure there may still
>> -         * be cleanup needing to be performed.
>> +         * If the kernel was loaded, unload it. We only needed LoadImage() to
>> +         * perform verification anyway, and in the case of a failure there may
>> +         * still be cleanup needing to be performed.
>>           */
>> -        shim_loader->UnloadImage(loaded_kernel);
>> +        if ( loaded_kernel )
>> +            shim_loader->UnloadImage(loaded_kernel);
>>      }
>
>To me this looks as odd as the earlier unconditional unloading. How would a
>halfway sane implementation of LoadImage() return an error, but require
>subsequent cleanup (and set what the last function argument points at to
>non-NULL)? Unless explicitly specified otherwise, my expectation would be
>that upon failure loaded_kernel could have any arbitrary value, possibly
>entirely unsuitable to pass to UnloadImage().

This is because LoadImage performs the verification step after the loading.

They are independent operations but the output conflates the two, so we can

receive a successfully loaded image and an EFI_SECURITY_VIOLATION

status code, in this particular case the image will need to be unloaded. The

generalised check for the EFI_IMAGE_HANDLE before unloading (as

opposed to checking this specific status code) protects against any future

changes in the protocol.

I've linked the specification which states that the ImageHandle is created

in this particular case.

https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-loadimage

Gerald Elder-Vass

Senior Software Engineer

XenServer

Cambridge, UK

On Thu, Sep 11, 2025 at 9:34 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 11.09.2025 10:24, Gerald Elder-Vass wrote:
> @@ -1078,11 +1078,12 @@ static void __init efi_verify_kernel(EFI_HANDLE ImageHandle)
>              verified = true;
> 
>          /*
> -         * Always unload the image.  We only needed LoadImage() to perform
> -         * verification anyway, and in the case of a failure there may still
> -         * be cleanup needing to be performed.
> +         * If the kernel was loaded, unload it. We only needed LoadImage() to
> +         * perform verification anyway, and in the case of a failure there may
> +         * still be cleanup needing to be performed.
>           */
> -        shim_loader->UnloadImage(loaded_kernel);
> +        if ( loaded_kernel )
> +            shim_loader->UnloadImage(loaded_kernel);
>      }

To me this looks as odd as the earlier unconditional unloading. How would a
halfway sane implementation of LoadImage() return an error, but require
subsequent cleanup (and set what the last function argument points at to
non-NULL)? Unless explicitly specified otherwise, my expectation would be
that upon failure loaded_kernel could have any arbitrary value, possibly
entirely unsuitable to pass to UnloadImage().

Jan