mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
@ 2024-09-11 23:19 Xin Li (Intel)
  2024-09-11 23:35 ` Andrew Cooper
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Xin Li (Intel) @ 2024-09-11 23:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz, andrew.cooper3

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 as 1, because the indirect branch tracker will be set in
the WAIT_FOR_ENDBRANCH state upon completion of the following ERETS
instruction and the CPU will restart from the IP that just caused a
previous missing-ENDBRANCH #CP.

Clear the WFE bit to avoid dead looping in missing-ENDBRANCH #CP.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 arch/x86/kernel/cet.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
index d2c732a34e5d..a095575d4d64 100644
--- a/arch/x86/kernel/cet.c
+++ b/arch/x86/kernel/cet.c
@@ -81,6 +81,23 @@ static void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code)
 
 static __ro_after_init bool ibt_fatal = true;
 
+/*
+ * The WFE (WAIT_FOR_ENDBRANCH) bit in the augmented CS of FRED stack frame is
+ * set to 1 in missing-ENDBRANCH #CP exceptions.
+ *
+ * If the WFE bit is left as 1, the CPU will generate another missing-ENDBRANCH
+ * #CP because the indirect branch tracker will be set in the WAIT_FOR_ENDBRANCH
+ * state upon completion of the following ERETS instruction and the CPU will
+ * restart from the IP that just caused a previous missing-ENDBRANCH #CP.
+ *
+ * Clear the WFE bit to avoid dead looping due to the above reason.
+ */
+static void ibt_clear_fred_wfe(struct pt_regs *regs)
+{
+	if (cpu_feature_enabled(X86_FEATURE_FRED))
+		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) {
@@ -90,6 +107,7 @@ static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
 
 	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
 		regs->ax = 0;
+		ibt_clear_fred_wfe(regs);
 		return;
 	}
 
@@ -97,6 +115,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();

base-commit: fe85ee391966c4cf3bfe1c405314e894c951f521
-- 
2.46.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-11 23:19 [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP Xin Li (Intel)
@ 2024-09-11 23:35 ` Andrew Cooper
  2024-09-11 23:44   ` Xin Li
  2024-09-15 18:17   ` Xin Li
  2024-09-12  0:22 ` Dave Hansen
  2024-09-12  7:57 ` Peter Zijlstra
  2 siblings, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2024-09-11 23:35 UTC (permalink / raw)
  To: Xin Li (Intel), linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz

On 12/09/2024 12:19 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 as 1, because the indirect branch tracker will be set in
> the WAIT_FOR_ENDBRANCH state upon completion of the following ERETS
> instruction and the CPU will restart from the IP that just caused a
> previous missing-ENDBRANCH #CP.
>
> Clear the WFE bit to avoid dead looping in missing-ENDBRANCH #CP.
>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>

Ah - good.  Finally some evidence that this hole in CET has been plugged
by FRED.

However, I'd suggest describing it differently.


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, loose 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 intentional #CP selfchecks need to clear WFE when they
are deemed to have succeeded, now that FRED is causing the state not to
get lost.

~Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-11 23:35 ` Andrew Cooper
@ 2024-09-11 23:44   ` Xin Li
  2024-09-15 18:17   ` Xin Li
  1 sibling, 0 replies; 10+ messages in thread
From: Xin Li @ 2024-09-11 23:44 UTC (permalink / raw)
  To: Andrew Cooper, linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz

On 9/11/2024 4:35 PM, Andrew Cooper wrote:
> On 12/09/2024 12:19 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 as 1, because the indirect branch tracker will be set in
>> the WAIT_FOR_ENDBRANCH state upon completion of the following ERETS
>> instruction and the CPU will restart from the IP that just caused a
>> previous missing-ENDBRANCH #CP.
>>
>> Clear the WFE bit to avoid dead looping in missing-ENDBRANCH #CP.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> 
> Ah - good.  Finally some evidence that this hole in CET has been plugged
> by FRED.
> 
> However, I'd suggest describing it differently.

Yep, it's a much better story including historical issues with IDT.

I will use it as the comment above ibt_clear_fred_wfe() in the next
iteration, but keep the change log.  Is it okay?

> 
> 
> 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, loose 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 intentional #CP selfchecks need to clear WFE when they
> are deemed to have succeeded, now that FRED is causing the state not to
> get lost.
> 
> ~Andrew


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-11 23:19 [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP Xin Li (Intel)
  2024-09-11 23:35 ` Andrew Cooper
