Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()

On 8/29/19 5:36 AM, Bin Meng wrote:

+Heinrich,

On Wed, Aug 28, 2019 at 2:35 AM Park, Aiden aiden.park@intel.com wrote:

...

This issue can be seen on 32bit operation when one of E820_RAM type entries is greater than 4GB memory space.

The efi_alloc() finds a free memory in the conventional memory which is greater than 4GB. But, it does type cast to 32bit address space and eventually returns invalid address.

Signed-off-by: Aiden Park aiden.park@intel.com

arch/x86/lib/e820.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/lib/e820.c b/arch/x86/lib/e820.c index d6ae2c4e9d..3e93931231 100644 --- a/arch/x86/lib/e820.c +++ b/arch/x86/lib/e820.c @@ -41,11 +41,15 @@ void efi_add_known_memory(void) { struct e820_entry e820[E820MAX]; unsigned int i, num;

•   u64 start, pages;
  

•   u64 start, pages, ram_top;
     int type;
  
     num = install_e820_map(ARRAY_SIZE(e820), e820);
  
  

•   ram_top = (u64)gd->ram_top & ~EFI_PAGE_MASK;
  

•   if (!ram_top)
  

So for the logic here to work, gd->ram_top is already zero in 32-bit, right? I was wondering how U-Boot could boot on such target?

If in 32bit mode RAM addresses up to 2^32-1 are available, gd->ram_top is 0 due to overflow.

...

•           ram_top = 0x100000000ULL;
  

In this special case we correct the value to 4GB.

...

•    for (i = 0; i < num; ++i) {
             start = e820[i].addr;
             pages = ALIGN(e820[i].size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT;
  

@@ -70,6 +74,22 @@ void efi_add_known_memory(void) }

             efi_add_memory_map(start, pages, type, false);

•           if (type == EFI_CONVENTIONAL_MEMORY) {
  

•                   u64 end = start + (pages << EFI_PAGE_SHIFT);
  

•                   /* reserve the memory region greater than ram_top */
  

•                   if (ram_top < start) {
  

•                           efi_add_memory_map(start, pages,
  

•                                              EFI_BOOT_SERVICES_DATA,
  

•                                              true);
  

Heinrich, could you please review the changes here?

These lines are verbatim copies of what we have in lib/efi_loader/efi_memory.c. Function wise this is ok.

But this creates code duplication. Most of what is in this loop and in the loop in lib/efi_loader/efi_memory.c (starting at /* Remove partial pages */) could be put into a separate common function.

Aiden, do you want to give a try?

Best regards

Heinrich

...

•                   } else if (start < ram_top && ram_top < end) {
  

•                           pages = (end - ram_top) >> EFI_PAGE_SHIFT;
  

•                           efi_add_memory_map(ram_top, pages,
  

•                                              EFI_BOOT_SERVICES_DATA,
  

•                                              true);
  

•                   }
  

•           }
     }
  

  } #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */

--

Regards, Bin