Re: [PATCH] efi_loader: esrt: Remove EFI_CALL invocation in efi_esrt_register

[Sughosh Ganu]

hi Heinrich,

On Sat, 10 Apr 2021 at 18:24, Heinrich Schuchardt <xypron.g...@gmx.de>
wrote:

> On 4/10/21 2:09 PM, Sughosh Ganu wrote:
> > The efi_esrt_register function calls efi_create_event and
> > efi_register_protocol_notify functions. These function calls are made
> > through the EFI_CALL macro.
> >
> > For the Arm and RiscV architecture platforms, the EFI_CALL macro,
> > before invoking the corresponding function, modifies the global_data
> > pointer. Before the function calls, the gd is set to app_gd, which is
> > the value used by UEFI app, and after the function call, it is
> > restored back to u-boot's gd.
> >
> > This becomes an issue when the EFI_CALL is used for the two function
> > invocations, since these functions make calls to calloc, which
> > dereferences the gd pointer. With the gd pointer being no longer
> > valid, this results in an abort. Since these functions are using
> > u-boot's api's, they should not be called through the EFI_CALL
> > macro. Fix this issue by calling these functions directly, without the
> > EFI_CALL macro.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org>
> > ---
> > This issue can be seen by executing a 'printenv -e' command on an arm
> > architecture platform. Executing the command results in an abort.
> >
> >   lib/efi_loader/efi_esrt.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_esrt.c b/lib/efi_loader/efi_esrt.c
> > index 947bdb5e95..141baabb01 100644
> > --- a/lib/efi_loader/efi_esrt.c
> > +++ b/lib/efi_loader/efi_esrt.c
> > @@ -490,15 +490,15 @@ efi_status_t efi_esrt_register(void)
> >               return ret;
> >       }
> >
> > -     ret = EFI_CALL(efi_create_event(EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> > -                                     efi_esrt_new_fmp_notify, NULL,
> NULL, &ev));
> > +     ret = efi_create_event(EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
> > +                            efi_esrt_new_fmp_notify, NULL, NULL, &ev);
>
> Dear Sughosh,
>
> thank you for reporting and analyzing the issue.
>
> This change is required. efi_create_event does not start with EFI_ENTRY().
>

Okay.


>
> >       if (ret != EFI_SUCCESS) {
> >               EFI_PRINT("ESRT failed to create event\n");
> >               return ret;
> >       }
> >
> > -     ret =
> EFI_CALL(efi_register_protocol_notify(&efi_guid_firmware_management_protocol,
> > -                                                 ev, &registration));
> > +     ret =
> efi_register_protocol_notify(&efi_guid_firmware_management_protocol,
> > +                                        ev, &registration);
>
> efi_register_protocol_notify() calls EFI_ENTRY() so you can only invoke
> it with EFI_CALL().
>

Okay, I see what you mean. EFI_ENTRY has a call to __efi_entry_check which
again sets the gd to efi_gd. I will drop this hunk in the next version.


> scripts/get_maintainer.pl asks for adding Alex on CC.
>

During one of my previous patchset, I had kept Alex on Cc, but the email
bounced. Will keep him on Cc in my next version.

-sughosh