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

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

[Andrew Cooper]

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.

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
-- 
2.39.5

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

[Marek Marczykowski-Górecki]

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

On Tue, Oct 14, 2025 at 02:06:48PM +0100, 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>

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

> ---
> 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.
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andrew Cooper]

On 14/10/2025 2:29 pm, Marek Marczykowski-Górecki wrote:
> On Tue, Oct 14, 2025 at 02:06:48PM +0100, 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>
> Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Thanks.

> with one comment below (I'm okay with the patch either way)
>
>>      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?

So to be clear, you're asking for:

loaded_kernel && (!EFI_ERROR(status) || (status == EFI_SECURITY_VIOLATION))

here?  Yeah, can fix that up on commit.

~Andrew

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

[Marek Marczykowski-Górecki]

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

On Tue, Oct 14, 2025 at 04:57:15PM +0100, Andrew Cooper wrote:
> On 14/10/2025 2:29 pm, Marek Marczykowski-Górecki wrote:
> > On Tue, Oct 14, 2025 at 02:06:48PM +0100, 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>
> > Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Thanks.
> 
> > with one comment below (I'm okay with the patch either way)
> >
> >>      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?
> 
> So to be clear, you're asking for:
> 
> loaded_kernel && (!EFI_ERROR(status) || (status == EFI_SECURITY_VIOLATION))
> 
> here?  

Yes.

> Yeah, can fix that up on commit.
> 
> ~Andrew

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Oleksii Kurochko]

[-- 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 --]

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

[Yann Sionneau]

On 10/14/25 15:09, 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>

Reviewed-by: Yann Sionneau <yann.sionneau@vates.tech>


--
Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech