[PATCH] lmb: Correctly unmap and free memory on errors

[Ilias Apalodimas]

Hi Heinrich

On Mon, 28 Oct 2024 at 08:40, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10/24/24 12:46, Ilias Apalodimas wrote:
> > We never free and unmap the memory on errors and we never unmap it when
> > freeing it. This won't cause any problems on actual hardware but it will
> > on sandbox
> >
> > Fixes: commit 22f2c9ed9f53 ("efi: memory: use the lmb API's for allocating and freeing memory")
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> >   lib/efi_loader/efi_memory.c | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 9f3f08769977..84be5532a655 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -451,7 +451,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> >                               enum efi_memory_type memory_type,
> >                               efi_uintn_t pages, uint64_t *memory)
> >   {
> > -     u64 len;
> > +     u64 efi_addr, len;
> >       uint flags;
> >       efi_status_t ret;
> >       phys_addr_t addr;
> > @@ -499,14 +499,17 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> >               return EFI_INVALID_PARAMETER;
> >       }
> >
> > -     addr = (u64)(uintptr_t)map_sysmem(addr, 0);
> > +     efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0);
> >       /* Reserve that map in our memory maps */
> > -     ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
> > -     if (ret != EFI_SUCCESS)
> > +     ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true);
> > +     if (ret != EFI_SUCCESS) {
> >               /* Map would overlap, bail out */
> > +             lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
> > +             unmap_sysmem((void *)(uintptr_t)efi_addr);
> >               return  EFI_OUT_OF_RESOURCES;
> > +     }
> >
> > -     *memory = addr;
> > +     *memory = efi_addr;
> >
> >       return EFI_SUCCESS;
> >   }
> > @@ -548,6 +551,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
> >       if (ret != EFI_SUCCESS)
> >               return EFI_NOT_FOUND;
> >
> > +     unmap_sysmem((void *)(uintptr_t)memory);
> > +
> >       return ret;
> >   }
> >
> > --
> > 2.45.2
> >
>
> AllocatePages() only provides access to EfiConventionalMemory.
>
> We can safely call map_sysmem() with len = 0 and not use unmap_sysmem().

I am not sure I am following you here. We are calling it with len = 0.
But map/unmap_sysmem keep track of internal ref counters for sandbox
and the special tag case. Why shouldn't we call unmap_sysmem()?

Thanks
/Ilias
>
> Best regards
>
> Heinrich