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

[Alejandro Vallejo <alejandro.garciavallejo@amd.com>]

On Thu Sep 11, 2025 at 10:24 AM CEST, Gerald Elder-Vass wrote:
> Commit 59a1d6d3ea1e introduced Shim's LoadImage protocol and unloads the
> image after loading it (for verification purposes) regardless of the
> returned status. The protocol API implies this is the correct behaviour
> but we should add a check to protect against the unlikely case this
> frees any memory in use.

Any what memory in use? The function loads an image, it's not a consumer.

It can't free anything because it doesn't know where it came from. It might've
been embedded in your own binary, and it can't compromise its integrity like
that.

>
> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
> ---
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Julien Grall <julien@xen.org>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/common/efi/boot.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 69fc022c18ab..ca162db0d8d3 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1062,7 +1062,7 @@ 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_HANDLE loaded_kernel = NULL;

This isn't required if unloading checks for the success case or the only error case
that has a successful load. See below.

Furthermore, you don't really know if the callee clobbered it.

>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_STATUS status;
>      bool verified = false;
> @@ -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 )

Not sure this is what you want. The image needs unloading only when there's no
error OR the error is EFI_SECURITY_VIOLATION. See section 7.4.1:

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

... and shim being a drop-in replacement, it's meant to be spec-compliant.

Trying to unload based on the assumption that loaded_image will remain undisturbed is
an assumption waiting to be broken.

IMO, this wants to be instead:

  if ( !EFI_ERROR(status) || (status == EFI_SECURITY_VIOLATION) )
      // unload

Cheers,
Alejandro