[PATCH] efi_loader: efi_allocate_pages: check parameter pages

[Heinrich Schuchardt]

On 2/20/23 09:46, Peng Fan wrote:
>
>
> On 2/20/2023 4:08 AM, Heinrich Schuchardt wrote:
>> On 2/16/23 11:53, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan at nxp.com>
>>>
>>> On i.MX8MM-EVK, when doing UEFI Capsule On Disk, we met such issue,
>>> It will create Boot option for capsule on disk:
>>> Boot0000:
>>> VenHw(E61D73B9-A384-4ACC-AEAB-82E828F3628B)/eMMC(0x2)/eMMC(0x1)/HD(1,GPT,F5CC8412-CD9F-4C9E-A782-0E945461E89E,0x800,0x32000)
>>> But when capsule update finished, the updated image booting will
>>> trigger:
>>> Loading Boot0000 'UEFI Capsule On Disk' failed
>>> EFI boot manager: Cannot load any image
>>> Found EFI removable media binary efi/boot/bootaa64.efi
>>> "Synchronous Abort" handler, esr 0x96000004
>>> elr: 000000004029f40c lr : 00000000402802f0 (reloc)
>>> elr: 00000000bcd8b40c lr : 00000000bcd6c2f0
>>> x0 : 02029ee86154940e x1 : 00000000bcd95458
>>> x2 : 0000000000000010 x3 : 00000000bad31ad0
>>> x4 : 0000000000000000 x5 : 02029ee86154940e
>>> x6 : 0000000007f00000 x7 : 0000000000000007
>>> x8 : 0000000000000009 x9 : 0000000000000008
>>> x10: 0000000000000035 x11: 0000000000000010
>>> x12: 0000000000000022 x13: 0000000000000001
>>> x14: 00000000bacdedf0 x15: 0000000000000021
>>> x16: 00000000bcd304d0 x17: 00000000000041a0
>>> x18: 00000000bacebdb0 x19: 00000000b9c9f040
>>> x20: 00000000bccecb28 x21: 00000000bcd95458
>>> x22: 0000000000000001 x23: 00000000bad1f010
>>> x24: 00000000bcdced70 x25: 0000000000001000
>>> x26: 00000000b9c9e000 x27: 0000000040000000
>>> x28: 0000000000000001 x29: 00000000bacdd030

Which place in the code do elr and lr relate to?

Have a look into u-boot.map to find the function and use objdump to
identify the exact code location in the *.o files.

>>>
>>> If is the pages is 0, the efi_find_free_memory will return the next used
>>> memory, check the parameter pages, and return EFI_INVALID_PARAMETER
>>> if it
>>> is 0.
>>>
>>> Reviewed-by: Ye Li <ye.li at nxp.com>
>>> Reported-by: Vincent Stehlé <vincent.stehle at arm.com>
>>> Signed-off-by: Peng Fan <peng.fan at nxp.com>
>>> ---
>>>   lib/efi_loader/efi_memory.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>> index b7bee98f79c..acca542033d 100644
>>> --- a/lib/efi_loader/efi_memory.c
>>> +++ b/lib/efi_loader/efi_memory.c
>>> @@ -495,6 +495,9 @@ efi_status_t efi_allocate_pages(enum
>>> efi_allocate_type type,
>>>       if (!memory)
>>>           return EFI_INVALID_PARAMETER;
>>>
>>> +    if (!pages)
>>> +        return EFI_INVALID_PARAMETER;
>>> +
>>
>> Looking at the UEFI specification this looks wrong. The EFI
>> specification does not forbid calling AllocatePages() with pages == 0.
>> So we should return EFI_SUCCESS.
>>
>> EDK II returns EFI_NOT_FOUND for pages == 0. But this has no basis in
>> the specification.
>>
>> Which function is the caller invoking AllocatePages() with pages = 0.
>> Where is the patch to fix it?
>
>
> The call trace as below:
> do_bootefi->do_efibootmgr->efi_bootmgr_load->try_load_entry
> ->efi_load_image->efi_load_image_from_path->efi_load_image_from_file
> ->efi_allocate_pages->efi_find_free_memory
>
> There is an entry Boot0000:
> VenHw(E61D73B9-A384-4ACC-AEAB-82E828F3628B)/eMMC(0x2)/eMMC(0x1)/HD(1,GPT,F5CC8412-CD9F-4C9E-A782-0E945461E89E,0x800,0x32000)
>
> But the capsule1.bin has been removed during capsule update,
> but the entry is still there.
>
> The efi_file_size(f, &bs); will return bs as 0.
>
> The issue is if len is 0, the efi_find_free_memory will return
> memory that are already used by others.

The bug is in the caller not in AllocatePages:

AllocatePages() sets a pointer to a memory range of size 0.

We learnt in math the intersection of an empty set with any other set is
an empty set. A memory range of size 0 does not overlap with any other
memory range.

We have to fix the caller that requests 0 pages and then uses memory
outside of this zero byte area.

How can your problem be reproduced on QEMU?

Best regards

Heinrich

>
> Regards,
> Peng.
>>
>> Best regards
>>
>> Heinrich
>>
>>>       switch (type) {
>>>       case EFI_ALLOCATE_ANY_PAGES:
>>>           /* Any page */
>>