Re: [PATCH] Reset grub_mm_add_region_fn after exiting EFI services

[Daniel Kiper]

On Mon, Dec 16, 2024 at 12:26:58PM +0800, Ruihan Li wrote:
> Currently, EFI services can be used after they are exited because we
> allocate memory after exiting EFI services.
>
> An example call stack is:
>
>   grub_multiboot_boot
>     grub_multiboot2_make_mbi
>       grub_efi_finish_boot_services
>         b->exit_boot_services
>     normal_boot
>       grub_relocator32_boot
>         grub_relocator_alloc_chunk_align_safe
>           grub_relocator_alloc_chunk_align
>             grub_malloc
>               grub_memalign
>                 grub_mm_add_region_fn
>                 [= grub_efi_mm_add_regions]
>                   grub_efi_allocate_any_pages
>                     grub_efi_allocate_pages_real
>                       b->allocate_pages
>
> This can lead to confusing errors. After exiting EFI services,
> b->allocate_pages may point to the NULL address, resulting in something
> like:
>
> !!!! X64 Exception Type - 01(#DB - Debug)  CPU Apic ID - 00000000 !!!!
> RIP  - 000000000000201F, CS  - 0000000000000038, RFLAGS - 0000000000200002
> RAX  - 000000007F9EE010, RCX - 0000000000000001, RDX - 0000000000000002
> RBX  - 0000000000000006, RSP - 00000000001CFBEC, RBP - 0000000000000000
> RSI  - 0000000000000000, RDI - 00000000FFFFFFFF
> R8   - 0000000000000006, R9  - 000000007FEDFFB8, R10 - 0000000000000000
> R11  - 0000000000000475, R12 - 0000000000000001, R13 - 0000000000000002
> R14  - 00000000FFFFFFFF, R15 - 000000007E432C08
> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> GS   - 0000000000000030, SS  - 0000000000000030
> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FC01000
> CR4  - 0000000000000668, CR8 - 0000000000000000
> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> GDTR - 000000007F9DE000 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000007F470018 0000000000000FFF,   TR - 0000000000000000
> FXSAVE_STATE - 00000000001CF840
>
> Ideally, we would like to avoid all memory allocations after exiting EFI
> services altogether, but that requires significant code changes. This
> patch suggests a simple workaround that resets grub_mm_add_region_fn
> after exiting EFI services, so:
>
>  - Memory allocations after exiting EFI services have a better chance of
>    success because grub_memalign will try to reclaim the disk cache if
>    it sees a NULL grub_mm_add_region_fn.
>
>  - At worst, it will fail to allocate memory after exiting EFI services,
>    but it will explicitly tell users that it's out of memory, which is
>    still much better than the current situation where it fails in a
>    fairly random way and triggers a CPU exception.
>
> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

However, should not we go further and extend the heap with additional
memory before EBS? 1 MiB?

Daniel