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

[Masahisa Kojima]

Hi Heinrich,

On Tue, 26 Dec 2023 at 10:43, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> Am 26. Dezember 2023 02:13:08 MEZ schrieb Masahisa Kojima <masahisa.kojima at linaro.org>:
> >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.
>
> Given an EFI application like iPXE that provides a handle with the block IO protocol implemented by the application: Why should U-Boot remove the block IO protocol or the device path protocol when the application calls DisconnectController()?

Yes, but the current implementation does the same.
So in the case of UCLASS_EFI_LOADER, we need to immediately return without
calling efi_delete_handle(), is my understanding correct?
I checked lib/efi_selftest/efi_selftest_block_device.c implementation,
the teadown() process uninstalls the all protocol_interface then
the EFI handle goes away if no protocol is left.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> >>
> >> 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;
> >>