[PATCH 0/3] efi: Shim LoadImage improvements

[PATCH 0/3] efi: Shim LoadImage improvements

[Gerald Elder-Vass]

A previous series for supporting Shim's LoadImage protocol had some
outstanding comments after it had been accepted, and internal review
revealed an improvement to be safe when unloading the image.

This patch series includes those changes.

https://gitlab.com/xen-project/people/geraldev/xen/-/pipelines/2032454096

Gerald Elder-Vass (3):
  efi: Fix line length in init_secure_boot_mode
  efi: Protect against unnecessary image unloading
  efi: Limit Shim's Verify success to EFI_SUCCESS

 xen/common/efi/boot.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
2.47.3

[PATCH 1/3] efi: Fix line length in init_secure_boot_mode

[Gerald Elder-Vass]

Commit cb41b4ce14a9 introduced init_secure_boot_mode but one line was
not wrapped appropriately.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b86c83d3348c..69fc022c18ab 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -923,7 +923,8 @@ static void __init init_secure_boot_mode(void)
 
     if ( status == EFI_NOT_FOUND ||
          (status == EFI_SUCCESS &&
-          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) &&
+          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS |
+                   EFI_VARIABLE_RUNTIME_ACCESS) &&
           size == 1 && data == 0) )
         /* Platform does not support Secure Boot or it's disabled. */
         efi_secure_boot = false;
-- 
2.47.3

Re: [PATCH 1/3] efi: Fix line length in init_secure_boot_mode

[Alejandro Vallejo]

On Thu Sep 11, 2025 at 10:24 AM CEST, Gerald Elder-Vass wrote:
> Commit cb41b4ce14a9 introduced init_secure_boot_mode but one line was
> not wrapped appropriately.
>
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index b86c83d3348c..69fc022c18ab 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -923,7 +923,8 @@ static void __init init_secure_boot_mode(void)
>  
>      if ( status == EFI_NOT_FOUND ||
>           (status == EFI_SUCCESS &&
> -          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) &&
> +          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +                   EFI_VARIABLE_RUNTIME_ACCESS) &&
>            size == 1 && data == 0) )
>          /* Platform does not support Secure Boot or it's disabled. */
>          efi_secure_boot = false;

You're not wrong, but it feels a bit excessive having a patch just for this.

Oh, well.

  Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>

Cheers,
Alejandro

Re: [PATCH 1/3] efi: Fix line length in init_secure_boot_mode

[Marek Marczykowski-Górecki]

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

On Thu, Sep 11, 2025 at 08:24:27AM +0000, Gerald Elder-Vass wrote:
> Commit cb41b4ce14a9 introduced init_secure_boot_mode but one line was
> not wrapped appropriately.
> 
> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index b86c83d3348c..69fc022c18ab 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -923,7 +923,8 @@ static void __init init_secure_boot_mode(void)
>  
>      if ( status == EFI_NOT_FOUND ||
>           (status == EFI_SUCCESS &&
> -          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) &&
> +          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +                   EFI_VARIABLE_RUNTIME_ACCESS) &&
>            size == 1 && data == 0) )
>          /* Platform does not support Secure Boot or it's disabled. */
>          efi_secure_boot = false;
> -- 
> 2.47.3
> 

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

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

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

[Gerald Elder-Vass]

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

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

[Jan Beulich]

On 11.09.2025 10:24, Gerald Elder-Vass wrote:
> @@ -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 )
> +            shim_loader->UnloadImage(loaded_kernel);
>      }

To me this looks as odd as the earlier unconditional unloading. How would a
halfway sane implementation of LoadImage() return an error, but require
subsequent cleanup (and set what the last function argument points at to
non-NULL)? Unless explicitly specified otherwise, my expectation would be
that upon failure loaded_kernel could have any arbitrary value, possibly
entirely unsuitable to pass to UnloadImage().

Jan

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

[Gerald Elder-Vass]

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

>> @@ -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 )
>> +            shim_loader->UnloadImage(loaded_kernel);
>>      }
>
>To me this looks as odd as the earlier unconditional unloading. How would a
>halfway sane implementation of LoadImage() return an error, but require
>subsequent cleanup (and set what the last function argument points at to
>non-NULL)? Unless explicitly specified otherwise, my expectation would be
>that upon failure loaded_kernel could have any arbitrary value, possibly
>entirely unsuitable to pass to UnloadImage().

