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

[Masahisa Kojima]

On Wed, 17 Jan 2024 at 20:52, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Kojima-san
>
> On Wed, 17 Jan 2024 at 11:46, Masahisa Kojima
> <masahisa.kojima at linaro.org> 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.
> >
> > This commit also removes the container_of() calls, and
> > adds the TODO comment of missing efi_close_protocol() call
> > for the parent EFI_BLOCK_IO_PROTOCOL.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> >  lib/efi_loader/efi_disk.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index 013842f077..ce46c1092a 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -707,35 +707,46 @@ 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))
> >                 return 0;
> >
> > +       if (!handle)
> > +               return 0;
> > +
>
> Is there a chance dev_tag_get_ptr() will return a NULL handle?

There is no chance unless we actually set a NULL.
Checking the return value of dev_tag_get_ptr above is enough, I will
remove this.

>
> >         id = device_get_uclass_id(dev);
> >         switch (id) {
> >         case UCLASS_BLK:
> >                 desc = dev_get_uclass_plat(dev);
> >                 if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
> > -                       diskobj = container_of(handle, struct efi_disk_obj,
> > -                                              header);
> > +                       diskobj = (struct efi_disk_obj *)handle;
> >                 break;
> >         case UCLASS_PARTITION:
> > -               diskobj = container_of(handle, struct efi_disk_obj, header);
> > +               diskobj = (struct efi_disk_obj *)handle;
> > +
> > +               /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */
> > +
> >                 break;
> >         default:
> >                 return 0;
> >         }
> >
> > +       if (diskobj) {
> > +               dp = diskobj->dp;
> > +               volume = diskobj->volume;
> > +       }
> > +
> >         ret = efi_delete_handle(handle);
>
> Hmm, in which context are the dp and volume pointers allocated? The
> handle is now freed, can we reliably access dp and volume ptrs?

dp and volume are allocated in efi_disk_add_dev() when the block
device is probed.
efi_delete_handle() will remove all protocols from the handle,
efi_uninstall_protocol()
checks whether protocol_interface is actually installed. So dp and
volume should be
freed after efi_delete_handle().

Thanks,
Masahisa Kojima

>
> >         /* 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;
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias