[v5,22/34] x86/fred: FRED initialization code

  From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

The code to initialize FRED when it's available and _not_ disabled.

cpu_init_fred_exceptions() is the core function to initialize FRED,
which
  1. Sets up FRED entrypoints for events happening in ring 0 and 3.
  2. Sets up a default stack for event handling.
  3. Sets up dedicated event stacks for DB/NMI/MC/DF, equivalent to
     the IDT IST stacks.
  4. Forces 32-bit system calls to use "int $0x80" only.
  5. Enables FRED and invalidtes IDT.

When the FRED is used, cpu_init_exception_handling() initializes FRED
through calling cpu_init_fred_exceptions(), otherwise it sets up TSS
IST and loads IDT.

As FRED uses the ring 3 FRED entrypoint for SYSCALL and SYSENTER,
it skips setting up SYSCALL/SYSENTER related MSRs, e.g., MSR_LSTAR.

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Co-developed-by: Xin Li <xin3.li@intel.com>
Tested-by: Shan Kang <shan.kang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
---
 arch/x86/include/asm/fred.h  | 14 +++++++
 arch/x86/include/asm/traps.h |  2 +
 arch/x86/kernel/Makefile     |  1 +
 arch/x86/kernel/cpu/common.c | 74 +++++++++++++++++++++++-------------
 arch/x86/kernel/fred.c       | 73 +++++++++++++++++++++++++++++++++++
 arch/x86/kernel/irqinit.c    |  7 +++-
 arch/x86/kernel/traps.c      | 16 +++++++-
 7 files changed, 157 insertions(+), 30 deletions(-)
 create mode 100644 arch/x86/kernel/fred.c
  

Hello


Comments in cpu_init_fred_exceptions() seem scarce for understanding.

On Tue, Mar 7, 2023 at 11:07 AM Xin Li <xin3.li@intel.com> wrote:

