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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?  Yeah, can fix that up on commit.

~Andrew