[PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Xin Li (Intel)]

The WFE, i.e., WAIT_FOR_ENDBRANCH, bit in the augmented CS of FRED
stack frame is set to 1 in missing-ENDBRANCH #CP exceptions.

The CPU will generate another missing-ENDBRANCH #CP if the WFE bit
is left set, because the CPU IBT will be set in the WFE state upon
completion of the following ERETS instruction and then the CPU will
resume from the instruction that just caused the previous #CP.

Clear WFE to avoid dead looping in missing-ENDBRANCH #CPs.

Describe the IBT story in the comment of ibt_clear_fred_wfe() using
Andrew Cooper's write-up.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---

Changes since v1:
* Rewrite the comment of ibt_clear_fred_wfe() using Andrew Cooper's
  write-up (Andrew Cooper).
* Unconditionally clear WFE in missing-ENDBRANCH #CPs (Peter Zijlstra).
* Don't check X86_FEATURE_FRED in ibt_clear_fred_wfe() (Dave Hansen &
  Andrew Cooper).
---
 arch/x86/kernel/cet.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
index d2c732a34e5d..d2cf6ee0d9a0 100644
--- a/arch/x86/kernel/cet.c
+++ b/arch/x86/kernel/cet.c
@@ -81,6 +81,36 @@ static void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code)
 
 static __ro_after_init bool ibt_fatal = true;
 
+/*
+ * By definition, all missing-ENDBRANCH #CPs are a result of WFE && !ENDBR.
+ *
+ * But, in original CET under IDT delivery, any transfer for
+ * interrupt/exception/etc that does not change privilege will clobber the
+ * WFE state because MSR_{U,S}_CET.WFE is intentionally set by microcode so
+ * as to expect to find an ENDBR at the interrupt/exception/syscall entrypoint.
+ *
+ * In practice, this means interrupts and exceptions hitting the kernel, or
+ * user interrupts, lose the WFE state of the interrupted context.  And
+ * yes, this means that a well timed interrupt (to the precise instruction
+ * boundary) will let an attacker sneak a bad function pointer past the
+ * CET-IBT enforcement.
+ *
+ * In FRED, the WFE state of the interrupted context (even if it is the
+ * same privilege) is preserved and restored, in order to close this hole.
+ *
+ * Therefore, the missing-ENDBRANCH #CP handler needs to clear WFE to avoid
+ * dead looping, now that FRED is causing the state not to get lost.  Otherwise
+ * if the WFE bit is left set in an intentional ibt selftest or when !ibt_fatal,
+ * the CPU will generate another missing-ENDBRANCH #CP because the IBT will be
+ * set in the WFE state upon completion of the following ERETS instruction and
+ * then the CPU will resume from the instruction that just caused the previous
+ * missing-ENDBRANCH #CP.
+ */
+static void ibt_clear_fred_wfe(struct pt_regs *regs)
+{
+	regs->fred_cs.wfe = 0;
+}
+
 static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
 {
 	if ((error_code & CP_EC) != CP_ENDBR) {
@@ -88,6 +118,8 @@ static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
 		return;
 	}
 
+	ibt_clear_fred_wfe(regs);
+
 	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
 		regs->ax = 0;
 		return;

base-commit: fe85ee391966c4cf3bfe1c405314e894c951f521
-- 
2.46.0

Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Xin Li]

