diff mbox series

efi_loader: esrt: Remove EFI_CALL invocation in efi_esrt_register

Message ID 20210410120927.10210-1-sughosh.ganu@linaro.org
State New
Headers show
Series efi_loader: esrt: Remove EFI_CALL invocation in efi_esrt_register | expand

Commit Message

Sughosh Ganu April 10, 2021, 12:09 p.m. UTC 
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.ganu@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(-)

-- 
2.17.1

Comments

Heinrich Schuchardt April 10, 2021, 12:53 p.m. UTC | #1 
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.ganu@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().

>   	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().

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

Best regards

Heinrich

>   	if (ret != EFI_SUCCESS) {

>   		EFI_PRINT("ESRT failed to register FMP callback\n");

>   		return ret;

>
Sughosh Ganu April 10, 2021, 2:52 p.m. UTC | #2 
hi Heinrich,

On Sat, 10 Apr 2021 at 18:24, Heinrich Schuchardt <xypron.glpk@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.ganu@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
diff mbox series

Patch

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);
 	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);
 	if (ret != EFI_SUCCESS) {
 		EFI_PRINT("ESRT failed to register FMP callback\n");
 		return ret;