1. Patchew
  2. Xen
  3. View series

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

Andrew Cooper posted 1 patch 2 weeks, 2 days ago
Failed in applying to current master (apply log)
xen/common/efi/boot.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
[PATCH for-4.21 v2] efi: Protect against unnecessary image unloading
Posted by Andrew Cooper 2 weeks, 2 days ago 
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
Posted by Yann Sionneau 2 weeks, 1 day ago 
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
Re: [PATCH for-4.21 v2] efi: Protect against unnecessary image unloading
Posted by Oleksii Kurochko 2 weeks, 2 days ago 
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
Re: [PATCH for-4.21 v2] efi: Protect against unnecessary image unloading
Posted by Marek Marczykowski-Górecki 2 weeks, 2 days ago 
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
Re: [PATCH for-4.21 v2] efi: Protect against unnecessary image unloading
Posted by Andrew Cooper 2 weeks, 1 day ago 
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
Posted by Marek Marczykowski-Górecki 2 weeks, 1 day ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.