Re: [PATCH] arm64: efi: Fix KASAN false positive for EFI runtime stack

[Ard Biesheuvel]

On Thu, 3 Jul 2025 at 18:35, Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>
> On Tue, Jun 24, 2025 at 05:55:53AM -0700, Breno Leitao wrote:
> > KASAN reports invalid accesses during arch_stack_walk() for EFI runtime
> > services due to vmalloc tagging[1]. The EFI runtime stack must be allocated
> > with KASAN tags reset to avoid false positives.
> >
> > This patch uses arch_alloc_vmap_stack() instead of __vmalloc_node() for
> > EFI stack allocation, which internally calls kasan_reset_tag()
> >
> > The changes ensure EFI runtime stacks are properly sanitized for KASAN
> > while maintaining functional consistency.
> >
> > Link: https://lore.kernel.org/all/aFVVEgD0236LdrL6@xxxxxxxxx/ [1]
> > Suggested-by: Andrey Konovalov <andreyknvl@xxxxxxxxx>
> > Suggested-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
> > ---
> >  arch/arm64/kernel/efi.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> > index 3857fd7ee8d46..d2af881a48290 100644
> > --- a/arch/arm64/kernel/efi.c
> > +++ b/arch/arm64/kernel/efi.c
> > @@ -15,6 +15,7 @@
> >
> >  #include <asm/efi.h>
> >  #include <asm/stacktrace.h>
> > +#include <asm/vmap_stack.h>
> >
> >  static bool region_is_misaligned(const efi_memory_desc_t *md)
> >  {
> > @@ -214,9 +215,11 @@ static int __init arm64_efi_rt_init(void)
> >       if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >               return 0;
> >
> > -     p = __vmalloc_node(THREAD_SIZE, THREAD_ALIGN, GFP_KERNEL,
> > -                        NUMA_NO_NODE, &&l);
> > -l:   if (!p) {
> > +     if (!IS_ENABLED(CONFIG_VMAP_STACK))
> > +             return -ENOMEM;
>
> Mark Rutland pointed out in a private chat that this should probably
> clear the EFI_RUNTIME_SERVICES flag as well.
>

If VMAP_STACK is a hard requirement, should we make CONFIG_EFI depend
on it for arm64?

> > +
> > +     p = arch_alloc_vmap_stack(THREAD_SIZE, NUMA_NO_NODE);
> > +     if (!p) {
> >               pr_warn("Failed to allocate EFI runtime stack\n");
> >               clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> >               return -ENOMEM;
> >
>
> With that:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>
> (but let's see if Ard has a different opinion on the approach)
>

I think this is fine - the stack just needs to be disjoint from the
ordinary kernel mode task stack so that buggy firmware is less likely
to corrupt it, and so that we can recover from an unexpected
synchronous exception more reliably.

In that sense, the old and the new code are equivalent, so no
objections from me.