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

[Park, Aiden]

Hi Bin,

> -----Original Message-----
> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> Sent: Friday, August 30, 2019 12:22 AM
> To: Park, Aiden <aiden.park at intel.com>
> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>; Simon Glass
> <sjg at chromium.org>; u-boot at lists.denx.de; Alexander Graf
> <agraf at csgraf.de>
> Subject: Re: [PATCH 1/1] x86: efi_loader: Fix invalid address return from
> efi_alloc()
> 
> Hi Aiden,
> 
> On Fri, Aug 30, 2019 at 5:31 AM Park, Aiden <aiden.park at intel.com> wrote:
> >
> > Hi Heinrich and Bin,
> >
> > > -----Original Message-----
> > > From: Heinrich Schuchardt [mailto:xypron.glpk at gmx.de]
> > > Sent: Wednesday, August 28, 2019 10:16 PM
> > > To: Bin Meng <bmeng.cn at gmail.com>; Park, Aiden
> > > <aiden.park at intel.com>
> > > Cc: Simon Glass <sjg at chromium.org>; u-boot at lists.denx.de; Alexander
> > > Graf <agraf at csgraf.de>
> > > Subject: Re: [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 at 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 at 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?
> > >
> > I have a quick question here. Do we really need to override
> > efi_add_known_memory() in arch/x86/lib/e820.c?
> > I think the original weak function would be okay for x86 arch as well.
> >
> 
> Yep, I see the generic one provided by the EFI loader is using
> gd->bd->bi_dram[i] to populate the EFI memory. The only handles
> EFI_CONVENTIONAL_MEMORY, but for x86, it may have
> EFI_ACPI_RECLAIM_MEMORY and EFI_ACPI_MEMORY_NVS which
> gd->bd->bi_dram[i] does not distinguish normal memory usage and these
> special ones. Hence I think we still need the x86 specific one.
> 
Yeah Right. The regions can be corrupted as those can be reported as conventional memory
even if efi_loader does not use those memory types. Thanks for clarification.

> But as Heinrich mentioned, there might be some room to refactor the codes
> a little bit to share as much common parts as possible.
> 
Let me refactor the common part. Thanks.

> Regards,
> Bin

Best Regards,
Aiden