Re: [PATCH v2 14/14] x86/fault, x86/efi: Fix and rename efi_recover_from_page_fault()

[Ard Biesheuvel]

On Wed, 10 Feb 2021 at 03:34, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>
> efi_recover_from_page_fault() doesn't recover -- it does a special EFI
> mini-oops.  Rename it to make it clear that it crashes.
>
> While renaming it, I noticed a blatant bug: a page fault oops in a
> different thread happening concurrently with an EFI runtime service call
> would be misinterpreted as an EFI page fault.  Fix that.
>
> This isn't quite exact.  We could do better by using a special CS for
> calls into EFI.
>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Cc: linux-efi@xxxxxxxxxxxxxxx
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>

Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>

> ---
>  arch/x86/include/asm/efi.h     |  2 +-
>  arch/x86/mm/fault.c            | 11 ++++++-----
>  arch/x86/platform/efi/quirks.c | 16 ++++++++++++----
>  3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index c98f78330b09..4b7706ddd8b6 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -150,7 +150,7 @@ extern void __init efi_apply_memmap_quirks(void);
>  extern int __init efi_reuse_config(u64 tables, int nr_tables);
>  extern void efi_delete_dummy_variable(void);
>  extern void efi_switch_mm(struct mm_struct *mm);
> -extern void efi_recover_from_page_fault(unsigned long phys_addr);
> +extern void efi_crash_gracefully_on_page_fault(unsigned long phys_addr);
>  extern void efi_free_boot_services(void);
>
>  /* kexec external ABI */
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index eed217d4a877..dfdd56d9c020 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -16,7 +16,7 @@
>  #include <linux/prefetch.h>            /* prefetchw                    */
>  #include <linux/context_tracking.h>    /* exception_enter(), ...       */
>  #include <linux/uaccess.h>             /* faulthandler_disabled()      */
> -#include <linux/efi.h>                 /* efi_recover_from_page_fault()*/
> +#include <linux/efi.h>                 /* efi_crash_gracefully_on_page_fault()*/
>  #include <linux/mm_types.h>
>
>  #include <asm/cpufeature.h>            /* boot_cpu_has, ...            */
> @@ -25,7 +25,7 @@
>  #include <asm/vsyscall.h>              /* emulate_vsyscall             */
>  #include <asm/vm86.h>                  /* struct vm86                  */
>  #include <asm/mmu_context.h>           /* vma_pkey()                   */
> -#include <asm/efi.h>                   /* efi_recover_from_page_fault()*/
> +#include <asm/efi.h>                   /* efi_crash_gracefully_on_page_fault()*/
>  #include <asm/desc.h>                  /* store_idt(), ...             */
>  #include <asm/cpu_entry_area.h>                /* exception stack              */
>  #include <asm/pgtable_areas.h>         /* VMALLOC_START, ...           */
> @@ -700,11 +700,12 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
>  #endif
>
>         /*
> -        * Buggy firmware could access regions which might page fault, try to
> -        * recover from such faults.
> +        * Buggy firmware could access regions which might page fault.  If
> +        * this happens, EFI has a special OOPS path that will try to
> +        * avoid hanging the system.
>          */
>         if (IS_ENABLED(CONFIG_EFI))
> -               efi_recover_from_page_fault(address);
> +               efi_crash_gracefully_on_page_fault(address);
>
>  oops:
>         /*
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 5a40fe411ebd..0463ef9cddd6 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -687,15 +687,25 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>   * @return: Returns, if the page fault is not handled. This function
>   * will never return if the page fault is handled successfully.
>   */
> -void efi_recover_from_page_fault(unsigned long phys_addr)
> +void efi_crash_gracefully_on_page_fault(unsigned long phys_addr)
>  {
>         if (!IS_ENABLED(CONFIG_X86_64))
>                 return;
>
> +       /*
> +        * If we are in an interrupt nested inside an EFI runtime service,
> +        * then this is a regular OOPS, not an EFI failure.
> +        */
> +       if (in_interrupt() || in_nmi() || in_softirq())
> +               return;
> +
>         /*
>          * Make sure that an efi runtime service caused the page fault.
> +        * READ_ONCE() because we might be OOPSing in a different thread,
> +        * and we don't want to trip KTSAN while trying to OOPS.
>          */
> -       if (efi_rts_work.efi_rts_id == EFI_NONE)
> +       if (READ_ONCE(efi_rts_work.efi_rts_id) == EFI_NONE ||
> +           current_work() != &efi_rts_work.work)
>                 return;
>
>         /*
> @@ -747,6 +757,4 @@ void efi_recover_from_page_fault(unsigned long phys_addr)
>                 set_current_state(TASK_IDLE);
>                 schedule();
>         }
> -
> -       return;
>  }
> --
> 2.29.2
>