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

[Heinrich Schuchardt]

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,

Best regards

Heinrich