diff mbox series

lmb: Correctly unmap and free memory on errors

Message ID 20241024104603.1340301-1-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series lmb: Correctly unmap and free memory on errors | expand

Commit Message

Ilias Apalodimas Oct. 24, 2024, 10:46 a.m. UTC 
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

Comments

Heinrich Schuchardt Oct. 28, 2024, 6:40 a.m. UTC | #1 
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
Ilias Apalodimas Oct. 29, 2024, 8:46 a.m. UTC | #2 
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
Heinrich Schuchardt Nov. 11, 2024, 10:47 a.m. UTC | #3 
On 10/29/24 09:46, Ilias Apalodimas wrote:
> 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()?

As discussed on IRC the sandbox currently does not do reference counting
for conventional memory. Ilias suggested to leave it here for consistency.

Moving the patch to the EFI patch queue.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>
> Thanks
> /Ilias
>>
>> Best regards
>>
>> Heinrich
diff mbox series

Patch

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;
 }