bug - bootflow: grub efi crashes when bootflow selected explicitly

[Heinrich Schuchardt]

On 11/15/23 23:46, Heinrich Schuchardt 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)

Unfortunately the description of the field bflow->buf is deceptively wrong:

  @buf: Bootflow file contents (allocated)

The EFI bootflow never allocates this buffer but uses the address
$kernel_addr_r without allocation.

We must not call free on an address that we never allocated via malloc().

Doesn't this also explain the error you experienced before writing

[PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows
https://lore.kernel.org/u-boot/CAHc5_t3v23k_Xbws5o-g9iQfoQ7fhpKScf89XDaaAgo+bu8tbQ@mail.gmail.com/T/#m992e20fb25fe0f2f0047e901a76e78628e59da7a

Best regards

Heinrich

>>>
>>> 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().
>
>>> 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.
>
> Best regards
>
> Heinrich
>
>
>
>>
>> Regards,
>> Simon
>>
>>>
>>> Kind regards,
>>> Shantur
>>>
>>> On Wed, Nov 15, 2023 at 3:58 PM Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> Hi Shantur,
>>>>
>>>> On Wed, 15 Nov 2023 at 08:23, Shantur Rathore <i at shantur.com> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>>
>>>>>> Is this the blue port on top of the USB-C connector?
>>>>>>
>>>>> Yes, that's correct.
>>>>> For my drive I needed -
>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20231110141311.512334-1-i@shantur.com/
>>>>>
>>>>>>
>>>>>> Which version of Armbian / download link?
>>>>>>
>>>>> https://redirect.armbian.com/rockpro64/Bookworm_current
>>>>>
>>>>>> Yes it is scanning first, before reading the efi app, etc.
>>>>>>
>>>>>> I have the same hardware so may be able to dig into this.
>>>>>
>>>>> Sorry, I meant to ask if anything specific to USB.
>>>>> I see it loads efi from the network or disk and calls bootefi with the
>>>>> loaded address.
>>>>> I don't know deep boot / efi stuff, just trying to compare how it's
>>>>> loading efi differently than fatload in this case.
>>>>
>>>> There is nothing special about USB in boootstd, so far as I know.
>>>>
>>>> But until we figure out this bug, it is hard to be sure.
>>>>
>>>> Regards,
>>>> Simon