@ 2024-09-12  0:22 ` Dave Hansen
  2024-09-12  8:53   ` Xin Li
  2024-09-12  7:57 ` Peter Zijlstra
  2 siblings, 1 reply; 10+ messages in thread
From: Dave Hansen @ 2024-09-12  0:22 UTC (permalink / raw)
  To: Xin Li (Intel), linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz, andrew.cooper3

On 9/11/24 16:19, Xin Li (Intel) wrote:
> +/*
> + * The WFE (WAIT_FOR_ENDBRANCH) bit in the augmented CS of FRED stack frame is
> + * set to 1 in missing-ENDBRANCH #CP exceptions.

I think there's a bit of relatively irrelevant info in there.  For
instance, I don't think it's super important to mention that FRED is
involved and where the WFE bit is in memory.

FRED's involvement is kinda a no-brainer from the whole X86_FEATURE_FRED
thing, and if you're reading exception handler code and don't know that
'regs' is on the stack, this probably isn't the place to explain that.

> + * If the WFE bit is left as 1, the CPU will generate another missing-ENDBRANCH
> + * #CP because the indirect branch tracker will be set in the WAIT_FOR_ENDBRANCH
> + * state upon completion of the following ERETS instruction and the CPU will
> + * restart from the IP that just caused a previous missing-ENDBRANCH #CP.
> + *
> + * Clear the WFE bit to avoid dead looping due to the above reason.
> + */
> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
> +		regs->fred_cs.wfe = 0;
> +}

Can I suggest a slightly different comment?

/*
 * WFE==1 (WAIT_FOR_ENDBRANCH) means that the CPU expects the next ERETS
 * to jump to an ENDBR instruction. If the ENDBR is missing, the CPU
 * raises a #CP.
 *
 * Clear WFE to avoid that #CP.
 *
 * Use this function in a #CP handler to effectively give the next
 * ERETS a free pass to ignore IBT for a single instruction.
 */

I think original comment really needs a "How do I use this?" sentence or
two.

A comment at the call site also wouldn't hurt:

 	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)){
 		regs->ax = 0;
+		/* Disable IBT enforcement for one exception return: */
+		ibt_clear_fred_wfe(regs);
 		return;
 	}

I'm finding it kinda hard to concisely differentiate between the
"disable IBT at one ERETS" and "disable IBT forever", but I hope this
sounds good to folks.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-11 23:19 [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP Xin Li (Intel)
  2024-09-11 23:35 ` Andrew Cooper
  2024-09-12  0:22 ` Dave Hansen
@ 2024-09-12  7:57 ` Peter Zijlstra
  2024-09-12  8:25   ` Xin Li
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2024-09-12  7:57 UTC (permalink / raw)
  To: Xin Li (Intel)
  Cc: linux-kernel, tglx, mingo, bp, dave.hansen, x86, hpa, andrew.cooper3

On Wed, Sep 11, 2024 at 04:19:29PM -0700, Xin Li (Intel) wrote:

> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
> +		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) {
> @@ -90,6 +107,7 @@ static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
>  
>  	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
>  		regs->ax = 0;
> +		ibt_clear_fred_wfe(regs);
>  		return;
>  	}
>  
> @@ -97,6 +115,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();

So, why not clear the bit the moment we know this is CP_ENDBR?

In the fatal case, we'll hit that BUG and die anyway, nobody cares about
the tracker state in that case.

diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
index d2c732a34e5d..fde4bdd25a73 100644
--- a/arch/x86/kernel/cet.c
+++ b/arch/x86/kernel/cet.c
@@ -88,6 +88,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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-12  7:57 ` Peter Zijlstra
@ 2024-09-12  8:25   ` Xin Li
  0 siblings, 0 replies; 10+ messages in thread
From: Xin Li @ 2024-09-12  8:25 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, tglx, mingo, bp, dave.hansen, x86, hpa, andrew.cooper3

On 9/12/2024 12:57 AM, Peter Zijlstra wrote:
> On Wed, Sep 11, 2024 at 04:19:29PM -0700, Xin Li (Intel) wrote:
> 
>> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
>> +		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) {
>> @@ -90,6 +107,7 @@ static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
>>   
>>   	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
>>   		regs->ax = 0;
>> +		ibt_clear_fred_wfe(regs);
>>   		return;
>>   	}
>>   
>> @@ -97,6 +115,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();
> 
> So, why not clear the bit the moment we know this is CP_ENDBR?
> 
> In the fatal case, we'll hit that BUG and die anyway, nobody cares about
> the tracker state in that case.

I did think about it, but preferred to leave it as in BUG().

But as you have asked, will change unless someone else disagrees with a
good reason :)

> 
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index d2c732a34e5d..fde4bdd25a73 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -88,6 +88,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;
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-12  0:22 ` Dave Hansen
@ 2024-09-12  8:53   ` Xin Li
  2024-09-12 13:13     ` Andrew Cooper
  0 siblings, 1 reply; 10+ messages in thread
