Re: [PATCH for-4.21 v2] efi: Protect against unnecessary image unloading

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 14, 2025 at 02:06:48PM +0100, Andrew Cooper wrote:
> From: Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>
> 
> 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.
> 
> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
with one comment below (I'm okay with the patch either way)

> ---
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> Gerald is OoO and time is tight on Xen 4.21, so I've picked the patch up.
> 
> Oleksii: This addresses follow-on feedback for a new feature in Xen 4.21, so
> really does want fixing before the release.  I forgot to put it on the
> tracking list, sorry.
> 
> v2:
>  * Apply feedback as Marek wants it.
> ---
>  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 5b84dbf26e5e..3a78e7571a5e 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;

While this isn't strictly necessary now (assuming correct firmware
implementation), it helps just a bit with buggy firmware (that would
leave loaded_kernel unset in case of EFI_SECURITY_VIOLATION, possibly
leaking some memory). It still assumes UnloadImage() verifies its
parameter in that case (spec suggests it should, but doesn't spell it
out explicitly).

>      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 ( !EFI_ERROR(status) || (status == EFI_SECURITY_VIOLATION) )

So, just in case of double-buggy firmware, check loaded_kernel here too?

> +            shim_loader->UnloadImage(loaded_kernel);
>      }
>  
>      /* Otherwise, fall back to SHIM_LOCK. */
> 
> base-commit: 53859596c0d34dbca776ec1e47bac8dd90552530
> -- 
> 2.39.5
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature