| Message ID | 20220907082013.66879-1-etienne.carriere@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] lib: efi_loader: don't delete invalid handles | expand |
Hi Etienne, On Wed, 7 Sept 2022 at 02:20, Etienne Carriere <etienne.carriere@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@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
On Wed, Sep 07, 2022 at 03:10:44PM -0600, Simon Glass wrote: > Hi Etienne, > > On Wed, 7 Sept 2022 at 02:20, Etienne Carriere > <etienne.carriere@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@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. The only driver which supports DRIVER_BINDING_PROTOCOL, hence connect_controller() interface, in the current UEFI implementation is "EFI block driver" (lib/efi_driver/efi_block_device.c). *Ordinary* block devices, other than UCLASS_EFI_LOADER, are set up without *binding* step. Nevertheless, "connect -r" eventually ends up calling "bind" operation of efi_block_device against those devices. I guess that it is the root cause. efi_uc_supported() should have a stricter check. -Takahiro Akashi > What is the next step, there? > > Regards, > Simon
On 9/7/22 23:10, Simon Glass wrote: > Hi Etienne, > > On Wed, 7 Sept 2022 at 02:20, Etienne Carriere > <etienne.carriere@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@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
On 9/7/22 10:20, Etienne Carriere 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@linaro.org> > --- > lib/efi_loader/efi_boottime.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 4da64b5d29..e38990ace2 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -619,9 +619,15 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle) > */ > void efi_delete_handle(efi_handle_t handle) > { > + efi_status_t ret; > + > if (!handle) > return; The patch does not solve the underlying problem but checking the validity of the handle makes sense anyway as we have a lot of callers. We can remove this check as we test the return value of efi_remove_all_protocols(). > - efi_remove_all_protocols(handle); > + > + ret = efi_remove_all_protocols(handle); > + if (ret == EFI_INVALID_PARAMETER) We should write a message here. log_err("Can't remove invalid handle %p\n", handle); Best regards Heinrich > + return; > + > list_del(&handle->link); > free(handle); > }
Hello Heinrich, Thanks a lot for the detailed feedback on how to address root issue. Indeed not an obvious fix. Regards, Etienne On Thu, 8 Sept 2022 at 08:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 9/7/22 10:20, Etienne Carriere 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@linaro.org> > > --- > > lib/efi_loader/efi_boottime.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_boottime.c > b/lib/efi_loader/efi_boottime.c > > index 4da64b5d29..e38990ace2 100644 > > --- a/lib/efi_loader/efi_boottime.c > > +++ b/lib/efi_loader/efi_boottime.c > > @@ -619,9 +619,15 @@ efi_status_t efi_remove_all_protocols(const > efi_handle_t handle) > > */ > > void efi_delete_handle(efi_handle_t handle) > > { > > + efi_status_t ret; > > + > > if (!handle) > > return; > > The patch does not solve the underlying problem but checking the > validity of the handle makes sense anyway as we have a lot of callers. > > We can remove this check as we test the return value of > efi_remove_all_protocols(). > > > - efi_remove_all_protocols(handle); > > + > > + ret = efi_remove_all_protocols(handle); > > + if (ret == EFI_INVALID_PARAMETER) > > We should write a message here. > > log_err("Can't remove invalid handle %p\n", handle); > > Best regards > > Heinrich > > > + return; > > + > > list_del(&handle->link); > > free(handle); > > } > >
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@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@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
Hello Heinirch, On Fri, 9 Sept 2022 at 08:55, Heinrich Schuchardt <xypron.glpk@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@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@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 >
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 4da64b5d29..e38990ace2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -619,9 +619,15 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle) */ void efi_delete_handle(efi_handle_t handle) { + efi_status_t ret; + if (!handle) return; - efi_remove_all_protocols(handle); + + ret = efi_remove_all_protocols(handle); + if (ret == EFI_INVALID_PARAMETER) + return; + list_del(&handle->link); free(handle); }
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@linaro.org> --- lib/efi_loader/efi_boottime.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)