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

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

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@xxxxxxxxx>
> >>
> >> 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@xxxxxxxxx>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Reviewed-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 
> 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: signature.asc
Description: PGP signature