From: Xin Li @ 2024-09-12  8:53 UTC (permalink / raw)
  To: Dave Hansen, linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz, andrew.cooper3

On 9/11/2024 5:22 PM, Dave Hansen wrote:
> On 9/11/24 16:19, Xin Li (Intel) wrote:
>> +/*
>> + * The WFE (WAIT_FOR_ENDBRANCH) bit in the augmented CS of FRED stack frame is
>> + * set to 1 in missing-ENDBRANCH #CP exceptions.
> 
> I think there's a bit of relatively irrelevant info in there.  For
> instance, I don't think it's super important to mention that FRED is
> involved and where the WFE bit is in memory.
> 
> FRED's involvement is kinda a no-brainer from the whole X86_FEATURE_FRED
> thing, and if you're reading exception handler code and don't know that
> 'regs' is on the stack, this probably isn't the place to explain that.

I often find myself in a dilemma, should I mention some technical
background which sometimes could also be distracting :(

Based on your feedback, maybe the following is better?

static void ibt_clear_fred_wfe(struct pt_regs *regs)
{
	if (regs->fred_cs.wfe)
		regs->fred_cs.wfe = 0;
}

And we know only FRED hardware will set the WFE bit.

> 
>> + * If the WFE bit is left as 1, the CPU will generate another missing-ENDBRANCH
>> + * #CP because the indirect branch tracker will be set in the WAIT_FOR_ENDBRANCH
>> + * state upon completion of the following ERETS instruction and the CPU will
>> + * restart from the IP that just caused a previous missing-ENDBRANCH #CP.
>> + *
>> + * Clear the WFE bit to avoid dead looping due to the above reason.
>> + */
>> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
>> +		regs->fred_cs.wfe = 0;
>> +}
> 
> Can I suggest a slightly different comment?
> 
> /*
>   * WFE==1 (WAIT_FOR_ENDBRANCH) means that the CPU expects the next ERETS
>   * to jump to an ENDBR instruction. If the ENDBR is missing, the CPU
>   * raises a #CP.
>   *
>   * Clear WFE to avoid that #CP.
>   *
>   * Use this function in a #CP handler to effectively give the next
>   * ERETS a free pass to ignore IBT for a single instruction.
>   */
> 
> I think original comment really needs a "How do I use this?" sentence or
> two.
> 
> A comment at the call site also wouldn't hurt:
> 
>   	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)){
>   		regs->ax = 0;
> +		/* Disable IBT enforcement for one exception return: */
> +		ibt_clear_fred_wfe(regs);
>   		return;
>   	}
> 
> I'm finding it kinda hard to concisely differentiate between the
> "disable IBT at one ERETS" and "disable IBT forever", but I hope this
> sounds good to folks.
> 

