| Message ID | 20241024104603.1340301-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | lmb: Correctly unmap and free memory on errors | expand |
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@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(). Best regards Heinrich
Hi Heinrich On Mon, 28 Oct 2024 at 08:40, Heinrich Schuchardt <xypron.glpk@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@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
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; }
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@linaro.org> --- lib/efi_loader/efi_memory.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.45.2