Hi Tom,
On Tue, 30 Jul 2024 at 13:49, Tom Rini <tr...@konsulko.com> wrote:
>
> On Tue, Jul 30, 2024 at 01:42:17PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 30 Jul 2024 at 09:24, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Tue, Jul 30, 2024 at 09:18:02AM -0600, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Tue, 30 Jul 2024 at 08:54, Ilias Apalodimas
> > > > <ilias.apalodi...@linaro.org> wrote:
> > > > >
> > > > > On Tue, 30 Jul 2024 at 17:38, Simon Glass <s...@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Tue, 30 Jul 2024 at 02:20, Ilias Apalodimas
> > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > >
> > > > > > > On Mon, 29 Jul 2024 at 18:28, Simon Glass <s...@chromium.org>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Ilias,
> > > > > > > >
> > > > > > > > On Mon, 29 Jul 2024 at 04:02, Ilias Apalodimas
> > > > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon
> > > > > > > > >
> > > > > > > > > On Fri, 26 Jul 2024 at 17:54, Simon Glass <s...@chromium.org>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Ilias,
> > > > > > > > > >
> > > > > > > > > > On Fri, 26 Jul 2024 at 02:31, Ilias Apalodimas
> > > > > > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Simon,
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 26 Jul 2024 at 02:33, Simon Glass
> > > > > > > > > > > <s...@chromium.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Heinrich,
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, 25 Jul 2024 at 09:54, Heinrich Schuchardt
> > > > > > > > > > > > <xypron.g...@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 <s...@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.
> > > > > > > > > >
> > > > > > > > > > As it stands, U-Boot's data is reported as
> > > > > > > > > > EFI_BOOT_SERVICES_CODE
> > > > > > > > > > through EFI. Perhaps the difference between
> > > > > > > > > > EFI_BOOT_SERVICES_CODE and
> > > > > > > > > > EFI_BOOT_SERVICES_DATA isn't that important?
> > > > > > > > > >
> > > > > > > > > > But if we did want to fix that, we could make the malloc
> > > > > > > > > > region
> > > > > > > > > > EFI_BOOT_SERVICES_DATA.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > Here I don't see the difference between EFI's malloc(n) and
> > > > > > > > > > memalign(8, n). I do see a lot of comments like 'use the
> > > > > > > > > > spec', 'read
> > > > > > > > > > the spec'. It seems very clear to me that the bootloader
> > > > > > > > > > can use
> > > > > > > > > > whatever algorithm it likes to provide the allocated memory
> > > > > > > > > > for the
> > > > > > > > > > pool.
> > > > > > > > > >
> > > > > > > > > > What sort of EFI semantics are you referring to?
> > > > > > > > >
> > > > > > > > > the memory type. Your patch removes the call to
> > > > > > > > > efi_allocate_pages
> > > > > > > > > which tracks it.
> > > > > > > >
> > > > > > > > I can put that code back, depending on what we decide below.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 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
> > > > > > > > > >
> > > > > > > > > > Yes I did a quick grep too, but then checked each site. The
> > > > > > > > > > first two
> > > > > > > > > > are used in an EFI app, not U-Boot itself. The ACPI one is
> > > > > > > > > > not an
> > > > > > > > > > allocation.
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > Any comments on this?
> > > > > > > > >
> > > > > > > > > This violates the spec and probably breaks a few tests and
> > > > > > > > > the EFI
> > > > > > > > > boot services API, as you are supposed to be able to define
> > > > > > > > > the memory
> > > > > > > > > type. We might be able to control it from U-Boot, but EFI
> > > > > > > > > apps are
> > > > > > > > > going to end up being buggy and hard to debug -- e.g an app
> > > > > > > > > calls
> > > > > > > > > allocatePool to allocate memory that needs to be preserved at
> > > > > > > > > runtime.
> > > > > > > >
> > > > > > > > To be more specific, I am suggesting:
> > > > > > > > - use malloc() for EFI_BOOT_SERVICES_DATA allocations (with no
> > > > > > > > memory
> > > > > > > > type since it is always that). This should cover all the U-Boot
> > > > > > > > allocations and make sure they are safely within the malloc()
> > > > > > > > pool
> > > > > > > > - use efi_allocate_pages() for other memory types (keeping the
> > > > > > > > memory
> > > > > > > > type in metadata)
> > > > > > > >
> > > > > > >
> > > > > > > Personally, I don't see the point and we deviate from the spec as
> > > > > > > well. Perhaps Heinrich thinks otherwise, but the EFI spec still
> > > > > > > says
> > > > > > > you need to allocate *pages* to grow the pool. What's missing
> > > > > > > from our
> > > > > > > efi_allocate_pool() is the ability to merge allocations of the
> > > > > > > same
> > > > > > > memory type to an existing pool assuming there's space, rather
> > > > > > > than
> > > > > > > requesting a new pool of 4kb.
> > > > > >
> > > > > > "This function allocates pages from EfiConventionalMemory as needed
> > > > > > to
> > > > > > grow the requested pool type". So far as EFI is concerned, the
> > > > > > malloc() region is in EfiConventionalMemory, so I don't see any
> > > > > > deviation.
> > > > > >
> > > > > > Please also see below.
> > > > > >
> > > > > > > > >
> > > > > > > > > You can switch some callsite of efi_allocate_pool to
> > > > > > > > > efi_allocate_pages() internally. Will that fix the behavior?
> > > > > > > >
> > > > > > > > It still allocates memory 'in space' and uses 4KB for each
> > > > > > > > allocation.
> > > > > > > >
> > > > > >
> > > > > > This is the key point that doesn't seem to be coming across. The
> > > > > > current allocator is allocating memory wherever it likes,
> > > > > > potentially
> > > > > > interfering with the kernel_addr_r addresses, etc. as on qemu_arm.
> > > > >
> > > > > It is, but I don't think using malloc is solving it. It's papering
> > > > > over the problem, because if someone in the future launches an EFI app
> > > > > or allocates EFI memory with a different type you are back on the same
> > > > > problem.
> > > >
> > > > It is certainly solving this problem.
> > > >
> > > > Once the app is launched it is OK to overwrite memory...after all it
> > > > has been loaded and is running. The issue is that these little
> > > > allocations can end up anywhere in memory. Did you see the qemu_arm
> > > > note?
> > >
> > > Isn't this another part of why we need the LMB rework? So that
> > > kernel_addr_r, et al, can be marked as reserved.
> >
> > If we mark them as reserved, we won't be able to load a file into that
> > region, so boot scripts will fail.
>
> No? We mark it as being overwriteable but allocated. This is part of the
> LMB rework series.
Yes, I see, that makes sense. I don't know any way to guess the
expected size of the kernel or ramdisk. I see that Apple uses 128MB
for the kernel (plenty) and 1GB for the ramdisk.
>
> > Please take a look at the whole series and let me know if there is
> > anything missing from the descriptions I have given. I have had this
> > problem in the back of my mind for some time...but just a few hours of
> > investigation was enough to determine that it really is broken.
>
> I'm missing something, sorry. Yes, it is known that EFI can make some
> incorrect assumptions about what memory is/isn't available (as other
> implementations give EFI the world to work with), hence the LMB rework
> series to address some of these problems.
My goal here is to tidy up EFI memory allocation so that it doesn't
result in allocating memory in strange places. Avoiding overlaps is
one thing, but the way this is heading, we will get overlap errors
randomly on platforms when someone tries to load something into RAM,
or we won't protect things that need to be protected. It is all a bit
mushy without a proper design.
Does that make sense?
Regards,
Simon
[1]
https://lore.kernel.org/all/caflszthqh01goqntxucve7s+wju-dz7uqutkb4ikxs6ggx6...@mail.gmail.com/T/