On 9/16/2024 11:10 AM, Xin Li (Intel) wrote:
> The WFE, i.e., WAIT_FOR_ENDBRANCH, bit in the augmented CS of FRED
> stack frame is set to 1 in missing-ENDBRANCH #CP exceptions.
> 
> The CPU will generate another missing-ENDBRANCH #CP if the WFE bit
> is left set, because the CPU IBT will be set in the WFE state upon
> completion of the following ERETS instruction and then the CPU will
> resume from the instruction that just caused the previous #CP.
> 
> Clear WFE to avoid dead looping in missing-ENDBRANCH #CPs.
> 
> Describe the IBT story in the comment of ibt_clear_fred_wfe() using
> Andrew Cooper's write-up.
> 
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> 
> Changes since v1:
> * Rewrite the comment of ibt_clear_fred_wfe() using Andrew Cooper's
>    write-up (Andrew Cooper).
> * Unconditionally clear WFE in missing-ENDBRANCH #CPs (Peter Zijlstra).
> * Don't check X86_FEATURE_FRED in ibt_clear_fred_wfe() (Dave Hansen &
>    Andrew Cooper).
> ---
>   arch/x86/kernel/cet.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index d2c732a34e5d..d2cf6ee0d9a0 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -81,6 +81,36 @@ static void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code)
>   
>   static __ro_after_init bool ibt_fatal = true;
>   
> +/*
> + * By definition, all missing-ENDBRANCH #CPs are a result of WFE && !ENDBR.
> + *
> + * But, in original CET under IDT delivery, any transfer for
> + * interrupt/exception/etc that does not change privilege will clobber the
> + * WFE state because MSR_{U,S}_CET.WFE is intentionally set by microcode so
> + * as to expect to find an ENDBR at the interrupt/exception/syscall entrypoint.
> + *
> + * In practice, this means interrupts and exceptions hitting the kernel, or
> + * user interrupts, lose the WFE state of the interrupted context.  And
> + * yes, this means that a well timed interrupt (to the precise instruction
> + * boundary) will let an attacker sneak a bad function pointer past the
> + * CET-IBT enforcement.
> + *
> + * In FRED, the WFE state of the interrupted context (even if it is the
> + * same privilege) is preserved and restored, in order to close this hole.
> + *
> + * Therefore, the missing-ENDBRANCH #CP handler needs to clear WFE to avoid
> + * dead looping, now that FRED is causing the state not to get lost.  Otherwise
> + * if the WFE bit is left set in an intentional ibt selftest or when !ibt_fatal,
> + * the CPU will generate another missing-ENDBRANCH #CP because the IBT will be
> + * set in the WFE state upon completion of the following ERETS instruction and
> + * then the CPU will resume from the instruction that just caused the previous
> + * missing-ENDBRANCH #CP.
> + */
> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
> +{
> +	regs->fred_cs.wfe = 0;
> +}
> +
>   static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
>   {
>   	if ((error_code & CP_EC) != CP_ENDBR) {
> @@ -88,6 +118,8 @@ static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
>   		return;
>   	}
>   
> +	ibt_clear_fred_wfe(regs);
> +
>   	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
>   		regs->ax = 0;
>   		return;
> 
> base-commit: fe85ee391966c4cf3bfe1c405314e894c951f521


Andrew,

can you please take another look?

If there is no obvious problem, please give it a review-by?

Thanks!
     Xin

Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Dave Hansen]

On 11/7/24 10:54, Xin Li wrote:
> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
> +{
> +    regs->fred_cs.wfe = 0;
> +}

Also, just looking at this, we have obviously FRED-specific code without
any FRED checks.  Now I know we asked you to remove those checks, but
this does need commenting or at least a changelog sentence on why this
incorrect-looking code is still OK.

Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Dave Hansen]

On 9/16/24 11:10, Xin Li (Intel) wrote:
> The WFE, i.e., WAIT_FOR_ENDBRANCH, bit in the augmented CS of FRED
> stack frame is set to 1 in missing-ENDBRANCH #CP exceptions.
> 
> The CPU will generate another missing-ENDBRANCH #CP if the WFE bit
> is left set, because the CPU IBT will be set in the WFE state upon
> completion of the following ERETS instruction and then the CPU will
> resume from the instruction that just caused the previous #CP.
> 
> Clear WFE to avoid dead looping in missing-ENDBRANCH #CPs.
> 
> Describe the IBT story in the comment of ibt_clear_fred_wfe() using
> Andrew Cooper's write-up.

I should have responded to this earlier.  I do see why Andrew thought my
earlier description was off base.  Let me see if I can try for a better
changelog:

The kernel can enable Indirect Branch Tracking (IBT) for itself.
Hardware generates a #CP exception if a kernel indirect branch lands
somewhere other than an ENDBR instruction.  The kernel #CP handler then
decides if the it should warn or do a fatal BUG().

The BUG() case works fine with or without FRED.  But the warning mode is
broken with FRED.

In short, the pre-FRED architecture clobbers the kernel IBT state of an
interrupted context.  That includes clobbering the state of IBT when the
#CP went off, suppressing future #CP's.  This is bad architecture, but
handy for a #CP handler that wants to suppress those future #CP's.

FRED, on the other hand, provides space on the entry stack (in an
expanded CS area) to save and restore IBT state. Since the hardware
doesn't clobber the IBT state, software must do it instead.

Aside:
	Why does without-FRED case work? There is only one CET WFE bit
	per privilege level.  The #CP handler itself has an ENDBR
	instruction.  That ENDBR clears WFE on the way to handling the
	#CP. Consider what would happen if a kernel indirect call landed
	on an XOR instead of an ENDBR:

	  CALL (*%rax)  // sets WFE
	  XOR %rax,%rax // uh oh, not an ENDBR
	  #CP
	  ENDBR	// first instruction in CP handler, clears WFE
	  ... handle #CP here
	  IRET
	  XOR %rax,%rax // No problem, WFE still clear!

	See? The handler clears WFE and lets the XOR run.

--

Is that a more complete (and accurate) story for folks?

Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Andrew Cooper]

