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

[Ilias Apalodimas]

On Thu, 24 Aug 2023 at 17:02, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.

Ok, I'll change that on v2

>
> >
> > [...]
> >
> >>>
> >>> -     /* 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.

Ok, I'll just adjust my explanation and add it as a comment in v2

Thanks
/Ilias
>
> 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
>