This is because LoadImage performs the verification step after the loading.
They are independent operations but the output conflates the two, so we can
receive a successfully loaded image and an EFI_SECURITY_VIOLATION
status code, in this particular case the image will need to be unloaded. The
generalised check for the EFI_IMAGE_HANDLE before unloading (as
opposed to checking this specific status code) protects against any future
changes in the protocol.

I've linked the specification which states that the ImageHandle is created
in this particular case.
https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#efi-boot-services-loadimage

*Gerald Elder-Vass*
Senior Software Engineer

XenServer
Cambridge, UK

On Thu, Sep 11, 2025 at 9:34 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 11.09.2025 10:24, Gerald Elder-Vass wrote:
> > @@ -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 )
> > +            shim_loader->UnloadImage(loaded_kernel);
> >      }
>
> To me this looks as odd as the earlier unconditional unloading. How would a
> halfway sane implementation of LoadImage() return an error, but require
> subsequent cleanup (and set what the last function argument points at to
> non-NULL)? Unless explicitly specified otherwise, my expectation would be
> that upon failure loaded_kernel could have any arbitrary value, possibly
> entirely unsuitable to pass to UnloadImage().
>
> Jan
>

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

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

[Alejandro Vallejo]

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

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

[Marek Marczykowski-Górecki]

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

On Thu, Sep 11, 2025 at 07:20:33PM +0200, Alejandro Vallejo wrote:
> 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

I agree, this version would be better.

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

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

[PATCH 3/3] efi: Limit Shim's Verify success to EFI_SUCCESS

[Gerald Elder-Vass]

Commit 59a1d6d3ea1e replaced the Verify status check with
!EFI_ERROR(...), this changed the behaviour to consider any warnings
(EFI_WARN_) to be considered a successful verification.

This commit reverts that behaviour change.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ca162db0d8d3..36e1e2cf9d4a 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1090,7 +1090,7 @@ static void __init efi_verify_kernel(EFI_HANDLE ImageHandle)
     if ( !verified &&
          !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
                                            (void **)&shim_lock)) &&
-         !EFI_ERROR(shim_lock->Verify(kernel.ptr, kernel.size)) )
+         shim_lock->Verify(kernel.ptr, kernel.size) == EFI_SUCCESS )
         verified = true;
 
     if ( !verified )
-- 
2.47.3

Re: [PATCH 3/3] efi: Limit Shim's Verify success to EFI_SUCCESS

[Jan Beulich]

On 11.09.2025 10:24, Gerald Elder-Vass wrote:
> Commit 59a1d6d3ea1e replaced the Verify status check with
> !EFI_ERROR(...), this changed the behaviour to consider any warnings
> (EFI_WARN_) to be considered a successful verification.
> 
> This commit reverts that behaviour change.

Reported-by: Jan Beulich <jbeulich@suse.com>
Fixes: ...

> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 3/3] efi: Limit Shim's Verify success to EFI_SUCCESS

[Marek Marczykowski-Górecki]

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

On Thu, Sep 11, 2025 at 10:35:51AM +0200, Jan Beulich wrote:
> On 11.09.2025 10:24, Gerald Elder-Vass wrote:
> > Commit 59a1d6d3ea1e replaced the Verify status check with
> > !EFI_ERROR(...), this changed the behaviour to consider any warnings
> > (EFI_WARN_) to be considered a successful verification.
> > 
> > This commit reverts that behaviour change.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Fixes: ...
> 
> > Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@cloud.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

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

Ping: [PATCH 0/3] efi: Shim LoadImage improvements

[Jan Beulich]

On 11.09.2025 10:24, Gerald Elder-Vass wrote:
> A previous series for supporting Shim's LoadImage protocol had some
> outstanding comments after it had been accepted, and internal review
> revealed an improvement to be safe when unloading the image.
> 
> This patch series includes those changes.
> 
> https://gitlab.com/xen-project/people/geraldev/xen/-/pipelines/2032454096
> 
> Gerald Elder-Vass (3):
>   efi: Fix line length in init_secure_boot_mode
>   efi: Protect against unnecessary image unloading
>   efi: Limit Shim's Verify success to EFI_SUCCESS

Can there please be timely responses to small fixes like these ones?
Especially patch 3 wants to go in rather sooner than later.

Thanks, Jan