On 07/11/2024 8:51 pm, Dave Hansen wrote:
> On 9/16/24 11:10, Xin Li (Intel) wrote:
>> The WFE, i.e., WAIT_FOR_ENDBRANCH, bit in the augmented CS of FRED
>> stack frame is set to 1 in missing-ENDBRANCH #CP exceptions.
>>
>> The CPU will generate another missing-ENDBRANCH #CP if the WFE bit
>> is left set, because the CPU IBT will be set in the WFE state upon
>> completion of the following ERETS instruction and then the CPU will
>> resume from the instruction that just caused the previous #CP.
>>
>> Clear WFE to avoid dead looping in missing-ENDBRANCH #CPs.
>>
>> Describe the IBT story in the comment of ibt_clear_fred_wfe() using
>> Andrew Cooper's write-up.
> I should have responded to this earlier.  I do see why Andrew thought my
> earlier description was off base.  Let me see if I can try for a better
> changelog:
>
> The kernel can enable Indirect Branch Tracking (IBT) for itself.
> Hardware generates a #CP exception if a kernel indirect branch lands
> somewhere other than an ENDBR instruction.  The kernel #CP handler then
> decides if the it should warn or do a fatal BUG().

Perhaps "an appropriate ENDBR"?

You also get #CP[endbr] for encountering the wrong ENDBR{32,64} instruction.

> The BUG() case works fine with or without FRED.  But the warning mode is
> broken with FRED.
>
> In short, the pre-FRED architecture clobbers the kernel IBT state of an
> interrupted context.  That includes clobbering the state of IBT when the
> #CP went off, suppressing future #CP's.  This is bad architecture, but
> handy for a #CP handler that wants to suppress those future #CP's.

There isn't really a warning mode, so much as a singleton selftest to
check that #CP gets generated.



>
> FRED, on the other hand, provides space on the entry stack (in an
> expanded CS area) to save and restore IBT state. Since the hardware
> doesn't clobber the IBT state, software must do it instead.
>
> Aside:
> 	Why does without-FRED case work? There is only one CET WFE bit
> 	per privilege level.  The #CP handler itself has an ENDBR
> 	instruction.  That ENDBR clears WFE on the way to handling the
> 	#CP. Consider what would happen if a kernel indirect call landed
> 	on an XOR instead of an ENDBR:
>
> 	  CALL (*%rax)  // sets WFE
> 	  XOR %rax,%rax // uh oh, not an ENDBR
> 	  #CP
> 	  ENDBR	// first instruction in CP handler, clears WFE
> 	  ... handle #CP here
> 	  IRET
> 	  XOR %rax,%rax // No problem, WFE still clear!
>
> 	See? The handler clears WFE and lets the XOR run.
>
> Is that a more complete (and accurate) story for folks?

More complete, yes.

"The #CP handler itself has an ENDBR instruction." is a consequence, not
a cause.

CALL *ind does indeed set WFE, and WFE stays asserted across the
instruction boundary, but behaves somewhat like the Resume Flag (falls
to zero everywhere else).  It's not "ENDBR clears WFE" because there's
the suppress state too generated by the NOTRK prefix, which causes WFE
to fall to 0 on all instructions.

When decode finds an instruction, and WFE is set, and the instruction is
not the right ENDBR, it raises a #CP fault.

IDT event delivery sets WFE=1 because it delivered an event, because the
spec says so.

If the old context was CPL3, then the interrupted context's WFE is
stashed away in MSR_U_CET.  But, if the old context was CPL<3, the WFE=1
both clobbers the interrupted context, and requires there to be an ENDBR
in the handler.

On the way out, the problem is that IRET doesn't set WFE=? in the
returned-to context.

What FRED does is stash the interrupted context's WFE on the stack, and
ERET{S,U} restores it on the way out.

Is this any clearer?

~Andrew

P.S. for bonus points, consider what happens if a regular interrupt
occurs between CALL *(%rax) and XOR.  If you can generate precise
interrupts, you can escape CET-IBT protections indefinitely.

Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Andrew Cooper]

