[PATCH] [RFC] lib: efi_loader: don't delete invalid handles

[Etienne Carriere]

Hello Heinirch,

On Fri, 9 Sept 2022 at 08:55, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 9/8/22 07:56, Heinrich Schuchardt wrote:
> > On 9/7/22 23:10, Simon Glass wrote:
> >> Hi Etienne,
> >>
> >> On Wed, 7 Sept 2022 at 02:20, Etienne Carriere
> >> <etienne.carriere at linaro.org> wrote:
> >>>
> >>> Changes efi_delete_handle() to not free EFI handles that are not related
> >>> to EFI objects.
> >>>
> >>> This change tries to resolved an issue seen since U-Boot v2022.07
> >>> in which EFI ExitBootService  attempts to release some EFI handles
> >>> twice.
> >>>
> >>> The issue was seen booting a EFI shell that invokes 'connect -r' and
> >>> then boots a Linux kernel. Execution of connect command makes EFI
> >>> subsystem to bind a block device for each root block devices EFI
> >>> handles.
> >>> However these EFI device handles are already bound to a driver and we
> >>> can have 2 registered devices relating to the same EFI handler. On
> >>> ExitBootService, the loop removing the devices makes these EFI handles
> >>> to be released twice which corrupts memory.
> >>>
> >>> This patch prevents the memory release operation caused by the issue but
> >>> I don't think this patch is the right way to addresse the problem. Any
> >>> help will be much appreciated.
> >>>
> >>> Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
> >>> ---
> >>>   lib/efi_loader/efi_boottime.c | 8 +++++++-
> >>>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> +AKASHI Takahiro who has been working on resolving the mismatch
> >> between driver model and the EFI implementation. We should be able to
> >> attach EFI data structures to driver model devices, which may help
> >> with this issue.
> >>
> >> What is the next step, there?
> >>
> >> Regards,
> >> Simon
> >
> > One of the bugs is in efi_disk_delete_raw().
> >
> > The only allowable way to delete a handle is to delete all protocols
> > that are installed on it. But there are some caveats:
> >
> > * Protocols may not be removable because they have been opened by a
> > driver or a child controller.
> > * We should only remove those protocols that we installed.
> >
> > A correct DM-EFI interface implementation would do the following:
> >
> > * When creating a block device create a handle.
> > * Install the EFI_BLOCK_IO_PROTOCOL on it.
> > * Use ConnectController() to install all other protocols on it. Our
> > implementation of the binding protocol then must open the
> > EFI_BLOCK_IO_PROTOCOL with EFI_OPEN_PROTOCOL_BY_DRIVER.
> > * When trying to remove the block device call
> > UninstallProtocolInterface() for the EFI_BLOCK_IO_PROTOCOL. This invokes
> > DisconnectController() for all drivers that have opened the protocol
> > with EFI_OPEN_PROTOCOL_BY_DRIVER. Only if uninstalling the protocol
> > interface succeeds remove the block device.
> >
> > To make this all work we have to change efi_bl_bind(). We have to
> > differentiate here between a handle being passed in from outside
> > (IF_EFI_LOADER) and a handle for a U-Boot device. We should be able to
> > do so using the field dev in efi_object. If it is NULL, the handle is
> > not for a U-Boot device.
> >
> > Further we need to implement the missing unbind function in the
> > efi_block driver.
> >
> > The ConnectController() call has to be in efi_disk_probe().
> > The UninstallProtocolInterface() call has to be in efi_disk_remove().
> >
> > Enough work for the next release cycle.
> >
> > Best regards
> >
> > Heinrich
>
> The connect -r command calls ConnectController() for all devices.
>
> The only instance of the EFI_DRIVER_BINDING_PROTOCOL that we supply is
> for binding to the EFI_BLOCK_IO_PROTOCOL.
>
> In efi_uc_supported() we try to open the the EFI_BLOCK_IO_PROTOCOL with
> EFI_OPEN_PROTOCOL_BY_DRIVER. This will return EFI_ALREADY_STARTED if our
> driver is already bound to the handle.
>
> This part of the logic works fine and you can see when executing
> 'connect -r' twice in the EFI Shell.
>
> EFI: Entry efi_uc_supported(000000001b241c40, 000000001b237b60, <NULL>)
>    EFI: Call: systab.boottime->open_protocol( controller_handle,
> bp->ops->protocol, &interface, this->driver_binding_handle,
> controller_handle, EFI_OPEN_PROTOCOL_BY_DRIVER)
>    EFI: 20 returned by systab.boottime->open_protocol(
> controller_handle, bp->ops->protocol, &interface,
> this->driver_binding_handle, controller_handle, EFI_OPEN_PROTOCOL_BY_DRIVER)
> EFI: Exit: efi_uc_supported: 20
>
> Where handle 000000001b237b60 is
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(2)
>
> But as we we don't use ConnectController() for installing protocols on
> the U-Boot internal devices the driver is not yet bound and EFI_SUCCESS
> is returned.
>
> As a short term solution before reworking the design the following
> should resolve the reported issue in the UEFI shell:
>
> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
> index b01ce89c84..d348960fc9 100644
> --- a/lib/efi_driver/efi_uclass.c
> +++ b/lib/efi_driver/efi_uclass.c
> @@ -71,6 +71,11 @@ static efi_status_t EFIAPI efi_uc_supported(
>          EFI_ENTRY("%p, %p, %ls", this, controller_handle,
>                    efi_dp_str(remaining_device_path));
>
> +       if (controller_handle->dev) {
> +               ret = EFI_ALREADY_STARTED;
> +               goto out;
> +       }
> +
>          ret = EFI_CALL(systab.boottime->open_protocol(
>                          controller_handle, bp->ops->protocol,
>                          &interface, this->driver_binding_handle,

Works perfectly, from the few tests I've been through.
Thanks.

>
> Best regards
>
> Heinrich
>