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

[Ilias Apalodimas]

On Thu, 18 Jan 2024 at 02:16, Masahisa Kojima
<masahisa.kojima at linaro.org> wrote:
>
> 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().

Ok that makes sense,

With the check above removed
Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>


>
> 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