>> @@ -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 <jbeul...@suse.com> 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 >
