| Message ID | 20240610122437.2358778-2-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efi/arm: Disable LPAE PAN when calling EFI runtime services | expand |
On Mon, Jun 10, 2024 at 2:24 PM Ard Biesheuvel <ardb+git@google.com> wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > EFI runtime services are remapped into the lower 1 GiB of virtual > address space at boot, so they are guaranteed to be able to co-exist > with the kernel virtual mappings without the need to allocate space for > them in the kernel's vmalloc region, which is rather small. > > This means those mappings are covered by TTBR0 when LPAE PAN is enabled, > and so 'user' access must be enabled while such calls are in progress. > > To avoid the need to refactor the code that is shared between ARM, arm64 > and other EFI architectures, fold this into efi_set_pgd(). Given that > EFI runtime services are serialized and not pre-emptible, storing the > flags into a global variable is reasonable here - efi_set_pgd() calls > will always occur in pairs on a single CPU. > > Cc: Kees Cook <keescook@chromium.org> > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Makes sense to me! Thanks for looking into this. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, 11 Jun 2024 at 15:17, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Jun 10, 2024 at 2:24 PM Ard Biesheuvel <ardb+git@google.com> wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > EFI runtime services are remapped into the lower 1 GiB of virtual > > address space at boot, so they are guaranteed to be able to co-exist > > with the kernel virtual mappings without the need to allocate space for > > them in the kernel's vmalloc region, which is rather small. > > > > This means those mappings are covered by TTBR0 when LPAE PAN is enabled, > > and so 'user' access must be enabled while such calls are in progress. > > > > To avoid the need to refactor the code that is shared between ARM, arm64 > > and other EFI architectures, fold this into efi_set_pgd(). Given that > > EFI runtime services are serialized and not pre-emptible, storing the > > flags into a global variable is reasonable here - efi_set_pgd() calls > > will always occur in pairs on a single CPU. > > > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Makes sense to me! Thanks for looking into this. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Thanks, I'll queue this up as a EFI fix. Note that I have to fix an error in the patch: CONFIG_ARM_TTBR0_PAN does not exist, it should be CONFIG_CPU_TTBR0_PAN (and don't ask me why it worked because it definitely did - probably forgot to do git commit --amend)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h index 78282ced5038..773fb072c040 100644 --- a/arch/arm/include/asm/efi.h +++ b/arch/arm/include/asm/efi.h @@ -14,6 +14,7 @@ #include <asm/mach/map.h> #include <asm/mmu_context.h> #include <asm/ptrace.h> +#include <asm/uaccess.h> #ifdef CONFIG_EFI void efi_init(void); @@ -32,6 +33,20 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md, boo static inline void efi_set_pgd(struct mm_struct *mm) { check_and_switch_context(mm, NULL); + + if (IS_ENABLED(CONFIG_ARM_TTBR0_PAN)) { + extern unsigned int efi_arm_ttbr0_pan_flags; + + /* + * EFI runtime services are mapped in the lower TTBR0 region, + * so TTBR0 based PAN should be disabled while making a EFI + * runtime service call. + */ + if (mm != current->active_mm) + efi_arm_ttbr0_pan_flags = uaccess_save_and_enable(); + else + uaccess_restore(efi_arm_ttbr0_pan_flags); + } } void efi_virtmap_load(void); diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c index 6f9ec7d28a71..6864338073e6 100644 --- a/arch/arm/kernel/efi.c +++ b/arch/arm/kernel/efi.c @@ -11,6 +11,10 @@ #include <asm/mach/map.h> #include <asm/mmu_context.h> +#ifdef CONFIG_ARM_TTBR0_PAN +unsigned int efi_arm_ttbr0_pan_flags; +#endif + static int __init set_permissions(pte_t *ptep, unsigned long addr, void *data) { efi_memory_desc_t *md = data;