[PATCH 1/3] efi_loader: use pointers in efi_acpi_register()

[Heinrich Schuchardt]

On 5/22/26 09:50, Ilias Apalodimas wrote:
> Hi Heinrich,
> 
> On Thu, 21 May 2026 at 21:50, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> Both efi_add_memory_map() and efi_install_configuration_table() expect
>> pointers and not virtual sandbox addresses.
> 
> efi_add_memory_map() expects a PA afaict. EFI is identify mapped so
> that will make no differece, but we can be a bit explicit on the
> commit message

Virtual sandbox addresses are not virtual addresses in the EFI sense.

They are offsets in a memory chunked retrieved via mmap().

On sandbox_defconfig:

=> echo $loadaddr
0x0

0x0 is definitively not the address of the mmap'ed memory chunk. It is 
the offset inside the mmap'ed memory chunk.

We must not use these values in the EFI memory map, ACPI tables, or 
anything else an application launched by the sandbox accesses.

> 
>>
>> We need to convert the virtual sandbox addresses in efi_acpi_register() to
>> pointers in memory before using them.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>   lib/efi_loader/efi_acpi.c | 42 ++++++++++++++++++++++++---------------
>>   1 file changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
>> index 046986a7518..70cb896ac25 100644
>> --- a/lib/efi_loader/efi_acpi.c
>> +++ b/lib/efi_loader/efi_acpi.c
>> @@ -25,7 +25,7 @@ static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
>>    */
>>   efi_status_t efi_acpi_register(void)
>>   {
>> -       ulong addr, start, end;
>> +       void *addr, *start, *end;
>>          efi_status_t ret;
>>
>>          /*
>> @@ -37,39 +37,49 @@ efi_status_t efi_acpi_register(void)
>>           */
>>          if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) {
>>                  int size;
>> -               void *tab = bloblist_get_blob(BLOBLISTT_ACPI_TABLES, &size);
>>
>> -               if (!tab)
>> +               addr = bloblist_get_blob(BLOBLISTT_ACPI_TABLES, &size);
>> +
>> +               if (!addr)
>>                          return EFI_NOT_FOUND;
>> -               addr = map_to_sysmem(tab);

In the previous code:

`tab` is a pointer which you can use to address memory.
`addr` is an offset to the memory that sandbox has claimed via mmap().

>>
>> -               ret = efi_add_memory_map(addr, size,

So instead of passing a memory address we were passing here an offset. 
This is wrong.

> 
> Why was this wrong? Isn't addr after map_to_sysmem() going to return a
> PA in normal hardware and the whatever sandbox has mapped? I think
> this will break the sandbox case for both cases.

No, map_to_sysmem() returns an offset to an mmap'ed memory chunk.
But in the EFI memory map we need the addresses that can be used as 
pointers.

Best regards

Heinrich

> 
>> +               ret = efi_add_memory_map((uintptr_t)addr, size,
>>                                           EFI_ACPI_RECLAIM_MEMORY);
>>                  if (ret != EFI_SUCCESS)
>>                          return ret;
>>          } else {
>>                  /* Mark space used for tables */
>> -               start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
>> -               end = ALIGN(gd->arch.table_end, EFI_PAGE_MASK);
>> -               ret = efi_add_memory_map(start, end - start,
>> +               start = (void *)ALIGN_DOWN(
>> +                       (uintptr_t)map_sysmem(gd->arch.table_start, 0),
>> +                       (uintptr_t)EFI_PAGE_MASK);
>> +               end = (void *)ALIGN((uintptr_t)map_sysmem(gd->arch.table_end,
>> +                                                         0),
>> +                                   (uintptr_t)EFI_PAGE_MASK);
>> +               ret = efi_add_memory_map((uintptr_t)start,
>> +                                        (uintptr_t)end - (uintptr_t)start,
>>                                           EFI_ACPI_RECLAIM_MEMORY);
>>                  if (ret != EFI_SUCCESS)
>>                          return ret;
>>                  if (gd->arch.table_start_high) {
>> -                       start = ALIGN_DOWN(gd->arch.table_start_high,
>> -                                          EFI_PAGE_MASK);
>> -                       end = ALIGN(gd->arch.table_end_high, EFI_PAGE_MASK);
>> -                       ret = efi_add_memory_map(start, end - start,
>> +                       start = (void *)ALIGN_DOWN(
>> +                               (uintptr_t)map_sysmem(gd->arch.table_start_high,
>> +                                                     0),
>> +                               (uintptr_t)EFI_PAGE_MASK);
>> +                       end = (void *)ALIGN((uintptr_t)map_sysmem(
>> +                                                   gd->arch.table_end_high, 0),
>> +                                           EFI_PAGE_MASK);
>> +                       ret = efi_add_memory_map((uintptr_t)start
> 
> [...]
> 
> Thanks
> /Ilias