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 <[email protected]> > --- > CC: Marek Marczykowski-Górecki <[email protected]> > CC: "Daniel P. Smith" <[email protected]> > CC: Jan Beulich <[email protected]> > CC: Andrew Cooper <[email protected]> > CC: Anthony PERARD <[email protected]> > CC: Michal Orzel <[email protected]> > CC: Julien Grall <[email protected]> > CC: "Roger Pau Monné" <[email protected]> > CC: Stefano Stabellini <[email protected]> > --- > 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
