[PATCH 2/3] efi_loader: avoid pointer access after calling efi_delete_handle

[Masahisa Kojima]

Hi Heinrich,

On Mon, 25 Dec 2023 at 19:05, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 12/25/23 05:43, Masahisa Kojima wrote:
> > efi_delete_handle() calls efi_purge_handle(), then it finally
> > frees the efi handle.
> > Both diskobj and handle variables in efi_disk_remove() have
> > the same pointer, we can not access diskobj->dp after calling
> > efi_delete_handle().
> >
> > This commit saves the struct efi_device_path pointer before
> > calling efi_delete_handle(). This commit also fixes the
> > missing free for volume member in struct efi_disk_obj.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> >   lib/efi_loader/efi_disk.c | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index a2f8b531a3..415d8601ba 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event)
> >       struct udevice *dev = event->data.dm.dev;
> >       efi_handle_t handle;
> >       struct blk_desc *desc;
> > +     struct efi_device_path *dp = NULL;
> >       struct efi_disk_obj *diskobj = NULL;
> > +     struct efi_simple_file_system_protocol *volume = NULL;
> >       efi_status_t ret;
> >
> >       if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
> > @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event)
> >               return 0;
> >       }
> >
> > +     if (diskobj) {
>
>
>    diskobj = container_of(handle, struct efi_disk_obj, header);
>
> can be replaced by
>
>    diskobj = handle
>
> We should check the handle immediately after dev_tag_get_ptr() and
> return 0 if the handle == NULL.

OK, I will try to improve the efi_disk_remove() function.

>
> > +             dp = diskobj->dp;
> > +             volume = diskobj->volume;
> > +     }
> > +
> >       ret = efi_delete_handle(handle);
>
> We must not delete the handle in case of UCLASS_EFI_LOADER.
>
> Instead of calling efi_delete_handle we should uninstall all protocols
> that we have installed. If no protocol is left the handle will go away.
>
> To make the protocols to delete tractable they should be opened with
> BY_DRIVER.

efi_delete_handle() calls efi_remove_all_protocols(), and frees the efi handle
if no protocol is left on the handle.
So I think efi_delete_handle() does all the required processes mentioned above.

>
> When a partition is removed we must close the protocol interfaces opened
> with EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER (cf. efi_disk_add_dev()).

OK, I will fix this.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >       /* Do not delete DM device if there are still EFI drivers attached. */
> >       if (ret != EFI_SUCCESS)
> >               return -1;
> >
> > -     if (diskobj)
> > -             efi_free_pool(diskobj->dp);
> > -
> > +     efi_free_pool(dp);
> > +     free(volume);
> >       dev_tag_del(dev, DM_TAG_EFI);
> >
> >       return 0;
>