> +/*
> + * Initialize FRED on this CPU. This cannot be __init as it is called
> + * during CPU hotplug.
> + */
> +void cpu_init_fred_exceptions(void)
> +{
> +       wrmsrl(MSR_IA32_FRED_CONFIG,
> +              FRED_CONFIG_ENTRYPOINT(fred_entrypoint_user) |
> +              FRED_CONFIG_REDZONE(8) | /* Reserve for CALL emulation */
> +              FRED_CONFIG_INT_STKLVL(0));

What is it about "Reserve for CALL emulation"?

I guess it relates to X86_TRAP_BP. In entry_64.S:

        .if \vector == X86_TRAP_BP
                /*
                 * If coming from kernel space, create a 6-word gap to allow the
                 * int3 handler to emulate a call instruction.
                 */

> +
> +       wrmsrl(MSR_IA32_FRED_STKLVLS,
> +              FRED_STKLVL(X86_TRAP_DB,  1) |
> +              FRED_STKLVL(X86_TRAP_NMI, 2) |
> +              FRED_STKLVL(X86_TRAP_MC,  2) |
> +              FRED_STKLVL(X86_TRAP_DF,  3));

Why each exception here needs a stack level > 0?
Especially for X86_TRAP_DB and X86_TRAP_NMI.

Why does or why does not X86_TRAP_VE have a stack level > 0?

X86_TRAP_DF is the highest stack level, is it accidental
or deliberate?

Thanks
Lai
  

On March 17, 2023 6:35:57 AM PDT, Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>Hello
>
>
>Comments in cpu_init_fred_exceptions() seem scarce for understanding.
>
>On Tue, Mar 7, 2023 at 11:07 AM Xin Li <xin3.li@intel.com> wrote:
>
>> +/*
>> + * Initialize FRED on this CPU. This cannot be __init as it is called
>> + * during CPU hotplug.
>> + */
>> +void cpu_init_fred_exceptions(void)
>> +{
>> +       wrmsrl(MSR_IA32_FRED_CONFIG,
>> +              FRED_CONFIG_ENTRYPOINT(fred_entrypoint_user) |
>> +              FRED_CONFIG_REDZONE(8) | /* Reserve for CALL emulation */
>> +              FRED_CONFIG_INT_STKLVL(0));
>
>What is it about "Reserve for CALL emulation"?
>
>I guess it relates to X86_TRAP_BP. In entry_64.S:
>
>        .if \vector == X86_TRAP_BP
>                /*
>                 * If coming from kernel space, create a 6-word gap to allow the
>                 * int3 handler to emulate a call instruction.
>                 */
>
>> +
>> +       wrmsrl(MSR_IA32_FRED_STKLVLS,
>> +              FRED_STKLVL(X86_TRAP_DB,  1) |
>> +              FRED_STKLVL(X86_TRAP_NMI, 2) |
>> +              FRED_STKLVL(X86_TRAP_MC,  2) |
>> +              FRED_STKLVL(X86_TRAP_DF,  3));
>
>Why each exception here needs a stack level > 0?
>Especially for X86_TRAP_DB and X86_TRAP_NMI.
>
>Why does or why does not X86_TRAP_VE have a stack level > 0?
>
>X86_TRAP_DF is the highest stack level, is it accidental
>or deliberate?
>
>Thanks
>Lai
>

Yes, the extra redzone space is there to allow for the call emulation without having to adjust the stack frame "manually".

In theory we could enable it only while code patching is in progress, but that would probably just result in stack overflows becoming utterly impossible to debug as we have to consider the worst case.

The purpose of separate stacks for NMI, #DB and #MC *in the kernel* (remember that user space faults are always taken on stack level 0) is to avoid overflowing the kernel stack. #DB in the kernel would imply the use of a kernel debugger.

#DF is the highest level because a #DF means "something went wrong *while delivering an exception*." The number of cases for which that can happen with FRED is drastically reduced and basically amount to "the stack you pointed me to is broken."

Thus, you basically always want to change stacks on #DF, which means it should be at the highest level.
  

On Sat, Mar 18, 2023 at 5:32 AM H. Peter Anvin <hpa@zytor.com> wrote:
>
> On March 17, 2023 6:35:57 AM PDT, Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> >Hello
> >
> >
> >Comments in cpu_init_fred_exceptions() seem scarce for understanding.
> >
> >On Tue, Mar 7, 2023 at 11:07 AM Xin Li <xin3.li@intel.com> wrote:
> >
> >> +/*
> >> + * Initialize FRED on this CPU. This cannot be __init as it is called
> >> + * during CPU hotplug.
> >> + */
> >> +void cpu_init_fred_exceptions(void)
> >> +{
> >> +       wrmsrl(MSR_IA32_FRED_CONFIG,
> >> +              FRED_CONFIG_ENTRYPOINT(fred_entrypoint_user) |
> >> +              FRED_CONFIG_REDZONE(8) | /* Reserve for CALL emulation */
> >> +              FRED_CONFIG_INT_STKLVL(0));
> >
> >What is it about "Reserve for CALL emulation"?
> >
> >I guess it relates to X86_TRAP_BP. In entry_64.S:
> >
> >        .if \vector == X86_TRAP_BP
> >                /*
> >                 * If coming from kernel space, create a 6-word gap to allow the
> >                 * int3 handler to emulate a call instruction.
> >                 */
> >
> >> +
> >> +       wrmsrl(MSR_IA32_FRED_STKLVLS,
> >> +              FRED_STKLVL(X86_TRAP_DB,  1) |
> >> +              FRED_STKLVL(X86_TRAP_NMI, 2) |
> >> +              FRED_STKLVL(X86_TRAP_MC,  2) |
> >> +              FRED_STKLVL(X86_TRAP_DF,  3));
> >
> >Why each exception here needs a stack level > 0?
> >Especially for X86_TRAP_DB and X86_TRAP_NMI.
> >
> >Why does or why does not X86_TRAP_VE have a stack level > 0?
> >
> >X86_TRAP_DF is the highest stack level, is it accidental
> >or deliberate?
> >
> >Thanks
> >Lai
> >
>
> Yes, the extra redzone space is there to allow for the call emulation without having to adjust the stack frame "manually".
>
> In theory we could enable it only while code patching is in progress, but that would probably just result in stack overflows becoming utterly impossible to debug as we have to consider the worst case.
>
> The purpose of separate stacks for NMI, #DB and #MC *in the kernel* (remember that user space faults are always taken on stack level 0) is to avoid overflowing the kernel stack. #DB in the kernel would imply the use of a kernel debugger.

Could you add it to the code, please? I think it can help other reviewers.

If there is no other concrete reason other than overflowing for
assigning NMI and #DB with a stack level > 0, #VE should also
be assigned with a stack level > 0, and #BP too. #VE can happen
anytime and anywhere, so it is subject to overflowing too.

>
>
> #DF is the highest level because a #DF means "something went wrong *while delivering an exception*." The number of cases for which that can happen with FRED is drastically reduced and basically amount to "the stack you pointed me to is broken."
>
> Thus, you basically always want to change stacks on #DF, which means it should be at the highest level.
  

On Fri, Mar 17, 2023 at 02:32:28PM -0700, H. Peter Anvin wrote:
> The purpose of separate stacks for NMI, #DB and #MC *in the kernel*
> (remember that user space faults are always taken on stack level 0) is
> to avoid overflowing the kernel stack. #DB in the kernel would imply
> the use of a kernel debugger.

Perf (and through it bpf) also has access to #DB. They can set
breakpoints on kernel instructions/memory just fine provided permission
etc.
  

On Sat, Mar 18, 2023 at 02:33:30PM +0800, Lai Jiangshan wrote:
> If there is no other concrete reason other than overflowing for
> assigning NMI and #DB with a stack level > 0, #VE should also
> be assigned with a stack level > 0, and #BP too. #VE can happen
> anytime and anywhere, so it is subject to overflowing too.

So #BP needs the stack-gap (redzone) for text_poke_bp().

#BP can end up in kprobes which can then end up in ftrace/perf,
depending on how it's all wired up.

#VE is currently a trainwreck vs NMI/MCE, but I think FRED solves the
worst of that. I'm not exactly sure how deep the #VE handler goes.
  

> > If there is no other concrete reason other than overflowing for
> > assigning NMI and #DB with a stack level > 0, #VE should also be
> > assigned with a stack level > 0, and #BP too. #VE can happen anytime
> > and anywhere, so it is subject to overflowing too.
> 
> So #BP needs the stack-gap (redzone) for text_poke_bp().
> 
> #BP can end up in kprobes which can then end up in ftrace/perf, depending on
> how it's all wired up.
> 
> #VE is currently a trainwreck vs NMI/MCE, but I think FRED solves the worst of
> that. I'm not exactly sure how deep the #VE handler goes.
> 

VE under IDT is *not* using an IST, we need some solid rationales here.

Thanks!
  Xin
  

> > The purpose of separate stacks for NMI, #DB and #MC *in the kernel*
> > (remember that user space faults are always taken on stack level 0) is
> > to avoid overflowing the kernel stack. #DB in the kernel would imply
> > the use of a kernel debugger.
> 
> Perf (and through it bpf) also has access to #DB. They can set
> breakpoints on kernel instructions/memory just fine provided permission
> etc.

So they are still *kernel* debuggers :)
  

> >>> If there is no other concrete reason other than overflowing for
> >>> assigning NMI and #DB with a stack level > 0, #VE should also be
> >>> assigned with a stack level > 0, and #BP too. #VE can happen anytime
> >>> and anywhere, so it is subject to overflowing too.
> >> So #BP needs the stack-gap (redzone) for text_poke_bp().
> >>
> >> #BP can end up in kprobes which can then end up in ftrace/perf, depending
> on
> >> how it's all wired up.
> >>
> >> #VE is currently a trainwreck vs NMI/MCE, but I think FRED solves the worst of
> >> that. I'm not exactly sure how deep the #VE handler goes.
> >>
> > VE under IDT is *not* using an IST, we need some solid rationales here.
> 
> #VE, and #VC on AMD, are borderline unusable.  Both under IDT and FRED.

Oops!

> The reason #VE is not IST is because there are plenty of real cases
> where a non-malicious outer hypervisor could create reentrant faults
> that lose program state.  e.g. hitting an IO instruction, then hitting
> an emulated MSR.
>
> There are fewer cases where a non-IST #VE ends up in a re-entrant fault
> (IIRC, you can still manage it by unmapping the entry stack), but you're
> still trusting the outer hypervisor to not e.g. unmap the SYSCALL entry
> point.
> 
> FRED gets rid of the "reentrant fault overwriting it on the stack" case,
> and removes the syscall gap case, replacing them instead with a stack
> overflow in the worst case because there is still no upper bound to how
> many times #VE can actually be delivered in the course of servicing a
> single #VE.

Exactly, FRED stack levels can make use of the whole regular stack space.

I guess you don't seem to support #VE on a higher stack level? 

> ~Andrew
> 
> P.S. While I hate to cite myself, if you haven't read
> https://docs.google.com/document/d/1hWejnyDkjRRAW-
> JEsRjA5c9CKLOPc6VKJQsuvODlQEI/edit?usp=sharing
> yet, do so.  It did feed into some of the FRED design.
  

> If there is no other concrete reason other than overflowing for assigning NMI and
> #DB with a stack level > 0, #VE should also be assigned with a stack level > 0, and
> #BP too. #VE can happen anytime and anywhere, so it is subject to overflowing too.

With IDT, both #VE and #BP do not use IST, but NMI, #DB, #MC and #DF do.

Let's keep this "secret" logic for now, i.e., not change the stack levels
for #VE and #BP at this point. We can do "optimization", i.e., change them
later :).
  

On 3/21/23 19:22, Li, Xin3 wrote:
>> If there is no other concrete reason other than overflowing for assigning NMI and
>> #DB with a stack level > 0, #VE should also be assigned with a stack level > 0, and
>> #BP too. #VE can happen anytime and anywhere, so it is subject to overflowing too.
> With IDT, both #VE and #BP do not use IST, but NMI, #DB, #MC and #DF do.
> 
> Let's keep this "secret" logic for now, i.e., not change the stack levels
> for #VE and #BP at this point. We can do "optimization", i.e., change them
> later 😄.

#VE also can't happen anywhere.  There is some documentation about it in
here:

	https://docs.kernel.org/x86/tdx.html#linux-ve-handler

But, basically, the only halfway sane thing a guest might do to hit a
#VE is touch some "MMIO".  The host can *not* cause them in arbitrary
places because of the SEPT_VE_DISABLE attribute.

#VE's also can't nest until after the guest retrieves the "VE info".
That means that the #VE handler at _least_ reaches C code before it's
subject to another #VE and that second one would still need to be
induced by something the guest does explicitly.
  

> >> If there is no other concrete reason other than overflowing for assigning NMI
> and
> >> #DB with a stack level > 0, #VE should also be assigned with a stack level > 0,
> and
> >> #BP too. #VE can happen anytime and anywhere, so it is subject to
> overflowing too.
> > With IDT, both #VE and #BP do not use IST, but NMI, #DB, #MC and #DF do.
> >
> > Let's keep this "secret" logic for now, i.e., not change the stack levels
> > for #VE and #BP at this point. We can do "optimization", i.e., change them
> > later 😄.
> 
> #VE also can't happen anywhere.  There is some documentation about it in
> here:
> 
> 	https://docs.kernel.org/x86/tdx.html#linux-ve-handler
> 
> But, basically, the only halfway sane thing a guest might do to hit a
> #VE is touch some "MMIO".  The host can *not* cause them in arbitrary
> places because of the SEPT_VE_DISABLE attribute.
> 
> #VE's also can't nest until after the guest retrieves the "VE info".
> That means that the #VE handler at _least_ reaches C code before it's
> subject to another #VE and that second one would still need to be
> induced by something the guest does explicitly.

Thanks a lot for the detailed background!
  

On 3/20/23 18:02, andrew.cooper3@citrix.com wrote:
> There are fewer cases where a non-IST #VE ends up in a re-entrant fault
> (IIRC, you can still manage it by unmapping the entry stack), but you're
> still trusting the outer hypervisor to not e.g. unmap the SYSCALL entry
> point.

This is a general weakness of #VE.  But, the current Linux TDX guest
implementation is not vulnerable to it.  If the host unmaps something
unexpectedly, the guest will just die because of ATTR_SEPT_VE_DISABLE.
No #VE:

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/coco/tdx/tdx.c#n216