[PATCH 3/6] efi: Allow monitoring of page allocations

[Heinrich Schuchardt]

On 25.07.24 15:56, Simon Glass wrote:
> Some confusion has set in over which memory-allocation method to use in
> EFI-related code, particularly for the pool allocator. Most of the time,
> malloc() is used, which is correct.
>
> However in some cases the page allocator is used. This means that some
> EFI information is sitting 'in space', outside of the malloc() and not
> allocated by lmb. This can cause problems if an image happens to be
> loaded at the same address.

We already agreed to resolve this problem by integrating LMB and UEFI
memory allocation.

Please, contribute to reviewing
[RFC PATCH v2 00/48] Make U-Boot memory reservations coherent
https://lists.denx.de/pipermail/u-boot/2024-July/557962.html

>
>  From what I can tell, there is currently no checking of this, but it
> seems to be a real bug.
>
> Add a way to control whether allocations are permitted, to help debug
> these sorts of issues.

Calling AllocatePages() and AllocatedPool() cannot be forbidden as we
want to comply to the UEFI specification.

Best regards

Heinrich

>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>   include/efi_loader.h         | 24 +++++++++++++++++++
>   lib/efi_loader/efi_bootbin.c |  2 ++
>   lib/efi_loader/efi_memory.c  | 46 ++++++++++++++++++++++++++++++++++++
>   lib/efi_loader/efi_setup.c   |  7 ++++++
>   4 files changed, 79 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index f2e5063a970..187078adf55 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -797,6 +797,30 @@ int efi_disk_probe(void *ctx, struct event *event);
>   int efi_disk_remove(void *ctx, struct event *event);
>   /* Called by board init to initialize the EFI memory map */
>   int efi_memory_init(void);
> +
> +/**
> + * enum efi_alloc_action - action to take when EFI does a page allocation
> + *
> + * @EFIAA_FAIL: Fail the allocation and print an error
> + * @EFIAA_WARN: Allow the allocation but print a warning
> + * @EFIAA_ALLOW: Allow the allocation with no message
> + */
> +enum efi_alloc_action {
> +	EFIAA_FAIL,
> +	EFIAA_WARN,
> +	EFIAA_ALLOW,
> +};
> +
> +/**
> + * efi_set_alloc() - Set behaviour on page allocation
> + *
> + * This is useful for debugging use of the page allocator when malloc() should
> + * be used instead
> + *
> + * @allow: true to allow EFI to allocate pages, false to fail the allocation
> + */
> +void efi_set_alloc(enum efi_alloc_action action);
> +
>   /* Adds new or overrides configuration table entry to the system table */
>   efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table);
>   /* Sets up a loaded image */
> diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> index a87006b3c0e..07c8fca68cc 100644
> --- a/lib/efi_loader/efi_bootbin.c
> +++ b/lib/efi_loader/efi_bootbin.c
> @@ -201,6 +201,8 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
>   {
>   	efi_status_t ret;
>
> +	efi_set_alloc(EFIAA_ALLOW);
> +
>   	/* Initialize EFI drivers */
>   	ret = efi_init_obj_list();
>   	if (ret != EFI_SUCCESS) {
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 12cf23fa3fa..087f4c88cdf 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -24,6 +24,43 @@ DECLARE_GLOBAL_DATA_PTR;
>   /* Magic number identifying memory allocated from pool */
>   #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2
>
> +/*
> + * This is true if EFI is permitted to allocate pages in memory. When false,
> + * such allocations will fail. This is useful for debugging page allocations
> + * which should in fact use malloc().
> + *
> + * This defaults to EFIAA_FAIL until set.
> + */
> +static enum efi_alloc_action alloc_action;
> +
> +void efi_set_alloc(enum efi_alloc_action action)
> +{
> +	alloc_action = action;
> +}
> +
> +/**
> + * efi_bad_alloc() - Indicate that an allocation is not allowed
> + *
> + * Set a breakpoint on this function to locate the bad allocation in the call
> + * stack
> + */
> +static void efi_bad_alloc(void)
> +{
> +	log_err("EFI: alloc not allowed\n");
> +}
> +
> +static bool check_allowed(void)
> +{
> +	if (alloc_action == EFIAA_FAIL) {
> +		efi_bad_alloc();
> +		return false;
> +	} else if (alloc_action == EFIAA_WARN) {
> +		log_warning("EFI: suspicious alloc detected\n");
> +	}
> +
> +	return true;
> +}
> +
>   efi_uintn_t efi_memory_map_key;
>
>   struct efi_mem_list {
> @@ -501,6 +538,9 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>   	efi_status_t ret;
>   	uint64_t addr;
>
> +	if (!check_allowed())
> +		return EFI_UNSUPPORTED;
> +
>   	/* Check import parameters */
>   	if (memory_type >= EFI_PERSISTENT_MEMORY_TYPE &&
>   	    memory_type <= 0x6FFFFFFF)
> @@ -649,6 +689,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>   	u64 num_pages = efi_size_in_pages(size +
>   					  sizeof(struct efi_pool_allocation));
>
> +	if (!check_allowed())
> +		return EFI_UNSUPPORTED;
> +
>   	if (!buffer)
>   		return EFI_INVALID_PARAMETER;
>
> @@ -952,6 +995,9 @@ int efi_memory_init(void)
>   	/* Request a 32bit 64MB bounce buffer region */
>   	uint64_t efi_bounce_buffer_addr = 0xffffffff;
>
> +	/* this is the earliest page allocation, so allow it */
> +	efi_set_alloc(EFIAA_ALLOW);
> +
>   	if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA,
>   			       (64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
>   			       &efi_bounce_buffer_addr) != EFI_SUCCESS)
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index a610e032d2f..27e24c2f223 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -193,6 +193,13 @@ int efi_init_early(void)
>   	/* Allow unaligned memory access */
>   	allow_unaligned();
>
> +	/*
> +	 * For now, allow EFI to allocate memory outside the malloc() region.
> +	 * Once these bugs are fixed, this can be changed to EFIAA_FAIL, with
> +	 * the allocations being allowed only when the EFI system is booting.
> +	 */
> +	efi_set_alloc(EFIAA_ALLOW);
> +
>   	/* Initialize root node */
>   	ret = efi_root_node_register();
>   	if (ret != EFI_SUCCESS)