On 07/11/2024 6:54 pm, Xin Li wrote:
> On 9/16/2024 11:10 AM, Xin Li (Intel) wrote:
>> The WFE, i.e., WAIT_FOR_ENDBRANCH, bit in the augmented CS of FRED
>> stack frame is set to 1 in missing-ENDBRANCH #CP exceptions.
>>
>> The CPU will generate another missing-ENDBRANCH #CP if the WFE bit
>> is left set, because the CPU IBT will be set in the WFE state upon
>> completion of the following ERETS instruction and then the CPU will
>> resume from the instruction that just caused the previous #CP.
>>
>> Clear WFE to avoid dead looping in missing-ENDBRANCH #CPs.
>>
>> Describe the IBT story in the comment of ibt_clear_fred_wfe() using
>> Andrew Cooper's write-up.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>> ---
>>
>> Changes since v1:
>> * Rewrite the comment of ibt_clear_fred_wfe() using Andrew Cooper's
>>    write-up (Andrew Cooper).
>> * Unconditionally clear WFE in missing-ENDBRANCH #CPs (Peter Zijlstra).
>> * Don't check X86_FEATURE_FRED in ibt_clear_fred_wfe() (Dave Hansen &
>>    Andrew Cooper).
>> ---
>>   arch/x86/kernel/cet.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
>> index d2c732a34e5d..d2cf6ee0d9a0 100644
>> --- a/arch/x86/kernel/cet.c
>> +++ b/arch/x86/kernel/cet.c
>> @@ -81,6 +81,36 @@ static void do_user_cp_fault(struct pt_regs *regs,
>> unsigned long error_code)
>>     static __ro_after_init bool ibt_fatal = true;
>>   +/*
>> + * By definition, all missing-ENDBRANCH #CPs are a result of WFE &&
>> !ENDBR.
>> + *
>> + * But, in original CET under IDT delivery, any transfer for
>> + * interrupt/exception/etc that does not change privilege will
>> clobber the
>> + * WFE state because MSR_{U,S}_CET.WFE is intentionally set by
>> microcode so
>> + * as to expect to find an ENDBR at the interrupt/exception/syscall
>> entrypoint.
>> + *
>> + * In practice, this means interrupts and exceptions hitting the
>> kernel, or
>> + * user interrupts, lose the WFE state of the interrupted context.  And
>> + * yes, this means that a well timed interrupt (to the precise
>> instruction
>> + * boundary) will let an attacker sneak a bad function pointer past the
>> + * CET-IBT enforcement.
>> + *
>> + * In FRED, the WFE state of the interrupted context (even if it is the
>> + * same privilege) is preserved and restored, in order to close this
>> hole.
>> + *
>> + * Therefore, the missing-ENDBRANCH #CP handler needs to clear WFE
>> to avoid
>> + * dead looping, now that FRED is causing the state not to get
>> lost.  Otherwise
>> + * if the WFE bit is left set in an intentional ibt selftest or when
>> !ibt_fatal,
>> + * the CPU will generate another missing-ENDBRANCH #CP because the
>> IBT will be
>> + * set in the WFE state upon completion of the following ERETS
>> instruction and
>> + * then the CPU will resume from the instruction that just caused
>> the previous
>> + * missing-ENDBRANCH #CP.
>> + */
>> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
>> +{
>> +    regs->fred_cs.wfe = 0;
>> +}
>> +
>>   static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long
>> error_code)
>>   {
>>       if ((error_code & CP_EC) != CP_ENDBR) {
>> @@ -88,6 +118,8 @@ static void do_kernel_cp_fault(struct pt_regs
>> *regs, unsigned long error_code)
>>           return;
>>       }
>>   +    ibt_clear_fred_wfe(regs);
>> +
>>       if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
>>           regs->ax = 0;
>>           return;
>>
>> base-commit: fe85ee391966c4cf3bfe1c405314e894c951f521
>
>
> Andrew,
>
> can you please take another look?

After discussing with Dave on IRC, the ibt_clear_fred_wfe(regs); really
needs to be inside the ibt_selftest_noendbr path.

It's a selftest where we're deliberately trying to trigger #CP, and in
the one case where we're happy should we say "yeah, safe to clobber WFE
in the interrupted context" to let execution continue.

Clobbering WFE in any other circumstance is a security-relevant bug.

~Andrew

Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Xin Li]

On 11/8/2024 12:53 PM, Andrew Cooper wrote:
>> Andrew,
>>
>> can you please take another look?
> After discussing with Dave on IRC, the ibt_clear_fred_wfe(regs); really
> needs to be inside the ibt_selftest_noendbr path.
> 

Sigh, I missed the discussion.

> It's a selftest where we're deliberately trying to trigger #CP, and in
> the one case where we're happy should we say "yeah, safe to clobber WFE
> in the interrupted context" to let execution continue.
> 
> Clobbering WFE in any other circumstance is a security-relevant bug.

I think we also need to clear WFE when !ibt_fatal.  No?

diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
index fb8f4238969e..69a34636811f 100644
--- a/arch/x86/kernel/cet.c
+++ b/arch/x86/kernel/cet.c
@@ -128,6 +128,7 @@ static void do_kernel_cp_fault(struct pt_regs *regs, 
unsigned long error_code)
	if (!ibt_fatal) {
		printk(KERN_DEFAULT CUT_HERE);
		__warn(__FILE__, __LINE__, (void *)regs->ip, TAINT_WARN, regs, NULL);
+		ibt_clear_fred_wfe(regs);
		return;
	}
	BUG();