bug - bootflow: grub efi crashes when bootflow selected explicitly

[Heinrich Schuchardt]

On 11/16/23 02:39, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 15 Nov 2023 at 15:47, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>>
>>
>> Am 15. November 2023 23:15:46 MEZ schrieb Simon Glass <sjg at chromium.org>:
>>> Hi Shantur,
>>>
>>> On Wed, 15 Nov 2023 at 15:13, Shantur Rathore <i at shantur.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> I have figured out the cause of the crash.
>>>> It happens here -
>>>> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
>>>> while doing - free(bflow->buf)
>>>>
>>>> As I understand it,
>>>> - Just before starting kernel EFI binary calls usb-uclass->usb_stop()
>>>> - This starts removing all devices in my case ( 6x usb_hub, 2x
>>>> partition, 1x blk, 2x bootdev, 1x usb_maas_storage)
>>>> - While removing bootdev, it ends up calling bootdev-uclass ->
>>>> bootdev_pre_unbind -> bootdev_clear_bootflows which calls
>>>> bootflow->bootflow_remove and bootflow_free
>>>>
>>>> I don't know why this buf cannot be freed, is it because the EFI
>>>> binary in the buffer is still being used ( efi/boot/bootaa64.efi ) ?
>>
>> EFI binaries should never be started from memory allocated with malloc. efi_load_image() should be invoked which allocates EFI memory via efi_allocate_pages(). The handle created has to be passed to efi_start_image().
>
> This is a mess at present, as you are probably aware. EFI has hack to
> look at loaded images and try to patch up its tables.

The UEFI implementation has to follow the specification.

>
> With bootstd we will be able to do this properly.
>
> But note that memory allocation in U-Boot is done via malloc() and
> lmb. We don't call efi_allocate_pages() except in the EFI code.

lmb as used today does not keep track of allocations. It creates a
memory map on the fly which is discarded after usage.

It would be great to unify lmb and the EFI page based allocation. Next
we need to properly allocate memory for loading files and require
freeing the memory before loading a second file into the same location.

The memory available in malloc() is much too small (default 4 MiB) to
handle large files like kernels. malloc() does not keep track of
different memory types that we need in the EFI case.

Best regards

Heinrich

>
>>
>>>> But commenting this line out fixes the crash and linux boots happily.
>>>>
>>>> Fixing this properly is above my pay grade as of now.
>>>
>>> Great, thank you! I will send a patch.
>>
>> Free() typically crashes in U-Boot when freeing the same memory twice.
>
> Indeed. In this case it was not even allocated.
>
> Regards,
> Simon