My understanding is that a missing-ENDBRANCH #CP is triggered in two
steps:

     1) Upon completion of an indirect call/jmp, or an event return
        instruction, the CPU indirect branch tracker is put in the
        WAIT_FOR_ENDBRANCH state.

     2) As the CPU is in WAIT_FOR_ENDBRANCH state, if the instruction to
        be executed is ENDBR, the CPU indirect branch tracker exits
        WAIT_FOR_ENDBRANCH state, otherwise a #CP is generated.

So this is more of "preserve WAIT_FOR_ENDBRANCH state" or not.

IDT is unable to preserve WAIT_FOR_ENDBRANCH state when returning from
event handling, which as Andrew mentioned is a security hole.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-12  8:53   ` Xin Li
@ 2024-09-12 13:13     ` Andrew Cooper
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2024-09-12 13:13 UTC (permalink / raw)
  To: Xin Li, Dave Hansen, linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz

On 12/09/2024 9:53 am, Xin Li wrote:
> On 9/11/2024 5:22 PM, Dave Hansen wrote:
>> On 9/11/24 16:19, Xin Li (Intel) wrote:
>>> +/*
>>> + * The WFE (WAIT_FOR_ENDBRANCH) bit in the augmented CS of FRED
>>> stack frame is
>>> + * set to 1 in missing-ENDBRANCH #CP exceptions.
>>
>> I think there's a bit of relatively irrelevant info in there.  For
>> instance, I don't think it's super important to mention that FRED is
>> involved and where the WFE bit is in memory.
>>
>> FRED's involvement is kinda a no-brainer from the whole X86_FEATURE_FRED
>> thing, and if you're reading exception handler code and don't know that
>> 'regs' is on the stack, this probably isn't the place to explain that.
>
> I often find myself in a dilemma, should I mention some technical
> background which sometimes could also be distracting :(
>
> Based on your feedback, maybe the following is better?
>
> static void ibt_clear_fred_wfe(struct pt_regs *regs)
> {
>     if (regs->fred_cs.wfe)
>         regs->fred_cs.wfe = 0;
> }

static void ibt_clear_fred_wfe(struct pt_regs *regs)
{
        regs->fred_cs.wfe = 0;
}

would be better.  With any luck, the compiler would drop the if() on
your behalf, but it would still be better not to have it to start with.

>
> And we know only FRED hardware will set the WFE bit.
>
>>
>>> + * If the WFE bit is left as 1, the CPU will generate another
>>> missing-ENDBRANCH
>>> + * #CP because the indirect branch tracker will be set in the
>>> WAIT_FOR_ENDBRANCH
>>> + * state upon completion of the following ERETS instruction and the
>>> CPU will
>>> + * restart from the IP that just caused a previous
>>> missing-ENDBRANCH #CP.
>>> + *
>>> + * Clear the WFE bit to avoid dead looping due to the above reason.
>>> + */
>>> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
>>> +{
>>> +    if (cpu_feature_enabled(X86_FEATURE_FRED))
>>> +        regs->fred_cs.wfe = 0;
>>> +}
>>
>> Can I suggest a slightly different comment?
>>
>> /*
>>   * WFE==1 (WAIT_FOR_ENDBRANCH) means that the CPU expects the next
>> ERETS
>>   * to jump to an ENDBR instruction. If the ENDBR is missing, the CPU
>>   * raises a #CP.
>>   *
>>   * Clear WFE to avoid that #CP.
>>   *
>>   * Use this function in a #CP handler to effectively give the next
>>   * ERETS a free pass to ignore IBT for a single instruction.
>>   */
>>
>> I think original comment really needs a "How do I use this?" sentence or
>> two.
>>
>> A comment at the call site also wouldn't hurt:
>>
>>       if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)){
>>           regs->ax = 0;
>> +        /* Disable IBT enforcement for one exception return: */
>> +        ibt_clear_fred_wfe(regs);
>>           return;
>>       }
>>
>> I'm finding it kinda hard to concisely differentiate between the
>> "disable IBT at one ERETS" and "disable IBT forever", but I hope this
>> sounds good to folks.
>>
>
> My understanding is that a missing-ENDBRANCH #CP is triggered in two
> steps:
>
>     1) Upon completion of an indirect call/jmp, or an event return
>        instruction, the CPU indirect branch tracker is put in the
>        WAIT_FOR_ENDBRANCH state.
>
>     2) As the CPU is in WAIT_FOR_ENDBRANCH state, if the instruction to
>        be executed is ENDBR, the CPU indirect branch tracker exits
>        WAIT_FOR_ENDBRANCH state, otherwise a #CP is generated.
>
> So this is more of "preserve WAIT_FOR_ENDBRANCH state" or not.
>
> IDT is unable to preserve WAIT_FOR_ENDBRANCH state when returning from
> event handling, which as Andrew mentioned is a security hole.

