| Message ID | 20220908061813.30400-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted |
| Commit | 793254893954dc96437b5b5541aab84485bf78b1 |
| Headers | show |
| Series | [v2,1/1] lib: efi_loader: don't delete invalid handles | expand |
Hi all, On Thu, 8 Sept 2022 at 09:18, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > From: Etienne Carriere <etienne.carriere@linaro.org> > > From: Etienne Carriere <etienne.carriere@linaro.org> > > Change efi_delete_handle() to not free EFI handles twice. > > This change tries to resolved an issue seen since U-Boot v2022.07 > in which 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 > but does not resolve the underlying problem. > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> > > Add log message. > Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > v2: > add log message > remove superfluous NULL test > tweak commit message > --- > lib/efi_loader/efi_boottime.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 4da64b5d29..6f7333638a 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -619,9 +619,14 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle) > */ > void efi_delete_handle(efi_handle_t handle) > { > - if (!handle) > + efi_status_t ret; > + > + ret = efi_remove_all_protocols(handle); > + if (ret == EFI_INVALID_PARAMETER) { > + log_err("Can't remove invalid handle %p\n", handle); > return; > - efi_remove_all_protocols(handle); > + } > + > list_del(&handle->link); > free(handle); Can't we just set the handle NULL here? Thanks /Ilias > } > -- > 2.37.2 >
On 9/8/22 08:46, Ilias Apalodimas wrote: > Hi all, > > On Thu, 8 Sept 2022 at 09:18, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> From: Etienne Carriere <etienne.carriere@linaro.org> >> >> From: Etienne Carriere <etienne.carriere@linaro.org> >> >> Change efi_delete_handle() to not free EFI handles twice. >> >> This change tries to resolved an issue seen since U-Boot v2022.07 >> in which 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 >> but does not resolve the underlying problem. >> >> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> >> >> Add log message. >> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> v2: >> add log message >> remove superfluous NULL test >> tweak commit message >> --- >> lib/efi_loader/efi_boottime.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 4da64b5d29..6f7333638a 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -619,9 +619,14 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle) >> */ >> void efi_delete_handle(efi_handle_t handle) >> { >> - if (!handle) >> + efi_status_t ret; >> + >> + ret = efi_remove_all_protocols(handle); >> + if (ret == EFI_INVALID_PARAMETER) { >> + log_err("Can't remove invalid handle %p\n", handle); >> return; >> - efi_remove_all_protocols(handle); >> + } >> + >> list_del(&handle->link); >> free(handle); > > Can't we just set the handle NULL here? If you set handle=NULL in efi_delete_handle(), this has no effect as the caller passes in handle and not *handle. Best regards Heinrich
hi Heinrich On Thu, 8 Sept 2022 at 09:54, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > > > On 9/8/22 08:46, Ilias Apalodimas wrote: > > Hi all, > > > > On Thu, 8 Sept 2022 at 09:18, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> From: Etienne Carriere <etienne.carriere@linaro.org> > >> > >> From: Etienne Carriere <etienne.carriere@linaro.org> > >> > >> Change efi_delete_handle() to not free EFI handles twice. > >> > >> This change tries to resolved an issue seen since U-Boot v2022.07 > >> in which 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 > >> but does not resolve the underlying problem. > >> > >> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> > >> > >> Add log message. > >> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> v2: > >> add log message > >> remove superfluous NULL test > >> tweak commit message > >> --- > >> lib/efi_loader/efi_boottime.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > >> index 4da64b5d29..6f7333638a 100644 > >> --- a/lib/efi_loader/efi_boottime.c > >> +++ b/lib/efi_loader/efi_boottime.c > >> @@ -619,9 +619,14 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle) > >> */ > >> void efi_delete_handle(efi_handle_t handle) > >> { > >> - if (!handle) > >> + efi_status_t ret; > >> + > >> + ret = efi_remove_all_protocols(handle); > >> + if (ret == EFI_INVALID_PARAMETER) { > >> + log_err("Can't remove invalid handle %p\n", handle); > >> return; > >> - efi_remove_all_protocols(handle); > >> + } > >> + > >> list_del(&handle->link); > >> free(handle); > > > > Can't we just set the handle NULL here? > > > If you set handle=NULL in efi_delete_handle(), this has no effect as > the caller passes in handle and not *handle. Yea you are right Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Best regards > > Heinrich
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 4da64b5d29..6f7333638a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -619,9 +619,14 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle) */ void efi_delete_handle(efi_handle_t handle) { - if (!handle) + efi_status_t ret; + + ret = efi_remove_all_protocols(handle); + if (ret == EFI_INVALID_PARAMETER) { + log_err("Can't remove invalid handle %p\n", handle); return; - efi_remove_all_protocols(handle); + } + list_del(&handle->link); free(handle); }