linux-kernel - Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6D3EF8F0-9D70-438B-9B9C-9BC1A64E17E3@zytor.com>
Date: Tue, 25 Jun 2024 10:24:53 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Xin Li <xin@...or.com>, Hou Wenlong <houwenlong.hwl@...group.com>
CC: linux-kernel@...r.kernel.org, Lai Jiangshan <jiangshan.ljs@...group.com>,
        Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        Xin Li <xin3.li@...el.com>, Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

On June 25, 2024 2:09:29 AM PDT, Xin Li <xin@...or.com> wrote:
>On 6/23/2024 11:21 PM, Hou Wenlong wrote:
>> On Sat, Jun 22, 2024 at 08:31:26AM +0800, Xin Li wrote:
>>> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
>>>> One issue is that FRED can be disabled in trap_init(), but
>>>> sysvec_install() can be called before trap_init(), thus the system
>>>> interrupt handler is not installed into the IDT if FRED is disabled
>>>> later. Initially, I attempted to parse the cmdline and decide whether to
>>>> enable or disable FRED after parse_early_param(). However, I ultimately
>>>> chose to always install the system handler into the IDT in
>>>> sysvec_install(), which is simple and should be sufficient.
>>> 
>>> Which module with a system vector gets initialized before trap_init() is
>>> called?
>>> 
>> Sorry, I didn't mention the case here. I see sysvec_install() is used
>> only in the guest part (KVM, HYPERV) currently. For example, the KVM
>> guest will register the HYPERVISOR_CALLBACK_VECTOR APF handler in
>> kvm_guest_init(), which is called before trap_init(). So if only the FRED
>> handler is registered and FRED is disabled in trap_init() later, then the
>> IDT handler of the APF handler is missing.
>
>This is a bug!  Your fix looks good to me.
>
>>>> Another problem is that the page fault handler (exc_page_fault()) is
>>>> installed into the IDT before FRED is enabled. Consequently, if a #PF is
>>>> triggered in this gap, the handler would receive the wrong CR2 from the
>>>> stack if FRED feature is present. To address this, I added a page fault
>>>> entry stub for FRED similar to the debug entry. However, I'm uncertain
>>>> whether this is enough reason to add a new entry. Perhaps a static key
>>>> may suffice to indicate whether FRED setup is completed and the handler
>>>> can use it.
>>> 
>>> How could a #PF get triggered during that gap?
>>> 
>>> Initialization time funnies are really unpleasant.
>>> 
>> I'm not sure if there will be a #PF during that gap; I just received the
>> wrong fault address when I made a mistake in that gap and a #PF
>> occurred. Before idt_setup_early_pf(), the registered page fault handler
>> is do_early_exception(), which uses native_read_cr2(). However, after
>> that, the page fault handler had been changed to exc_page_fault(), so if
>> something bad happened and an unexpected #PF occurred, the fault address
>> in the error output will be wrong, although the CR2 in __show_regs() is
>> correct. I'm not sure if this matters or not since the kernel will panic
>> at that time.
>
>So this doesn't sound a real problem, right?
>
>We could simply do:
>
>diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>index e6c469b323cc..e500777ed2b4 100644
>--- a/arch/x86/mm/fault.c
>+++ b/arch/x86/mm/fault.c
>@@ -1495,7 +1495,7 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
>           irqentry_state_t state;
>           unsigned long address;
>
>-       address = cpu_feature_enabled(X86_FEATURE_FRED) ? fred_event_data(regs) : read_cr2();
>+       address = native_read_cr4() & X86_CR4_FRED ? fred_event_data(regs) : read_cr2();
>
>           prefetchw(&current->mm->mmap_lock);
>
>
>But the page fault handler is an extreme hot path, it's not worth it.
>
>Thanks!
>       Xin
>
>
>
>

Reading CR4 is insane from a performance perspective. Also, there is pretty much never a reason to, since CR4 is programmed by the OS.

But this is basically insane. We should enable FRED from the point we cut over from the early exception vector. That is: 

Early IDT → Final IDT
or
Early IDT → FRED

But not

Early IDT → Final IDT → FRED

Eventually we should enable FRED for early exceptions too (which ought to be quite trivial, but makes the whole CLI enable/disable a bit of a mess.)