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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> > CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > CC: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > CC: Michal Orzel <michal.orzel@xxxxxxx>
> > CC: Julien Grall <julien@xxxxxxx>
> > CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> >  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: signature.asc
Description: PGP signature