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 <[email protected]> >> >> 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 <[email protected]> >> Signed-off-by: Andrew Cooper <[email protected]> > Reviewed-by: Marek Marczykowski-Górecki <[email protected]>
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? Yeah, can fix that up on commit. ~Andrew
