| Message ID | f43b1e80830dc78ed60ed8b0826f4f189254570c.1612924255.git.luto@kernel.org |
|---|---|
| State | Accepted |
| Commit | c46f52231e79af025e2c89e889d69ec20a4c024f |
| Headers | show |
| Series | None | expand |
On Wed, 10 Feb 2021 at 03:34, Andy Lutomirski <luto@kernel.org> 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@linux.intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: linux-efi@vger.kernel.org > Signed-off-by: Andy Lutomirski <luto@kernel.org> Acked-by: Ard Biesheuvel <ardb@kernel.org> > --- > 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 >
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; }
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@linux.intel.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: linux-efi@vger.kernel.org Signed-off-by: Andy Lutomirski <luto@kernel.org> --- 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(-)