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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

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

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

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