[U-Boot] [RFC] efi_loader: memory leak in bootefi

[Alexander Graf]

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