I just said hole, but that was the subtext :)


But yes - mentioning ERETS is unhelpful; it's not relevant, and it
confuses how this works.

In a FRED world, WFE will be restored for the interrupted context.  Then
there's an instruction boundary (normal #RESET/MC/INIT/INTR processing),
and only on decoding the next instruction might a #CP[endbr] be raised.

WFE is a state that crosses an instruction boundary.  It is very similar
to EFLAGS.RF, existing to alter the behaviour of the *next* instruction.

~Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-11 23:35 ` Andrew Cooper
  2024-09-11 23:44   ` Xin Li
@ 2024-09-15 18:17   ` Xin Li
  2024-09-16  9:16     ` Andrew Cooper
  1 sibling, 1 reply; 10+ messages in thread
From: Xin Li @ 2024-09-15 18:17 UTC (permalink / raw)
  To: Andrew Cooper, linux-kernel
  Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz

On 9/11/2024 4:35 PM, Andrew Cooper wrote:
> On 12/09/2024 12:19 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 as 1, because the indirect branch tracker will be set in
>> the WAIT_FOR_ENDBRANCH state upon completion of the following ERETS
>> instruction and the CPU will restart from the IP that just caused a
>> previous missing-ENDBRANCH #CP.
>>
>> Clear the WFE bit to avoid dead looping in missing-ENDBRANCH #CP.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> 
> Ah - good.  Finally some evidence that this hole in CET has been plugged
> by FRED.
> 
> However, I'd suggest describing it differently.
> 
> 
> 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, loose the WFE state of the interrupted context.  And

loose -> lose?

     Xin

> 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 intentional #CP selfchecks need to clear WFE when they
> are deemed to have succeeded, now that FRED is causing the state not to
> get lost.
> 
> ~Andrew


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP
  2024-09-15 18:17   ` Xin Li
@ 2024-09-16  9:16     ` Andrew Cooper
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2024-09-16  9:16 UTC (permalink / raw)
  To: Xin Li, linux-kernel; +Cc: tglx, mingo, bp, dave.hansen, x86, hpa, peterz

On 15/09/2024 7:17 pm, Xin Li wrote:
> On 9/11/2024 4:35 PM, Andrew Cooper wrote:
>> On 12/09/2024 12:19 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 as 1, because the indirect branch tracker will be set in
>>> the WAIT_FOR_ENDBRANCH state upon completion of the following ERETS
>>> instruction and the CPU will restart from the IP that just caused a
>>> previous missing-ENDBRANCH #CP.
>>>
>>> Clear the WFE bit to avoid dead looping in missing-ENDBRANCH #CP.
>>>
>>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>>
>> Ah - good.  Finally some evidence that this hole in CET has been plugged
>> by FRED.
>>
>> However, I'd suggest describing it differently.
>>
>>
>> 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, loose the WFE state of the interrupted context.  And
>
> loose -> lose?

Yes indeed.  Sorry.

~Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-09-16  9:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-11 23:19 [PATCH v1 1/1] x86/fred: Clear the WFE bit in missing-ENDBRANCH #CP Xin Li (Intel)
2024-09-11 23:35 ` Andrew Cooper
2024-09-11 23:44   ` Xin Li
2024-09-15 18:17   ` Xin Li
2024-09-16  9:16     ` Andrew Cooper
2024-09-12  0:22 ` Dave Hansen
2024-09-12  8:53   ` Xin Li
2024-09-12 13:13     ` Andrew Cooper
2024-09-12  7:57 ` Peter Zijlstra
2024-09-12  8:25   ` Xin Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®