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]

Date: Tue, 25 Jun 2024 10:31:54 -0700
From: Xin Li <xin@...or.com>
To: "H. Peter Anvin" <hpa@...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 6/25/2024 10:24 AM, H. Peter Anvin wrote:
> 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:
>>> 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.

Agreed!

> 
> 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.)
> 

I think you and tglx are talking the same thing :)

Thanks!
     Xin