[U-Boot] [RFC] efi_loader: memory leak in bootefi
Hello Alex,
in bootefi.c do_bootefi_exec we build the efi_obj_list. This includes allocation of memory for some handlers (e.g. in efi_gop_register).
After returning from the efi appliation we have no clean up code to release these objects.
We do not remove the list elements from efi_obj_list.
Furthermore we rely on a lot of static initializations e.g. for protocols. We know that this data may be changed by the application but we do not care to restore the original state.
So if an application registers protocols and exits without unregistering we will offer invalid function pointers to the next efi application to be started.
I suggest the following:
In structure struct efi_object we add a function pointer to a clean-up function which takes as only argument the efi_object:
struct efi_object { struct list_head link; struct efi_handler protocols[4]; void (*cleanup)(struct efi_object *obj); void *handle; };
A clean up function may look like this: void efi_gop_cleanup(struct efi_object *obj) { free(obj); }
When returning from the EFI application we would work our way from the tail to the head of the object list:
while (list is not empty) { Remove last object from list. Call cleanup function of removed object. }
We should get rid of the static structures loaded_image_info_obj and boot_efi_obj. Let's use register functions with calloc here too.
Would you agree to this design?
Best regards
Heinrich
Hi Heinrich,
On 07/06/2017 05:43 PM, Heinrich Schuchardt wrote:
Hello Alex,
in bootefi.c do_bootefi_exec we build the efi_obj_list. This includes allocation of memory for some handlers (e.g. in efi_gop_register).
After returning from the efi appliation we have no clean up code to release these objects.
We do not remove the list elements from efi_obj_list.
Furthermore we rely on a lot of static initializations e.g. for protocols. We know that this data may be changed by the application but we do not care to restore the original state.
So if an application registers protocols and exits without unregistering we will offer invalid function pointers to the next efi application to be started.
I suggest the following:
In structure struct efi_object we add a function pointer to a clean-up function which takes as only argument the efi_object:
struct efi_object { struct list_head link; struct efi_handler protocols[4]; void (*cleanup)(struct efi_object *obj); void *handle; };
A clean up function may look like this: void efi_gop_cleanup(struct efi_object *obj) { free(obj); }
When returning from the EFI application we would work our way from the tail to the head of the object list:
while (list is not empty) { Remove last object from list. Call cleanup function of removed object. }
We should get rid of the static structures loaded_image_info_obj and boot_efi_obj. Let's use register functions with calloc here too.
Would you agree to this design?
The basic long-term plan was to instead integrate the efi object model with the DM object model and slowly make every DM object potentially an EFI object. That way we would only initialize the objects once on bootup.
The fact that then an application may modify pointers is actually intended by UEFI. A driver may want to intercept what another driver does or route things through itself. It works the same way in edk2 as far as I can tell :).
Alex
On 07/06/2017 05:52 PM, Alexander Graf wrote:
Hi Heinrich,
On 07/06/2017 05:43 PM, Heinrich Schuchardt wrote:
Hello Alex,
in bootefi.c do_bootefi_exec we build the efi_obj_list. This includes allocation of memory for some handlers (e.g. in efi_gop_register).
After returning from the efi appliation we have no clean up code to release these objects.
We do not remove the list elements from efi_obj_list.
Furthermore we rely on a lot of static initializations e.g. for protocols. We know that this data may be changed by the application but we do not care to restore the original state.
So if an application registers protocols and exits without unregistering we will offer invalid function pointers to the next efi application to be started.
I suggest the following:
In structure struct efi_object we add a function pointer to a clean-up function which takes as only argument the efi_object:
struct efi_object { struct list_head link; struct efi_handler protocols[4]; void (*cleanup)(struct efi_object *obj); void *handle; };
A clean up function may look like this: void efi_gop_cleanup(struct efi_object *obj) { free(obj); }
When returning from the EFI application we would work our way from the tail to the head of the object list:
while (list is not empty) { Remove last object from list. Call cleanup function of removed object. }
We should get rid of the static structures loaded_image_info_obj and boot_efi_obj. Let's use register functions with calloc here too.
Would you agree to this design?
The basic long-term plan was to instead integrate the efi object model with the DM object model and slowly make every DM object potentially an EFI object. That way we would only initialize the objects once on bootup.
The fact that then an application may modify pointers is actually intended by UEFI. A driver may want to intercept what another driver does or route things through itself. It works the same way in edk2 as far as I can tell :).
There are two types of EFI binaries: - applications - drivers
In the "Microsoft Portable Executable and Common Object File Format Specification" this is described as field "Windows Subsystem":
/* An Extensible Firmware Interface (EFI) application */ IMAGE_SUBSYSTEM_EFI_APPLICATION = 10 /* An EFI driver with boot services */ IMAGE_SUBSYSTEM_EFI_BOOT_ SERVICE_DRIVER = 11
In my notes above I only related to EFI applications. And you are right in that we may have to treat drivers differently.
My test was running the iPXE application and terminating it from the shell with "exit". When I tried to run it again I got an error EFI_OUT_OF_RESOURCES which I did not get on the first run.
Currently we are using bootefi to run applications like grub. Do we want to use the same command to install drivers?
If yes, bootefi should evaluate the PE32+ header to find out if the binary is a driver or an application binary.
In case of an application returning we should restore the situation before the application was started.
Independent of the discussion above there is a design error that is relevant for any type of binary:
We calloc a new efi object for efi_driver and efi_gop and add it to the efi object list every time that bootefi is called. We never free the objects. So this results in a memory leak.
According to the UEFI specification additional protocols should be installed on the system handle. So I guess the efi_gop and efi_driver objects can be safely freed after use.
Best regards
Heinrich
On 06.07.17 20:23, Heinrich Schuchardt wrote:
On 07/06/2017 05:52 PM, Alexander Graf wrote:
Hi Heinrich,
On 07/06/2017 05:43 PM, Heinrich Schuchardt wrote:
Hello Alex,
in bootefi.c do_bootefi_exec we build the efi_obj_list. This includes allocation of memory for some handlers (e.g. in efi_gop_register).
After returning from the efi appliation we have no clean up code to release these objects.
We do not remove the list elements from efi_obj_list.
Furthermore we rely on a lot of static initializations e.g. for protocols. We know that this data may be changed by the application but we do not care to restore the original state.
So if an application registers protocols and exits without unregistering we will offer invalid function pointers to the next efi application to be started.
I suggest the following:
In structure struct efi_object we add a function pointer to a clean-up function which takes as only argument the efi_object:
struct efi_object { struct list_head link; struct efi_handler protocols[4]; void (*cleanup)(struct efi_object *obj); void *handle; };
A clean up function may look like this: void efi_gop_cleanup(struct efi_object *obj) { free(obj); }
When returning from the EFI application we would work our way from the tail to the head of the object list:
while (list is not empty) { Remove last object from list. Call cleanup function of removed object. }
We should get rid of the static structures loaded_image_info_obj and boot_efi_obj. Let's use register functions with calloc here too.
Would you agree to this design?
The basic long-term plan was to instead integrate the efi object model with the DM object model and slowly make every DM object potentially an EFI object. That way we would only initialize the objects once on bootup.
The fact that then an application may modify pointers is actually intended by UEFI. A driver may want to intercept what another driver does or route things through itself. It works the same way in edk2 as far as I can tell :).
There are two types of EFI binaries:
- applications
- drivers
In the "Microsoft Portable Executable and Common Object File Format Specification" this is described as field "Windows Subsystem":
/* An Extensible Firmware Interface (EFI) application */ IMAGE_SUBSYSTEM_EFI_APPLICATION = 10 /* An EFI driver with boot services */ IMAGE_SUBSYSTEM_EFI_BOOT_ SERVICE_DRIVER = 11
In my notes above I only related to EFI applications. And you are right in that we may have to treat drivers differently.
I don't think edk2 treats them that differently. The object model is persistent across binary invocations. That's the thing we're lacking in U-Boot right now.
My test was running the iPXE application and terminating it from the shell with "exit". When I tried to run it again I got an error EFI_OUT_OF_RESOURCES which I did not get on the first run.
Currently we are using bootefi to run applications like grub. Do we want to use the same command to install drivers?
If we ever want to support drivers (not sure we do yet), then yes.
If yes, bootefi should evaluate the PE32+ header to find out if the binary is a driver or an application binary.
Would it make a difference?
In case of an application returning we should restore the situation before the application was started.
Again, I don't think this is what edk2 does. But let's ask Ard.
Independent of the discussion above there is a design error that is relevant for any type of binary:
We calloc a new efi object for efi_driver and efi_gop and add it to the efi object list every time that bootefi is called. We never free the objects. So this results in a memory leak.
Yes. Recreating the object model in itself is problematic. We really should only create it once on U-Boot instantiation - together with the U-Boot internal device model.
According to the UEFI specification additional protocols should be installed on the system handle. So I guess the efi_gop and efi_driver objects can be safely freed after use.
No, instead they should simply persist before and after bootefi :).
The reason I didn't always create efi objects without bootefi invocation was simply because I didn't want to incur any overhead on people that don't want to run efi binaries. I guess a quick fix would be to just remember that the efi object model is instantiated already and only create objects if it's not initialized yet.
Alex
On 6 July 2017 at 20:24, Alexander Graf agraf@suse.de wrote:
On 06.07.17 20:23, Heinrich Schuchardt wrote:
On 07/06/2017 05:52 PM, Alexander Graf wrote:
Hi Heinrich,
On 07/06/2017 05:43 PM, Heinrich Schuchardt wrote:
Hello Alex,
in bootefi.c do_bootefi_exec we build the efi_obj_list. This includes allocation of memory for some handlers (e.g. in efi_gop_register).
After returning from the efi appliation we have no clean up code to release these objects.
We do not remove the list elements from efi_obj_list.
Furthermore we rely on a lot of static initializations e.g. for protocols. We know that this data may be changed by the application but we do not care to restore the original state.
So if an application registers protocols and exits without unregistering we will offer invalid function pointers to the next efi application to be started.
I suggest the following:
In structure struct efi_object we add a function pointer to a clean-up function which takes as only argument the efi_object:
struct efi_object { struct list_head link; struct efi_handler protocols[4]; void (*cleanup)(struct efi_object *obj); void *handle; };
A clean up function may look like this: void efi_gop_cleanup(struct efi_object *obj) { free(obj); }
When returning from the EFI application we would work our way from the tail to the head of the object list:
while (list is not empty) { Remove last object from list. Call cleanup function of removed object. }
We should get rid of the static structures loaded_image_info_obj and boot_efi_obj. Let's use register functions with calloc here too.
Would you agree to this design?
The basic long-term plan was to instead integrate the efi object model with the DM object model and slowly make every DM object potentially an EFI object. That way we would only initialize the objects once on bootup.
The fact that then an application may modify pointers is actually intended by UEFI. A driver may want to intercept what another driver does or route things through itself. It works the same way in edk2 as far as I can tell :).
There are two types of EFI binaries:
- applications
- drivers
In the "Microsoft Portable Executable and Common Object File Format Specification" this is described as field "Windows Subsystem":
/* An Extensible Firmware Interface (EFI) application */ IMAGE_SUBSYSTEM_EFI_APPLICATION = 10 /* An EFI driver with boot services */ IMAGE_SUBSYSTEM_EFI_BOOT_ SERVICE_DRIVER = 11
In my notes above I only related to EFI applications. And you are right in that we may have to treat drivers differently.
I don't think edk2 treats them that differently. The object model is persistent across binary invocations. That's the thing we're lacking in U-Boot right now.
There are actually three classes of executables: application, driver and runtime driver.
Applications typically only consume protocols, and are otherwise expected to clean up after themselves at exit, i.e., free memory allocations and perform deregistration of data structures that is has registered with other protocols. The memory allocated for the PE/COFF image is freed automatically by the DXE core. (We had a bug for a while in GRUB for arm64 where it would crash at exit due to a timer pointing into the GRUB image not being disarmed)
Drivers primarily produce protocols for use by other drivers, and may consume other protocols if needed for their implementation. Drivers remain in memory after their entry point function exits, and are not expected to free any resources or perform deregistration until their [optional] unload hook is called.
Runtime drivers are like drivers, only their PE/COFF image is loaded into runtime services memory rather than boot services memory.
My test was running the iPXE application and terminating it from the shell with "exit". When I tried to run it again I got an error EFI_OUT_OF_RESOURCES which I did not get on the first run.
Currently we are using bootefi to run applications like grub. Do we want to use the same command to install drivers?
If we ever want to support drivers (not sure we do yet), then yes.
If yes, bootefi should evaluate the PE32+ header to find out if the binary is a driver or an application binary.
Would it make a difference?
It should, but if you never free the memory allocated for (and by) applications, then it doesn't matter I suppose.
In case of an application returning we should restore the situation before the application was started.
Again, I don't think this is what edk2 does. But let's ask Ard.
The only thing the UEFI core is responsible for is freeing the memory used for loading the PE/COFF image in memory. Everything else is the responsibility of the application itself.
Independent of the discussion above there is a design error that is relevant for any type of binary:
We calloc a new efi object for efi_driver and efi_gop and add it to the efi object list every time that bootefi is called. We never free the objects. So this results in a memory leak.
Yes. Recreating the object model in itself is problematic. We really should only create it once on U-Boot instantiation - together with the U-Boot internal device model.
According to the UEFI specification additional protocols should be installed on the system handle. So I guess the efi_gop and efi_driver objects can be safely freed after use.
No, instead they should simply persist before and after bootefi :).
The reason I didn't always create efi objects without bootefi invocation was simply because I didn't want to incur any overhead on people that don't want to run efi binaries. I guess a quick fix would be to just remember that the efi object model is instantiated already and only create objects if it's not initialized yet.
Alex
participants (3)
-
Alexander Graf -
Ard Biesheuvel -
Heinrich Schuchardt