[PATCH v2 1/1] lib: efi_loader: don't delete invalid handles

[Heinrich Schuchardt]

On 9/8/22 08:46, Ilias Apalodimas wrote:
> Hi all,
> 
> On Thu, 8 Sept 2022 at 09:18, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> From: Etienne Carriere <etienne.carriere at linaro.org>
>>
>> From: Etienne Carriere <etienne.carriere at linaro.org>
>>
>> Change efi_delete_handle() to not free EFI handles twice.
>>
>> This change tries to resolved an issue seen since U-Boot v2022.07
>> in which ExitBootService() attempts to release some EFI handles twice.
>>
>> The issue was seen booting a EFI shell that invokes 'connect -r' and
>> then boots a Linux kernel. Execution of connect command makes EFI
>> subsystem to bind a block device for each root block devices EFI handles.
>> However these EFI device handles are already bound to a driver and we
>> can have 2 registered devices relating to the same EFI handler. On
>> ExitBootService(), the loop removing the devices makes these EFI handles
>> to be released twice which corrupts memory.
>>
>> This patch prevents the memory release operation caused by the issue but
>> but does not resolve the underlying problem.
>>
>> Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
>>
>> Add log message.
>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> v2:
>>          add log message
>>          remove superfluous NULL test
>>          tweak commit message
>> ---
>>   lib/efi_loader/efi_boottime.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 4da64b5d29..6f7333638a 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -619,9 +619,14 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle)
>>    */
>>   void efi_delete_handle(efi_handle_t handle)
>>   {
>> -       if (!handle)
>> +       efi_status_t ret;
>> +
>> +       ret = efi_remove_all_protocols(handle);
>> +       if (ret == EFI_INVALID_PARAMETER) {
>> +               log_err("Can't remove invalid handle %p\n", handle);
>>                  return;
>> -       efi_remove_all_protocols(handle);
>> +       }
>> +
>>          list_del(&handle->link);
>>          free(handle);
> 
> Can't we just set the handle NULL here?


If you set handle=NULL in  efi_delete_handle(), this has no effect as 
the caller passes in handle and not *handle.

Best regards

Heinrich