[PATCH] efi_loader: delete handle from events when a protocol is uninstalled

[Heinrich Schuchardt]

On 24.08.23 15:06, Ilias Apalodimas wrote:
> Hi Henrich
>
>
>>>
>>> +/**
>>> + * efi_purge_handle() - Clean the deleted handle from the various lists
>>> + * @handle: handle to remove
>>> + *
>>> + * Return: status code
>>> + */
>>> +static efi_status_t efi_purge_handle(efi_handle_t handle)
>>> +{
>>> +     struct efi_register_notify_event *item, *next;
>>> +
>>> +     if (!list_empty(&handle->protocols))
>>> +             return EFI_ACCESS_DENIED;
>>> +     /* The handle is about to be freed. Remove it from events */
>>> +     list_for_each_entry_safe(item, next, &efi_register_notify_events, link) {
>>
>> You are not deleting from efi_register_notify_events.
>
> I am not sure I understand this.  I am not trying to delete the event
> here, but the handle on which the notified protocol was installed.
>
>>   Why do you use the safe version of the iterator?
>
> No particular reason, I just tend to prefer it over the unsafe one.
> The latter is perfectly fine here.

The save version is only needed if you delete the current item or insert
in the current position.

>
> [...]
>
>>>
>>> -     /* If the last protocol has been removed, delete the handle. */
>>> -     if (list_empty(&handle->protocols)) {
>>
>>>        size_t i = 0;
>>> @@ -2795,25 +2827,25 @@ efi_uninstall_multiple_protocol_interfaces_int(efi_handle_t handle,
>>>                return EFI_INVALID_PARAMETER;
>>>
>>>        efi_va_copy(argptr_copy, argptr);
>>> +     protocol = efi_va_arg(argptr, efi_guid_t*);
>>>        for (;;) {
>>> -             protocol = efi_va_arg(argptr, efi_guid_t*);
>>> +             bool preserve = true;
>>> +
>>>                if (!protocol)
>>>                        break;
>>>                protocol_interface = efi_va_arg(argptr, void*);
>>> +             next_protocol = efi_va_arg(argptr, efi_guid_t*);
>>> +             if (!next_protocol)
>>> +                     preserve = false;
>>
>> Is your idea that if 5 protocols are to be removed but only the first 3
>> are installed your want to reinstall the first 3?
>
> Not only that. Image you install 2 protocols and then someone tries to
> uninstall 3. If the invalid guid/protocol is at the beginning or the
> middle of the list you'll be fine.  The current code will reinstall
> the previously removed protocols.
> However, if the invalid protocol is last, you will have removed 2
> valid protocols and the handle will be empty.  In that case, we will
> delete the handle before trying to reinstall the uninstalled
> protocols.

This is just what I meant.

Best regards

Heirnich

>
>>
>> Can we have some code comment describing what the function should do? In
>> 5 years nobody will remember why the code was implemented this way.
>>
>
> Of course
>
> [...]
>
> Cheers
> /Ilias