[PATCH 5/6] efi: Use malloc() for the EFI pool
Ilias Apalodimas
ilias.apalodimas at linaro.org
Fri Jul 26 10:31:05 CEST 2024
Hi Simon,
On Fri, 26 Jul 2024 at 02:33, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Heinrich,
>
> On Thu, 25 Jul 2024 at 09:54, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 25.07.24 15:56, Simon Glass wrote:
> > > This API call is intended for allocating small amounts of memory,
> > > similar to malloc(). The current implementation rounds up to whole pages
> > > which can waste large amounts of memory. It also implements its own
> > > malloc()-style header on each block.
> > >
> > > Use U-Boot's built-in malloc() instead, to avoid these problems:
> > >
> > > - it should normally be large enough for pool allocations
> > > - if it isn't we can enforce a minimum size for boards which use
> > > EFI_LOADER
> > > - the existing mechanism may create an unwatned entry in the memory map
> > > - it is used for most EFI allocations already
> > >
> > > One side effect is that this seems to be showing up some bugs in the
> > > EFI code, since the malloc() pool becomes corrupted with some tests.
> > > This has likely crept in due to the very large gaps between allocations
> > > (around 4KB), which provides a lot of leeway when the allocation size is
> > > too small. Work around this by increasing the size for now, until these
> > > (presumed) bugs are located.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > ---
> > >
> > > lib/efi_loader/efi_memory.c | 93 ++++++++-----------------------------
> > > 1 file changed, 19 insertions(+), 74 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > > index 2945f5648c7..fabe9e3a87a 100644
> > > --- a/lib/efi_loader/efi_memory.c
> > > +++ b/lib/efi_loader/efi_memory.c
> > > @@ -80,45 +80,6 @@ static LIST_HEAD(efi_mem);
> > > void *efi_bounce_buffer;
> > > #endif
> > >
> > > -/**
> > > - * struct efi_pool_allocation - memory block allocated from pool
> > > - *
> > > - * @num_pages: number of pages allocated
> > > - * @checksum: checksum
> > > - * @data: allocated pool memory
> > > - *
> > > - * U-Boot services each UEFI AllocatePool() request as a separate
> > > - * (multiple) page allocation. We have to track the number of pages
> > > - * to be able to free the correct amount later.
> > > - *
> > > - * The checksum calculated in function checksum() is used in FreePool() to avoid
> > > - * freeing memory not allocated by AllocatePool() and duplicate freeing.
> > > - *
> > > - * EFI requires 8 byte alignment for pool allocations, so we can
> > > - * prepend each allocation with these header fields.
> > > - */
> > > -struct efi_pool_allocation {
> > > - u64 num_pages;
> > > - u64 checksum;
> > > - char data[] __aligned(ARCH_DMA_MINALIGN);
> > > -};
> > > -
> > > -/**
> > > - * checksum() - calculate checksum for memory allocated from pool
> > > - *
> > > - * @alloc: allocation header
> > > - * Return: checksum, always non-zero
> > > - */
> > > -static u64 checksum(struct efi_pool_allocation *alloc)
> > > -{
> > > - u64 addr = (uintptr_t)alloc;
> > > - u64 ret = (addr >> 32) ^ (addr << 32) ^ alloc->num_pages ^
> > > - EFI_ALLOC_POOL_MAGIC;
> > > - if (!ret)
> > > - ++ret;
> > > - return ret;
> > > -}
> > > -
> > > /**
> > > * efi_mem_cmp() - comparator function for sorting memory map
> > > *
> > > @@ -681,13 +642,10 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align)
> > > * @buffer: allocated memory
> > > * Return: status code
> > > */
> > > -efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
> > > +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> > > + void **buffer)
> > > {
> > > - efi_status_t r;
> > > - u64 addr;
> > > - struct efi_pool_allocation *alloc;
> > > - u64 num_pages = efi_size_in_pages(size +
> > > - sizeof(struct efi_pool_allocation));
> > > + void *ptr;
> > >
> > > if (!check_allowed())
> > > return EFI_UNSUPPORTED;
> > > @@ -700,16 +658,21 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> > > return EFI_SUCCESS;
> > > }
> >
> > Unfortunately this patch does not match the UEFI specification:
> >
> > Two different memory types cannot reside in the same memory page. You
> > have to keep track of the memory type of each allocated memory page and
> > report it in GetMemoryMap().
>
> I actually hadn't thought about that. Is it implied in the spec or
> actually stated somewhere?
I don't remember if it's explicitly stated, but it *really* doesn't
have to be. Although u-boot doesn't currently support this on modern
secure systems you can't have code and data mixed on the same page.
This would prevent you to map pages with proper permissions and take
advantage of CPU security features e.g RW^X.
In any case, the AllocatePool description reads "This function
allocates pages from EfiConventionalMemory as needed to grow the
requested pool type. All allocations are eight-byte aligned". I don't
think using malloc in AllocatePool is appropriate. If we ever want to
do that and allocate from the malloc space, we need to teach malloc
some EFI semantics, but that's a really bad idea.
>
> Anyway, from what I can tell, we mostly use EFI_BOOT_SERVICES_DATA.
> The only exception is EFI_RUNTIME_SERVICES_DATA for the runtime stuff.
That's not entirely correct, we use a lot more than these 2.
EFI_LOADER_DATA, EFI_LOADER_CODE and EFI_ACPI_RECLAIM_MEMORY from a
quick grep
/Ilias
>
> So perhaps we can use malloc() for EFI_BOOT_SERVICES_DATA allocations
> and stick with whole pages otherwise? That will mostly fix the problem
> I am seeing.
>
> >
> > The specification is available https://uefi.org/specifications.
>
> The problem is that it is thousands of pages so it is hard to pick up
> on some of these issues.
>
> >
> > >
> > > - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
> > > - &addr);
> > > - if (r == EFI_SUCCESS) {
> > > - alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
> > > - alloc->num_pages = num_pages;
> > > - alloc->checksum = checksum(alloc);
> > > - *buffer = alloc->data;
> > > - }
> > > + /*
> > > + * Some tests crash on qemu_arm etc. if the correct size is allocated.
> > > + * Adding 0x10 seems to fix test_efi_selftest_device_tree
> > > + * Increasing it to 0x20 seems to fix test_efi_selftest_base except
> > > + * for riscv64 (in CI only). But 0x100 fixes CI too.
> > > + *
> > > + * This workaround can be dropped once these problems are resolved
> > > + */
> > > + ptr = memalign(8, size + 0x100);
> >
> > We were using ARCH_DMA_MINALIGN before this patch.
>
> Where is that in the code? All I see is EFI_PAGE_SHIFT.
>
> >
> > If we are overwriting allocated memory, we must fix that instead of
> > increasing size. Do you have a git tag showing the problem?
>
> Not easy to do, because the 4KB per allocation thing has been there
> from the start. It might be possible to apply my patch to earlier and
> earlier U-Boots to try to bisect it.
>
> Regards,
> Simon
>
>
>
> >
> > Best regards
> >
> > Heinrich
> >
> > > + if (!ptr)
> > > + return EFI_OUT_OF_RESOURCES;
> > > +
> > > + *buffer = ptr;
> > >
> > > - return r;
> > > + return EFI_SUCCESS;
> > > }
> > >
> > > /**
> > > @@ -742,30 +705,12 @@ void *efi_alloc(size_t size)
> > > */
> > > efi_status_t efi_free_pool(void *buffer)
> > > {
> > > - efi_status_t ret;
> > > - struct efi_pool_allocation *alloc;
> > > -
> > > if (!buffer)
> > > return EFI_INVALID_PARAMETER;
> > >
> > > - ret = efi_check_allocated((uintptr_t)buffer, true);
> > > - if (ret != EFI_SUCCESS)
> > > - return ret;
> > > -
> > > - alloc = container_of(buffer, struct efi_pool_allocation, data);
> > > -
> > > - /* Check that this memory was allocated by efi_allocate_pool() */
> > > - if (((uintptr_t)alloc & EFI_PAGE_MASK) ||
> > > - alloc->checksum != checksum(alloc)) {
> > > - printf("%s: illegal free 0x%p\n", __func__, buffer);
> > > - return EFI_INVALID_PARAMETER;
> > > - }
> > > - /* Avoid double free */
> > > - alloc->checksum = 0;
> > > -
> > > - ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
> > > + free(buffer);
> > >
> > > - return ret;
> > > + return EFI_SUCCESS;
> > > }
> > >
> > > /**
> >
More information about the U-Boot
mailing list