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

[Heinrich Schuchardt]

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()?

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