| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMj1kXFVaF8_EbsdQwPagci=3==qgvqLY6bGE8fMK2V-bzG03g@mail.gmail.com>
Date: Thu, 11 Feb 2021 09:38:58 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Andy Lutomirski <luto@...nel.org>
Cc: X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Yonghong Song <yhs@...com>,
Masami Hiramatsu <mhiramat@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
linux-efi <linux-efi@...r.kernel.org>
Subject: Re: [PATCH v2 14/14] x86/fault, x86/efi: Fix and rename efi_recover_from_page_fault()
On Wed, 10 Feb 2021 at 03:34, Andy Lutomirski <luto@...nel.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@...ux.intel.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ard Biesheuvel <ardb@...nel.org>
> Cc: linux-efi@...r.kernel.org
> Signed-off-by: Andy Lutomirski <luto@...nel.org>
Acked-by: Ard Biesheuvel <ardb@...nel.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
>
Powered by blists - more mailing lists