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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]


On 10/14/25 3:06 PM, Andrew Cooper wrote:
> From: Gerald Elder-Vass<gerald.elder-vass@cloud.com>
>
> 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@cloud.com>
> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
> ---
> CC: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
> CC: Daniel P. Smith<dpsmith@apertussolutions.com>
> CC: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>
> 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.

It seems critical enough as it could lead to undef. behaviour/boot-time crashes/etc.
So we really want to have it in 4.21:
  Release-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

>
> 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;
>       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) )
> +            shim_loader->UnloadImage(loaded_kernel);
>       }
>   
>       /* Otherwise, fall back to SHIM_LOCK. */
>
> base-commit: 53859596c0d34dbca776ec1e47bac8dd90552530

[-- Attachment #2: Type: text/html, Size: 3873 bytes --]