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
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
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
>> @@ -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 >
© 2016 - 2025 Red Hat, Inc.