x86/smp: Set up exception handling before cr4_init()
Subsystems (2)
PATCH 1/1 x86/smp: Set up exception handling before cr4_init()
The current AP boot sequence initializes CR4 before setting up exception handling. With FRED enabled, however, CR4.FRED is set prior to initializing the FRED configuration MSRs, introducing a brief window where a triple fault could occur. This wasn't considered a problem, as the early boot code was carefully designed to avoid triggering exceptions. Moreover, if an exception does occur at this stage, it's preferable for the CPU to triple fault rather than risk a potential exploit.
However, under TDX or SEV-ES/SNP, printk() triggers a #VE or #VC, so any logging during this small window results in a triple fault.
Swap the order of cr4_init() and cpu_init_exception_handling(), since cr4_init() only involves reading from and writing to CR4, and setting up exception handling does not depend on any specific CR4 bits being set (Arguably CR4.PAE, CR4.PSE and CR4.PGE are related but they are already set before start_secondary() anyway).
Notably, this triple fault can still occur before FRED is enabled, while the bringup IDT is in use, since it lacks a #VE handler for TDX.
BTW, on 32-bit systems, loading CR3 with swapper_pg_dir is moved ahead of cr4_init(), which appears to be harmless.
--- arch/x86/kernel/smpboot.c | 41 ++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-)
arch/x86/kernel/smpboot.c +34 -7
@@ -228,13 +228,6 @@ static void ap_calibrate_delay(void)*/ static void notrace __noendbr start_secondary(void *unused) { -/* -* Don't put *anything* except direct CPU state initialization -* before cpu_init(), SMP booting is too fragile that we want to -* limit the things done here to the most necessary things. -*/ -cr4_init (); -/* * 32-bit specific. 64-bit reaches this code with the correct page * table established. Yet another historical divergence.
@@ -245,8 +238,37 @@ static void notrace __noendbr start_secondary(void *unused)__flush_tlb_all (); } +/* +* AP startup assembly code has setup the following before calling +* start_secondary() on 64-bit: +* +* 1) CS set to __KERNEL_CS. +* 2) CR3 switched to the init_top_pgt. +* 3) CR4.PAE, CR4.PSE and CR4.PGE are set. +* 4) GDT set to per-CPU gdt_page. +* 5) ALL data segments set to the NULL descriptor. +* 6) MSR_GS_BASE set to per-CPU offset. +* 7) IDT set to bringup IDT. +* 8) CR0 set to CR0_STATE. +* +* So it's ready to setup exception handling. +*/ cpu_init_exception_handling (false); +/* +* Ensure bits set in cr4_pinned_bits are set in CR4. +* +* cr4_pinned_bits is a subset of cr4_pinned_mask, which includes +* the following bits: +* X86_CR4_SMEP +* X86_CR4_SMAP +* X86_CR4_UMIP +* X86_CR4_FSGSBASE +* X86_CR4_CET +* X86_CR4_FRED +*/ +cr4_init (); +/* * Load the microcode before reaching the AP alive synchronization * point below so it is not part of the full per CPU serialized
@@ -272,6 +294,11 @@ static void notrace __noendbr start_secondary(void *unused)*/ cpuhp_ap_sync_alive (); +/* +* Don't put *anything* except direct CPU state initialization +* before cpu_init(), SMP booting is too fragile that we want to +* limit the things done here to the most necessary things. +*/ cpu_init ();fpu__init_cpu ();rcutree_report_cpu_starting (raw_smp_processor_id ());
Signature · 1 line
2.52.0
Quoted 6 lines
arch/x86/kernel/smpboot.c +6
-0+/* +* AP startup assembly code has setup the following before calling +* start_secondary() on 64-bit: +* +* 1) CS set to __KERNEL_CS. +* 2) CR3 switched to the init_top_pgt.
arch/x86/kernel/smpboot.c static void notrace __noendbr start_secondary(void *unused) · line 256
Quoted 10 lines
arch/x86/kernel/smpboot.c +9
-0+* 3) CR4.PAE, CR4.PSE and CR4.PGE are set. +* 4) GDT set to per-CPU gdt_page. +* 5) ALL data segments set to the NULL descriptor. +* 6) MSR_GS_BASE set to per-CPU offset. +* 7) IDT set to bringup IDT. +* 8) CR0 set to CR0_STATE. +* +* So it's ready to setup exception handling. +*/ cpu_init_exception_handling (false);
This is fine because very little of that ^ is ever going to change. It's not great that it duplicates the assembly logic, but it's good documentation I guess.
arch/x86/kernel/smpboot.c static void notrace __noendbr start_secondary(void *unused) · line 270
Quoted 13 lines
arch/x86/kernel/smpboot.c +13
-0+/* +* Ensure bits set in cr4_pinned_bits are set in CR4. +* +* cr4_pinned_bits is a subset of cr4_pinned_mask, which includes +* the following bits: +* X86_CR4_SMEP +* X86_CR4_SMAP +* X86_CR4_UMIP +* X86_CR4_FSGSBASE +* X86_CR4_CET +* X86_CR4_FRED +*/ +cr4_init ();
I'm not as big of a fan of this comment. The next pinned bit that gets added will make this stale. Could we try to make this more timeless, please?
I'm also not sure I like the asymmetry of this between the boot and secondary CPUs. On a boot CPU, CR4.SMEP will get set via identify_boot_cpu() and eventually setup_smep(). On a secondary CPU, it'll get set in cr4_init() and *not* in setup_smep().
This asymmetry is (I think) part of what the root of the problem is here and how this bug came to be.
I really think the current pinning behavior is too invasive. It has zero benefit to be pinning CR bits this early in bringup. It's only causing pain.
I _really_ think we need a defined per-cpu point where pinning comes into effect. Marking the CPU online is one idea.
Thoughts?
Quoted 15 lines
Quoted 4 lines
+ /*
(quoted diff) +2
-0+ * Ensure bits set in cr4_pinned_bits are set in CR4. + *Review
Quoted 10 lines
(quoted diff) +10
-0+ * cr4_pinned_bits is a subset of cr4_pinned_mask, which includes + * the following bits: + * X86_CR4_SMEP + * X86_CR4_SMAP + * X86_CR4_UMIP + * X86_CR4_FSGSBASE + * X86_CR4_CET + * X86_CR4_FRED + */ + cr4_init();
Quoted 2 lines
I'm not as big of a fan of this comment. The next pinned bit that gets added will make this stale. Could we try to make this more timeless, please?
Right, it’s still a potential problem.
Quoted 8 lines
I'm also not sure I like the asymmetry of this between the boot and secondary CPUs. On a boot CPU, CR4.SMEP will get set via
identify_boot_cpu()and eventuallysetup_smep(). On a secondary CPU, it'll get set incr4_init()and *not* insetup_smep().This asymmetry is (I think) part of what the root of the problem is here and how this bug came to be.
Are you proposing a single CPU initialization function for both BSP and APs?
It reminds me that tglx proposed the following change when I was fixing a FRED boot order bug.
arch/x86/kernel/cpu/common.c +1 -1
@@ -2176,7 +2176,7 @@ static inline void tss_setup_io_bitmap(struct tss_struct *tss)* Setup everything needed to handle exceptions from the IDT, including the IST * exceptions which use paranoid_entry(). */ -void cpu_init_exception_handling (void ) +void cpu_init_exception_handling (bool boot_cpu) {struct tss_struct *tss =this_cpu_ptr (&cpu_tss_rw);int cpu =raw_smp_processor_id ();
So something like
void cpu_init(bool boot_cpu)
and it gets called on both BSP and APs.
Quoted 3 lines
I really think the current pinning behavior is too invasive. It has zero benefit to be pinning CR bits this early in bringup. It's only causing pain.
Maybe someone can explain why it’s added like this before I find time to dig into it.
I’m curious why cr4_init() is not part of the following cpu_init()? IOW, why does it need to be called so early in the existing code?
Quoted 5 lines
I _really_ think we need a defined per-cpu point where pinning comes into effect. Marking the CPU online is one idea.
Thoughts?
It seems a good fit. Just that {on,off}line() are not called on BSP (not a real problem).
Question is that who would work on it ;) ?
Quoted 4 lines · Xin Li
I’m curious why
cr4_init()is not part of the followingcpu_init()? IOW, why does it need to be called so early in the existing code?
The name cpu_init() is misleading. Most of the pinned features don't get initialized in cpu_init(). They are set up slightly later:
start_secondary()
ap_starting()
identify_secondary_cpu()
identify_cpu()
The original reason for writing CR4 early on APs probably originates in commit c7ad5ad297e6 ("x86/mm/64: Initialize CR4.PCIDE early"). Then, when CR pinning was introduced, it was a global system-wide concept. So, the pinned bits had to be programmed when the first write to CR4 happened.
Quoted 7 lines
Quoted 6 lines
I _really_ think we need a defined per-cpu point where pinning comes into effect. Marking the CPU online is one idea.
Thoughts?
I think this approach could work. It should cover APs as well as hotplug CPUs that come online later.
Quoted 3 lines
It seems a good fit. Just that {on,off}line() are not called on BSP (not a real problem).
The BSP is marked online in boot_cpu_init()->set_cpu_online(). So, it should be covered as well.
Quoted 1 line
Question is that who would work on it ;) ?
I think Dave already posted the patch for it here.
https://lore.kernel.org/lkml/[email protected]/
I will test that out to confirm that it doesn't mess up some implicit behavior.
Quoted 14 lines
Quoted 5 lines
I’m curious why
cr4_init()is not part of the followingcpu_init()? IOW, why does it need to be called so early in the existing code?The name
cpu_init()is misleading. Most of the pinned features don't get initialized incpu_init(). They are set up slightly later:
start_secondary()
ap_starting()
identify_secondary_cpu()
identify_cpu()
Quoted 4 lines
The original reason for writing CR4 early on APs probably originates in commit
c7ad5ad297e6("x86/mm/64: Initialize CR4.PCIDE early"). Then, when CR pinning was introduced, it was a global system-wide concept. So, the pinned bits had to be programmed when the first write to CR4 happened.
Thanks for digging into it and explain it.
Quoted 19 lines
Quoted 8 lines
Quoted 7 lines
I _really_ think we need a defined per-cpu point where pinning comes into effect. Marking the CPU online is one idea.
Thoughts?
I think this approach could work. It should cover APs as well as hotplug CPUs that come online later.
Quoted 3 lines
It seems a good fit. Just that {on,off}line() are not called on BSP (not a real problem).
The BSP is marked online in boot_cpu_init()->set_cpu_online(). So, it should be covered as well.
Quoted 8 lines
Quoted 1 line
Question is that who would work on it ;) ?
I think Dave already posted the patch for it here.
https://lore.kernel.org/lkml/[email protected]/I will test that out to confirm that it doesn't mess up some implicit behavior.
I’m not sure if Dave also wants to make BSP/AP boot code symmetric at the same time ;)
Quoted 2 lines · Xin Li
I’m not sure if Dave also wants to make BSP/AP boot code symmetric at the same time 😉
It's perfectly fine to do things one bit at a time. In this case, cr4_init() is does two different things underneath the covers depending on the state of CR pinning. If CR pinning as a feature is kicked out influencing the early CPU boot code at _all_, then it certainly helps the goal if making the AP/BSP code more symmetric.
So I very much prefer doing things one bit at a time, if we can.
Quoted 6 lines · Sohil Mehta
I think Dave already posted the patch for it here.
https://lore.kernel.org/lkml/[email protected]/I will test that out to confirm that it doesn't mess up some implicit behavior.
I verified that the above patch works as expected. I added a debug print in cpu_init_fred_exceptions() to test FRED behavior.
if (cr4_read_shadow() & X86_CR4_FRED)
pr_warn("FRED is already enabled on CPU%d\n",
smp_processor_id());With Dave's patch, the warning no longer shows up on APs.
However, I found another location where we enable FRED in CR4 before enabling the MSRs.
__restore_processor_state():
...
__write_cr4(ctxt->cr4);
...
if (ctxt->cr4 & X86_CR4_FRED) {
cpu_init_fred_exceptions();
cpu_init_fred_rsps();
}Due to limitations of my test platform, I couldn't verify the FRED print in __restore_processor_state()'s path. But, could "restore" run into a similar issue in the future?
Quoted 12 lines · Sohil Mehta
However, I found another location where we enable FRED in CR4 before enabling the MSRs.
__restore_processor_state():...
__write_cr4(ctxt->cr4);...
Quoted 8 lines
if (ctxt->cr4 & X86_CR4_FRED) { cpu_init_fred_exceptions(); cpu_init_fred_rsps(); }Due to limitations of my test platform, I couldn't verify the FRED print in __restore_processor_state()'s path. But, could "restore" run into a similar issue in the future?
It sure looks like it. Good catch! I have the feeling there's never been an exception in that code anyway. The:
wrmsrq(MSR_GS_BASE, ctxt->kernelmode_gs_base);
is misplaced too. Exception handling leans heavily on MSR_GS_BASE.
But, it doesn't hurt to be consistent about the ordering.
In a perfect world, we'd probably unify all this code. Maybe the boot code establishes a 'saved_context' and all the APs just restore_processor_state() to go online normally instead of just for a restore. Or maybe the restore_processor_state() should be unified more with start_secondary() because a big chunk of the stuff its restoring is pretty static already.
But there's no need today to do anything that drastic.
Quoted 8 lines · Dave Hansen
Quoted 3 lines · Sohil Mehta
However, I found another location where we enable FRED in CR4 before enabling the MSRs.
IIRC, it’s expected:
Quoted 36 lines
Quoted 9 lines
__restore_processor_state():...
__write_cr4(ctxt->cr4);...
Quoted 8 lines
if (ctxt->cr4 & X86_CR4_FRED) { cpu_init_fred_exceptions(); cpu_init_fred_rsps();}
Due to limitations of my test platform, I couldn't verify the FRED print in __restore_processor_state()'s path. But, could "restore" run into a similar issue in the future?
It sure looks like it. Good catch! I have the feeling there's never been an exception in that code anyway. The:
wrmsrq(MSR_GS_BASE, ctxt->kernelmode_gs_base);is misplaced too. Exception handling leans heavily on
MSR_GS_BASE.But, it doesn't hurt to be consistent about the ordering.
In a perfect world, we'd probably unify all this code. Maybe the boot code establishes a '
saved_context' and all the APs justrestore_processor_state()to go online normally instead of just for a restore. Or maybe therestore_processor_state()should be unified more withstart_secondary()because a big chunk of the stuff its restoring is pretty static already.But there's no need today to do anything that drastic.